Fix deserialization for str unions #1239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alicederyn
added
semver:patch
PR argoproj-labs#1168 changed the logic of map_runner_input and _parse to no longer pass incoming values to json.loads if their annotated type was a union that included str, rather than only when given a subtype of `Optional[str]`. Split `origin_type_issubclass` into two functions, `origin_type_issupertype` (which matches the previous behaviour) and `origin_type_issubtype`, and use the latter instead to restore the original behaviour. Add a runner check which verifies this behaviour. Signed-off-by: Alice Purcell <[email protected]>
alicederyn
force-pushed
the
revert._is_str_kwarg_of.change
branch
from
9192b57
to
52437be
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1239 +/- ##
=====================================
Coverage 45.9% 45.9%
=====================================
Files 60 60
Lines 4082 4096 +14
Branches 857 861 +4
=====================================
+ Hits 1875 1884 +9
- Misses 2175 2180 +5
Partials 32 32 ☔ View full report in Codecov by Sentry. |
parse_obj_as does not appear to support union types (it is converting ints to strings). Signed-off-by: Alice Purcell <[email protected]>
alicederyn
commented
Oct 15, 2024
alicederyn
requested review from
flaviuvadan,
sambhav and
elliotgunton
as code owners
jeongukjae
reviewed
Oct 15, 2024
Comment on lines
-128
to
+134
| return origin_type_issubclass(func_param_annotation, str) | ||
| return origin_type_issubtype(func_param_annotation, (str, NoneType)) |
Signed-off-by: Alice Purcell <[email protected]>
If origin_type_issubtype or origin_type_issupertype are passed a special form annotation, they will raise a TypeError due to passing it into issubclass. Fix this issue by first checking if the annotation is a type, and returning False if not. Unit test this with NoReturn, as we are very unlikely to ever create a special case for this type in the function. Signed-off-by: Alice Purcell <[email protected]>
The origin_type_is* functions were accidentally calling get_args on the original annotation rather than the unwrapped type. Signed-off-by: Alice Purcell <[email protected]>
alicederyn
force-pushed
the
revert._is_str_kwarg_of.change
branch
from
5f03b16
to
c434384
Compare
sambhav
reviewed
Oct 21, 2024
Pydantic v1 checks union types in declaration order, returning as soon as a coercion succeeds; see https://docs.pydantic.dev/1.10/usage/model_config/#smart-union for more. Change the test str_or_int_parameter signature from `str | int` to `int | str` to work with this legacy mode. Signed-off-by: Alice Purcell <[email protected]>
sambhav
approved these changes
Oct 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Description of PR
PR #1168 changed the logic of
map_runner_inputand_parseto no longer pass incoming values tojson.loadsif their annotated type was a union that included str, rather than only when given a subtype ofOptional[str]. This makes it impossible to, for instance, pass an int to aUnion[str, int].This PR splits
origin_type_issubclassinto two functions,origin_type_issupertype(which matches the previous behaviour) andorigin_type_issubtype, and use the latter instead to restore the original behaviour.Additionally, it fixes a bug where we were passing an annotation to
issubclasswithout first checking if it's a type, resulting in aTypeError(partially addresses #1173).