-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add JSON schema support (v2) #5029
Conversation
@samuelcolvin there's still a lot of work to be done before the JSON schema work is "finished", but I think I am starting to add too much to reasonably review. I think it would be better to stop new work on this branch, and just get everything it adds reviewed and merged before continuing. There are already enough decisions in here that may be controversial that I think it's probably a mistake to continue building on them before establishing more thorough agreement. I will be happy to remove things from this branch (such as my possibly-half-baked implementation of handling |
@dmontagu ah, very clever! And good point that subclassing I think this idea of including errors in the schema with a flag should work. My only fear is including two types of things in the same object (schema and errors), but at the same time, this would only happen when enabling the flag, so whoever does that (me) would have to know what they are doing and know what to expect, so I think that might be enough. And this is probably the simplest solution, not affecting anyone else nor changing much the return type annotations. I was also thinking about an alternative implementation separating the errors and typing it with Not sure if you would rather keep the conversation here in a central place, or there in the other PR to avoid my extra noise here, let me know if you would prefer to switch over there! |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking awesome.
I'm in favour of merging it asap and creating separate issues for outstanding problems & discussions.
Thanks for the replies to all the points @samuelcolvin! All looks good, and agreed it makes sense in subsequent PRs.
|
There is an important consideration here that I discuss in #5072, which is that when generating clients based on an OpenAPI spec (one of the biggest benefits of FastAPI imo, and one of the main reasons I have used it for years), models are frequently used both as inputs (so will be "validated"), and as outputs (so will be "serialized"). And unless you make two separate models (which comes with its own issues), they will share a schema. So it's not uncommon that you'll have to have one schema for the both inputs and outputs, and therefore need to resolve this. And even if we were okay with creating two separate schemas for the model based on whether it is an "input" or an "output" of the API — and I am not sure we should be, at least in the context of FastAPI — there's still the issue that we'd probably want a single generator instance to produce the "output" format in some places and the "input" format in others, within the same schema (at least that's the closest to how FastAPI does it today, I think). So I'm thinking it may make more sense to somehow make it an annotation on the core schema (that would be set by FastAPI/similar), as opposed to on the generator. I'm not sure, but I'd be inclined to postpone addressing this in this PR and hopefully have some discussion in #5072 |
@tiangolo Yeah this was my plan for how to do this. We can remove the overload down the line after people have had time to migrate, and it will still cause mypy/IDE errors for them now. 👍 |
from your suggestion to @tiangolo's feature request: schema_generator = schema_generator_cls(by_alias=by_alias, ref_template=ref_template)
s = s.generate(cls.__pydantic_core_schema__)
if s.errors:
raise PydanticInvalidForJsonSchema(...)
The main shortcoming I see with this is providing context about where the error was raised. Right now we don't have a good system for understanding the "path" that was taken through the crazy recursive function calls to produce the error, which I think may be necessary for the kind of errors @tiangolo wants to show. I am not opposed to implementing that logic in principle, but that logic would end up reflecting the structure of the CoreSchema more than the JsonSchema (when they differ anyway), which I could imagine leading to some confusion. (And would require some logic..) Maybe you've thought of a good way to do it. Another downside to always deferring error raising is that it makes it a lot more annoying to debug when you want it to raise, and see where it was raised. If we add a deferred-error-collection mode I would suggest we still retain the ability to raise immediately (through one choice of an 'errors' kwarg or similar) so that we can easily get a stack trace to where the exception was raised when desired. Either way, I think let's create a separate issue for this. (Actually I might just open a PR making the change I suggested above that at least retains information about where the error is within the final json schema, at least then the alternative is made concrete.) |
Thanks so much for this @dmontagu, amazing to have this merged. On the error stuff, my instinct is that the traceback might not be that useful anyway, we could even store the traceback with the errors if really necessary, but I'm also not that bothered about it. It might be easier to just add a kwarg config setting to |
@Julian I've done some work on JSON schema for discriminated unions in #5051; I know it's more OpenAPI than JSON schema, so not sure if it's something you can help with, but I would appreciate any insight there. (And thanks for your offer to look at the JSON schema stuff, whether or not you can help in this particular case.) |
fix #4666
Things left to resolve:
URLsCreated an issue for this: JSON schema: URL #5095Json type
Callable typeCreated an issue for this: JSON schema: Callable #5096tests.test_schema.test_callable_type
)DecimalRelevant issues: Decimal JSON encoder is lossy #1511 and v2 JSON schema — parsing vs. serialization #5072__pydantic_modify_json_schema__
__modify_schema__
, or force migration to__pydantic_modify_json_schema__
?__modify_schema__
; new syntax in__pydantic_json_schema__
__pydantic_modify_json_schema__
besides the schema itself?'definitions'
->'$defs'
'definitions'
to'$defs'
everywhere? This is in line with JSON schema 2020-12, but it would be nice to confirm this won't break any important tooling (e.g., openapi-generator).tests.test_schema.test_schema_with_refs
; openapi doesn't actually use the definitions key anyway, and FastAPI will change out the$defs
key as appropriate.)