Skip to content

Conversation

vadim-su
Copy link
Contributor

@vadim-su vadim-su commented Jul 1, 2023

Change Summary

In some cases like that:

import enum
from typing import Annotated, Literal

from pydantic import BaseModel, Field


class ComponentType(enum.IntEnum):
        SomeComponent1 = 1
        SomeComponent2 = 2

class SomeComponent1(BaseModel):
    type: Literal[ComponentType.SomeComponent1] = ComponentType.SomeComponent1
    
class SomeComponent2(BaseModel):
    type: Literal[ComponentType.SomeComponent2] = ComponentType.SomeComponent2

Component = Annotated[SomeComponent1|SomeComponent2, Field(discriminator='type')]

class TestModel1(BaseModel):
    components: list[Component]
    
class TestModel2(BaseModel):
    components: list[Component]
    
class TestModel3(BaseModel):
    submodels: TestModel1|TestModel2

apply_discriminators function could process tagged-unions because the tagged-union contains discriminator, but doesn't take that the schema was processed in another model. I excluded tagged-unions from the recursive processing.

I'm not sure about the place of the fix. Possibly a better place for this is _ApplyInferredDiscriminator.apply

self.definitions.update(collect_definitions(schema))
assert not self._used
schema = self._apply_to_root(schema)
if self._should_be_nullable and not self._is_nullable:
schema = core_schema.nullable_schema(schema)
self._used = True
return schema

Related issue number

Fix #6339

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @lig

@vadim-su
Copy link
Contributor Author

vadim-su commented Jul 1, 2023

please review

union_model: UnionModel

class TestModel(BaseModel):
submodel: Union[SubModel1, SubModel2]
Copy link
Contributor

Choose a reason for hiding this comment

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

@suharnikov any chance you could add a bit of validation here just to make sure it actually behaves correctly during validation, not just that it stops erroring while building the schema

Copy link
Member

Choose a reason for hiding this comment

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

Also a json schema check would be good

@adriangb adriangb enabled auto-merge (squash) July 1, 2023 15:53
@adriangb adriangb merged commit 1943e9d into pydantic:main Jul 1, 2023
@samuelcolvin
Copy link
Member

We need to start requiring change files.

@vadim-su
Copy link
Contributor Author

vadim-su commented Jul 1, 2023

I missed all the fun 😅

@vadim-su vadim-su deleted the fix_tagged_unions_in_submodels branch July 1, 2023 20:56
@a10y
Copy link

a10y commented Jul 4, 2023

@samuelcolvin / @dmontagu Is there going to be a minor release sometime soon with this fix? I use nested discriminated unions pretty extensively in my API so this is currently blocking my upgrade to v2

EDIT: This is now in 2.0.3, thanks all!

a10y added a commit to IntrinsicLabsAI/intrinsic-model-server that referenced this pull request Jul 6, 2023
## Bump to Pydantic v2

This PR brings the repo onto Pydantic v 2.0.2 and FastAPI v
0.100.0-beta3 (0.100.0 is expected to cut by EOW and this is the last
beta release).

* Rework how we do custom tagged union types
* The new paradigm for this involves extending a `RootModel` generic
type from pydantic, which I find a bit gross but it is the new supported
way to do this so 🤷
* Use all of the new `model_` methods that were renamed for AFAICT no
particular reason
* Bump the FE generated types to use the new SemVer type alias in the
FE.

- [X] Blocked until we get a new release
pydantic/pydantic#6340 (comment)
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.

TypeAdapter doesn't work properly with tagged-unions
6 participants