Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set pydantic json mode to serialization #1156

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

codebutler
Copy link
Contributor

See: pydantic/pydantic#7457 (comment)

This fixes the schema for Decimal types.

Before:

d:
  anyOf:
  - type: number
  - type: string
  title: D
  type: string

After:

d:
  title: D
  type: string

@tfranzel
Copy link
Owner

Hi! So I skimmed over the other issue and I'm not sure this is an improvement here. What exactly do we fix here and what do we gain from this narrowing?

@codebutler
Copy link
Contributor Author

The number type in json-schema (and javascript) is floating point and can't be safely used to serialize decimals. This is why Pydantic serializes decimals to strings: https://docs.pydantic.dev/latest/api/standard_library_types/#decimaldecimal

The schemas generated by drf-spectacular describe language-agnostic serialization (rather than python-specific validation) and should require decimals be encoded as strings.

@tfranzel
Copy link
Owner

I get your point now, but I'm not entirely convinced that everybody wants to have it that way.

Actually DRF has a dedicated feature flag for this question: https://www.django-rest-framework.org/api-guide/settings/#coerce_decimal_to_string Maybe we should use it for this also? We build the serializer.DecimalField also dependent on that flag.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (db414d6) 98.58% compared to head (525475f) 98.58%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1156   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files          72       72           
  Lines        8766     8768    +2     
=======================================
+ Hits         8642     8644    +2     
  Misses        124      124           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@microHoffman
Copy link

microHoffman commented May 28, 2024

Related to this: having some kind of option for passing a mode arg that is passed to pydantic's model_json_schema in PydanticExtension would be also helpful for cases when you use a model with AliasGenerator(serialization_alias=...) specified like this:

class MyModel(BaseModel):
    model_config = ConfigDict(alias_generator=AliasGenerator(serialization_alias=to_camel))

    my_property: str

In current mode the drf-spectacular will always generate a schema with my_property in snake_case, as there is no way to tell the model_json_schema in PydanticExtension that I would like to use a mode="serialization" for the schema generation.

I can open a PR if you agree this would be helpful. If you have some tips how to best implement this, lmk!

@tfranzel
Copy link
Owner

tfranzel commented Jul 1, 2024

Thanks for chiming in @microHoffman. That is another good point for this. Another point was also just raised in #1258.

I think now with those 3 points, the benefits outweigh the potential issues this may bring. In general it seems that serialization mode is the more correct thing to do anyway.

Let's merge it and see what happens 😄

@tfranzel tfranzel merged commit cc91637 into tfranzel:master Jul 1, 2024
35 checks passed
@microHoffman
Copy link

microHoffman commented Aug 20, 2024

Hello @tfranzel , just a note that this is possibly unexpected schema breaking change for some users - just because someone has a serialization_alias set on pydantic model, it does not necessarily mean that they are using it for serializing the data from the endpoint, as in pydantic you need to write explicitly MyPydanticModelInstance.model_dump(by_alias=True)...

Second case is e.g. if you use serialization_alias only for the responses, but for the request body you want to use validation_alias (or just keep the original name) - in a way it's done after merging this change, it's impossible to say to drf-spectacular to not use the serialization mode.

In my opinion ideal would be to be able to somehow explicitly pass to the drf-spectacular what mode of serialization you use for each endpoint (and ideally for request and response separately), e.g. via @extend_schema decorator or in some other way. Being able to pass the serialization mode would be imho best and most flexible solution, and would not necessarily mean potentially breaking change in schema generation for some people and so this would be easier to release without complications.

What do you think?

@tfranzel
Copy link
Owner

@microHoffman thank you for your thorough analysis. As we already noted, this should be the better solution and not necessarily the best one. I still think it is an improvement.

Second case is e.g. if you use serialization_alias only for the responses, but for the request body you want to use validation_alias (or just keep the original name) - in a way it's done after merging this change, it's impossible to say to drf-spectacular to not use the serialization mode.

Do you mean that is impossible currently to generate a request/response schema currently? Maybe injecting a custom generator would be an idea for this?
https://docs.pydantic.dev/latest/concepts/json_schema/#top-level-schema-generation

e.g. via @extend_schema

Can't really apply this here and changing the decorator would be too big of an impact. Apart from that you would want to patch the fields and that would be @extend_schema_field instead.

If anything I think this is an improvement over before and pydantic support is by design never going to be perfect as it is simply a convenience without any conventions (wrt. DRF).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants