Handle "type" being an array of strings in JSON schema converter #423
Conversation
recap/converters/json_schema.py
Outdated
| @@ -65,12 +69,22 @@ def _parse( | |||
| self.json_registry = resource @ self.json_registry | |||
| extra_attrs["alias"] = alias_strategy(resource_id) | |||
|
|
|||
| # Special handling for "type" defined as a list of strings like | |||
| # {"type": ["string", "boolean"]} | |||
| if "type" in json_schema and isinstance(json_schema["type"], list): | |||
There was a problem hiding this comment.
I think this can be put inside the match statement. Something like:
case {"type": list(type_list)}:
types = [self._parse(s, alias_strategy) for s in type_list]
return UnionType(types, **extra_attrs)
recap/converters/json_schema.py
Outdated
| # Make type nullable if it's not required, but avoid double nullable types | ||
| if ( | ||
| name not in json_schema.get("required", []) | ||
| and not field.is_nullable() |
There was a problem hiding this comment.
I think there's a nuance that's not captured here. is_nullable checks UnionType with a NullType in it. It's not checking if there's a default value. Without a default value, the field is still required (a value must be set, even if it's null).
A JSON schema field with {"type": ["string", "null"]} that's not listed in required should be optional (i.e. it should have a default null, even if it's not set explicitly in the JSON schema). I don't think current logic accounts for this.
There was a problem hiding this comment.
Ah good catch, you are correct. Updated the logic here, and the relevant test
| name="required_nullable_with__default", | ||
| default="default_value", | ||
| ), | ||
| UnionType([NullType(), StringType()], name="nullable_no_default"), |
There was a problem hiding this comment.
I believe a nullable_no_default should have defualt=None set since it's not listed in required. See my comment above.
|
@adrianisk checking in here |
|
Heyo, wrapping up a project that needs to get done today, then I'll take a look at this! Edit: or first thing tomorrow 😪 |
| :return: True if the type is nullable. | ||
| """ | ||
|
|
||
| return isinstance(self, UnionType) and NullType() in self.types |
There was a problem hiding this comment.
This logic needs to be tweaked slightly. NullType() in self.types won't work in all cases because __eq__ checks docs, alias, logic, and extra_attrs, which might vary between NullTypes. See this code where I ran across the same issue when adding SQLite client/converter.
I believe the right solution is to search self.types for an isinstance(NullType). That's what I did in my PR.
| field = field.make_nullable() | ||
| if not field.is_nullable(): | ||
| field = field.make_nullable() | ||
| if "default" not in field.extra_attrs: |
There was a problem hiding this comment.
Nit: Maybe a comment here to explain why you're setting default even though it's already set in make_nullable. It's the right logic, since is_nullable doesn't guarantee a default of none is set, but it's worth noting since it's subtle.
There was a bug in #423. Null types in a union with `extra_attrs` or other attributes set would result in `False` returned when there was in fact a NullType in the UnionType's `types` list. `__eq__` checks all attributes, so you must iterate over the UnionType's `types` attribute and look for any type with `isinstance(..., NullType) is True`. I updated the logic accordingly. In doing so, I discovered that the JSON converter logic was converting JSON schema fields of `null` type to a UnionType with a single nested NullType type. This seems wrong; I updated the test to validate that `null` JSON fields are returned as `NullType` with a default of `None`.
|
@adrianisk I've merged your PR and made the remaining tweaks here: Note that my PR results in a subtle change to the way |
There was a bug in #423. Null types in a union with `extra_attrs` or other attributes set would result in `False` returned when there was in fact a NullType in the UnionType's `types` list. `__eq__` checks all attributes, so you must iterate over the UnionType's `types` attribute and look for any type with `isinstance(..., NullType) is True`. I updated the logic accordingly. In doing so, I discovered that the JSON converter logic was converting JSON schema fields of `null` type to a UnionType with a single nested NullType type. This seems wrong; I updated the test to validate that `null` JSON fields are returned as `NullType` with a default of `None`.
|
Released in 0.12.0: |
Summary
Fixes #412
Updates to handle the case where a
typein JSON schema is an array of strings, like{"type": ["null", "string", "boolean", "number"]}. From the JSON schema docsDetails
I added tests to make sure nullable/optional types are converted correctly, and that properties of an object that are both not required & a union with null (
{"type": ["null", "string"]}) are not made "double nullable".