-
-
Notifications
You must be signed in to change notification settings - Fork 520
Add union type and subtypes check in schema model signature #1242
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
base: master
Are you sure you want to change the base?
Add union type and subtypes check in schema model signature #1242
Conversation
Could you also add some unit tests for this ? |
@fazeelghafoor Thanks for this fix! Any chance to see the extra coverage on this? It's a pity this fix is stuck and it's really important imho. |
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 obviously don't have the permissions to really review this, but this is currently a big blocker for my team to update to the latest version of django-ninja. Even when using some_field: Optional[SomeSchema]
-style annotations with pydantic 2.0, nested schemas do not work. With this PR, they do.
if get_origin(model) in UNION_TYPES: | ||
# If the model is a union type, process each type in the union | ||
for arg in get_args(model): | ||
if type(arg) is None: |
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 applied this patch locally to test things out and noticed that this is flipped.
if type(arg) is None: | |
if arg is type(None): |
arg
will be a type, and we want to check if it's NoneType
.
# check union types | ||
if get_origin(annotation_or_field) in UNION_TYPES: | ||
for arg in get_args(annotation_or_field): | ||
if type(arg) is None: |
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.
if type(arg) is None: | |
if arg is type(None): |
I tried to pick this up in #1512 |
Fixes #1153 Add support for union type field in model schema