-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(api): Create workaround for null union with multiple types #64834
fix(api): Create workaround for null union with multiple types #64834
Conversation
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.
🙌 nice! Thanks for figuring this out!
@@ -146,7 +146,16 @@ def resolve_type_hint(hint) -> Any: | |||
else: | |||
schema = resolve_type_hint(type_args[0]) | |||
if type(None) in args: | |||
schema["nullable"] = True | |||
# There's an issue where if 3 types are OR'd together and one of |
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.
wanna double check, what does this function look like on drf-spectacular's main branch now? do we need this shim anymore now that we're on an up to date python version?
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.
Hmm I think we might be able to use the original now. The typeddict part looks like it'll work, the only difference is it doesn't look like it excludes fields like ours. in _resolve_typeddict
?
It seems the very recent version also now supports openapi 3.1 which drops nullable
in favor of null
, although I don't see the code doing this in the Union check in resolve_type_hint
🤔
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.
Actually I just saw this: tfranzel/drf-spectacular#925
Someone brought up this exact null issue
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.
requesting changes until we verify the drf-spectacular's upstream doessn't already have this fix
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.
thanks @schew2381 for checking. if you get time, could be worth trying to contribute the "excluded_fields" change upstream, and perhaps showing what we're doing to attempt to fix in that thread.
if we can get both of those things figured out, would be lovely to be able to get rid of this shim!
82cd478
to
d3a5b66
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #64834 +/- ##
===========================================
+ Coverage 63.45% 81.38% +17.92%
===========================================
Files 5218 5241 +23
Lines 231033 231351 +318
Branches 45341 45380 +39
===========================================
+ Hits 146608 188288 +41680
+ Misses 78854 37212 -41642
- Partials 5571 5851 +280
|
d3a5b66
to
888c014
Compare
See comment in code for detail