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

Add discriminated union support (v2) #5051

Merged
merged 18 commits into from
Mar 15, 2023
Merged

Add discriminated union support (v2) #5051

merged 18 commits into from
Mar 15, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Feb 14, 2023

Note to self: Once PR #5125 is merged, I'll want to merge in the changes from the v2-discriminated-union-generics branch.

There are still some remaining challenges, but I think this is mostly good:

  • Handling models with __root__ (I think this is getting removed in v2 though)
  • Handling aliases properly (will do that in the future once more things with alias are handled properly)

fix #4675

@dmontagu
Copy link
Contributor Author

dmontagu commented Feb 23, 2023

@Julian I called out a few cases of more schemas in the tests making use of the OpenAPI discriminator. (Will probably be better to view the final file than the diff in the files view). I recognize this is OpenAPI-specific, but I would be interested to get your take on:

  1. Would you consider this to be valid JSON schema? (And appropriate for the relevant types.)
    • If not, is there anything I can do to help it be more compliant? (E.g., something something vocabularies/extensions/???, or maybe adding a flag for whether to include the discriminator stuff in the generated schema, i.e., whether to include openapi extensions.)
  2. Does this seem to be valid for OpenAPI (not sure if that's your wheelhouse or not)?

I did run some examples through some web-based JSON schema validators and it seemed to respect the discriminator stuff properly, but I don't necessarily view that as authoritative 😄.

@dmontagu dmontagu mentioned this pull request Feb 23, 2023
11 tasks
Copy link

@Julian Julian left a comment

Choose a reason for hiding this comment

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

Left a comment or two which hopefully is helpful to think about, though may be undesirable implementation-wise.

Happy to keep looking at pings of course!

tests/test_json_schema.py Show resolved Hide resolved
tests/test_json_schema.py Show resolved Hide resolved
tests/test_json_schema.py Outdated Show resolved Hide resolved
tests/test_json_schema.py Show resolved Hide resolved
@dmontagu dmontagu self-assigned this Feb 28, 2023
@dmontagu
Copy link
Contributor Author

dmontagu commented Mar 3, 2023

@samuelcolvin Would you be open to adding jsonschema as a testing dependency for the sake of validating generated schemas? I like the idea, if for no other reason than making sure we recognize if/when we make a change that isn't compatible with whatever draft we target (i.e., 2020-12 for now).

@samuelcolvin
Copy link
Member

@samuelcolvin Would you be open to adding jsonschema as a testing dependency for the sake of validating generated schemas? I like the idea, if for no other reason than making sure we recognize if/when we make a change that isn't compatible with whatever draft we target (i.e., 2020-12 for now).

yup, fine by me.

@dmontagu
Copy link
Contributor Author

dmontagu commented Mar 9, 2023

This should be ready for review now, there's nothing else I intend to do before merging (unless I get feedback to the contrary).

pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/_internal/_generate_schema.py Outdated Show resolved Hide resolved
pydantic/json_schema.py Outdated Show resolved Hide resolved
@dmontagu
Copy link
Contributor Author

please review

@okke-formsma
Copy link

okke-formsma commented Mar 11, 2023

I've been fighting with pydantic + jsonschema for a bit, and one of the reasons is that while pydantic claims to support json-schema, there are some things like discriminated unions in it that are openAPI-specific.

For the unexpecting user of Json Schema and Pydantic, it is rather surprising that non-standard json schema extensions are included without them being labeled as such.

Should v2 of pydantic not have a "target" or "type" option in the schema method that specifies the target to build the schema for? (e.g., 'OpenAPI' or 'json-schema' or maybe even 'json-schema-2020-12' for when they inevitably come up with another non-backward-compatible json schema release)

@dmontagu
Copy link
Contributor Author

@okke-formsma I think you aren't the first person to mention wanting the ability to request specific JSON schema behavior along these lines. Fortunately, we've made a lot of improvements to the modularity of the JSON schema generation as part of v2, and it should be much easier to modify it to suit various needs (whether we end up supporting that directly or leave it to user code).

There are a handful of more-minor JSON schema issues that still need to be resolved before v2 is released, and the integration with FastAPI still needs to happen. I am not exactly sure what the best API would be yet for controlling the JSON schema behavior, but I will note that it should be pretty easy to exclude the discriminated union stuff in user code currently on V2 — if I understand how JSON schema should work correctly, you'd just need to subclass GenerateJsonSchema and replace the current logic for handling tagged unions with (essentially) the same logic used in regular unions. There is a semi-related example of how to override JSON schema generation in v2 in tests/test_fastapi_json_schema.py.

If you have opinions about the API you'd like for controlling what kind of (pseudo?-)JSON schema should be produced (e.g., with or without OpenAPI), feel free to create an issue about it. I would be open to including a strictly-JSON-schema generating class as an alternative to what is currently called GenerateJsonSchema, and possibly tweaking the method signature for model_json_schema to perhaps have some special allowed string values for schema_generator for some "official" choices.

All that said, I think for the sake of this PR I don't want to get too caught up with the JSON schema details, since this PR is more focused on achieving parity with the v1 behavior. But definitely happy to put more thought into how we can improve the JSON schema APIs before v2 is released.

@okke-formsma
Copy link

Thanks for your response. That sounds like a pretty good solution. I'm definitely interested in the ability to subclass the schema generation, but I'll chime in in a more appropriate place.

@dmontagu dmontagu merged commit 14ebd21 into main Mar 15, 2023
@dmontagu dmontagu deleted the v2-discriminated-union branch March 15, 2023 14:52
@Kludex
Copy link
Member

Kludex commented May 31, 2023

Thanks for your response. That sounds like a pretty good solution. I'm definitely interested in the ability to subclass the schema generation, but I'll chime in in a more appropriate place.

I was talking to @dmontagu , and he implemented it on a conversation we had:

from pydantic_core import core_schema

from pydantic.json_schema import GenerateJsonSchema, JsonSchemaValue


class GenerateStrictJsonSchema(GenerateJsonSchema):
    def tagged_union_schema(
        self, schema: core_schema.TaggedUnionSchema
    ) -> JsonSchemaValue:
        schema = schema.copy()
        schema.pop("discriminator")
        schema = dict(
            **schema,
            type="union",
            choices=[x for x in schema["choices"].values() if isinstance(x, dict)]
        )
        return self.union_schema(schema)

The above should be enough. 🙏

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.

V2: Descriminated Unions
8 participants