Skip to content

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Jul 23, 2023

Closes #6726, closes #6375,

Selected Reviewer: @dmontagu

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 23, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2e756f4
Status: ✅  Deploy successful!
Preview URL: https://9c93e4ad.pydantic-docs2.pages.dev
Branch Preview URL: https://json-encoders.pydantic-docs2.pages.dev

View logs

@hramezani hramezani force-pushed the json-encoders branch 2 times, most recently from 114a4c6 to a6c4e3b Compare July 24, 2023 09:43
@adriangb
Copy link
Member Author

please review

Comment on lines 5 to 15
@dataclass
# MYPY: error: Expression type contains "Any" (has type overloaded function) [misc]
class Foo:
foo: int


@dataclass(config=ConfigDict(title='Bar Title'))
# MYPY: error: Expression type contains "Any" (has type "Type[ConfigDict]") [misc]
# MYPY: error: Expression type contains "Any" (has type "ConfigDict") [misc]
class Bar:
bar: str
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramezani did you intentionally add this error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, It was generated by the script. Actually, we can say by your change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this test was specifically to make sure this error didn't get created. However, I've discovered that using a type alias for the callable in json_encoders seems to make this go away. (At least with mypy 1.1.1...), which I've just pushed in a commit. So hopefully that resolves the issue.

Comment on lines 240 to 241
if 'ref' in schema:
return core_schema.definition_reference_schema(schema_ref=schema['ref'], serialization=serialization) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems problematic to me — it's not currently hit by any of the tests (as determined by putting if 'ref' in schema: assert False, and it seems like in principle it could result in a definition getting removed from the schema. Does that make sense? I'm trying to look further into it now, maybe it happens to be the case that this can't cause issues...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to reproduce it (though investigating did lead me to the other comment I made, about self-referencing encoders), but yeah this does seem to make the assumption that the original definition will be present elsewhere, I'm not sure whether that is a safe assumption or not, maybe you know based on the way definitions get stored in GenerateSchema or whatever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's not needed. We're pretty good about always retuning a definitions-reference schema and putting the schema into definitions so we never really end up passing a round a schema with a 'ref'.

@dmontagu
Copy link
Contributor

Maybe enough of an edge case to ignore, but it might be nice to improve the way you can use a self-referential JSON encoder.

In particular, the following works:

from pydantic import BaseModel


class Model1(BaseModel):
    model_config = {'json_encoders': {}}
    x: 'Model1 | None'


Model1.model_config['json_encoders'][Model1] = lambda x: x.x if x.x is not None else 1
del Model1.__pydantic_core_schema__
Model1.model_rebuild(force=True)

print(Model1(x=Model1(x=None)).model_dump_json())
#> {"x":1}

But obviously this is a lot worse than doing something like model_config = {'json_encoders': {'Model1': lambda x: x.x}}

I guess maybe we can postpone this as a feature request for the future.

Copy link
Contributor

@dmontagu dmontagu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the comments I've made, this all looks good to me, so I'll approve for now so you can merge after reviewing those comments and making changes if appropriate.

adriangb and others added 3 commits July 25, 2023 02:30
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
@adriangb
Copy link
Member Author

Maybe enough of an edge case to ignore, but it might be nice to improve the way you can use a self-referential JSON encoder.
I guess maybe we can postpone this as a feature request for the future.

Yeah I don't think this worked in v1 so let's table it.

@samuelcolvin samuelcolvin added this to the 2.0.X bugfixes milestone Jul 25, 2023
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM. Do we need to add, alter or remove something in the migration guide?

adriangb and others added 3 commits July 25, 2023 04:04
Co-authored-by: Samuel Colvin <s@muelcolvin.com>
Co-authored-by: Samuel Colvin <s@muelcolvin.com>
Co-authored-by: Samuel Colvin <s@muelcolvin.com>
Comment on lines +244 to +245
f'`json_encoders` is deprecated. See https://docs.pydantic.dev/{VERSION}/usage/serialization/#custom-serializers for alternatives',
PydanticDeprecatedSince20,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is incorrect, it should only be the major and minor versions.

Maybe the solution is to have a proper warnings page like we have for user and validation errors. I can work on this today.

Fixing this should not block release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only for specific versions see #6527.

I'll provide a general fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Okay then we can just strip the patch version.

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jul 25, 2023
@pydantic-hooky pydantic-hooky bot assigned adriangb and unassigned dmontagu Jul 25, 2023
@samuelcolvin samuelcolvin changed the title Add support for json_encoders Add back support for json_encoders Jul 25, 2023
@adriangb adriangb enabled auto-merge (squash) July 25, 2023 09:55
@adriangb
Copy link
Member Author

please review

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Jul 25, 2023
@pydantic-hooky pydantic-hooky bot assigned dmontagu and unassigned adriangb Jul 25, 2023
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@adriangb adriangb merged commit 413fea3 into main Jul 25, 2023
@adriangb adriangb deleted the json-encoders branch July 25, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Appropriate replacement for json_encoders Support json_encoders in Pydantic v2
4 participants