Skip to content
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

Handle "type" being an array of strings in JSON schema converter #423

Merged
merged 2 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions recap/converters/json_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,14 @@ def to_recap(

def _parse(
self,
json_schema: dict,
json_schema: dict | str,
alias_strategy: AliasStrategy,
) -> RecapType:
extra_attrs = {}
# Check if json_schema is just a string representing a basic type, and convert
# to a dict with a "type" property if so
if isinstance(json_schema, str):
json_schema = {"type": json_schema}
if "description" in json_schema:
extra_attrs["doc"] = json_schema["description"]
if "default" in json_schema:
Expand All @@ -66,12 +70,23 @@ def _parse(
extra_attrs["alias"] = alias_strategy(resource_id)

match json_schema:
# Special handling for "type" defined as a list of strings like
# {"type": ["string", "boolean"]}
case {"type": list(type_list)}:
types = [self._parse(s, alias_strategy) for s in type_list]
return UnionType(types, **extra_attrs)
case {"type": "object", "properties": properties}:
fields = []
for name, prop in properties.items():
field = self._parse(prop, alias_strategy)
# If not explicitly required, make optional by ensuring the field is
# nullable, and has a default
if name not in json_schema.get("required", []):
field = field.make_nullable()
if not field.is_nullable():
field = field.make_nullable()
if "default" not in field.extra_attrs:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

field.extra_attrs["default"] = None

field.extra_attrs["name"] = name
fields.append(field)
return StructType(fields, **extra_attrs)
Expand Down
8 changes: 8 additions & 0 deletions recap/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ def make_nullable(self) -> UnionType:
type_copy = RecapTypeClass(**attrs, **extra_attrs)
return UnionType([NullType(), type_copy], **union_attrs)

def is_nullable(self) -> bool:
"""
Returns True if the type is nullable.
:return: True if the type is nullable.
"""

return isinstance(self, UnionType) and NullType() in self.types
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


def validate(self) -> None:
# Default to valid type
pass
Expand Down
66 changes: 66 additions & 0 deletions tests/unit/converters/test_json_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,72 @@ def test_all_basic_types():
]


def test_nullable_types():
"""Tests nullable types (["null", "string"]), with and without default values. Also tests that nullable properties aren't
made double nullable if they're not required."""
json_schema = """
{
"type": "object",
"properties": {
"required_nullable_no_default": {"type": ["null", "string"]},
"required_nullable_with_null_default": {"type": ["null", "string"], "default": null},
"required_nullable_with_default": {"type": ["null", "string"], "default": "default_value"},
"nullable_no_default": {"type": ["null", "string"]},
"nullable_with_null_default": {"type": ["null", "string"], "default": null},
"nullable_with_default": {"type": ["null", "string"], "default": "default_value"}
},
"required": ["required_nullable_no_default", "required_nullable_with_null_default", "required_nullable_with_default"]
}
"""
Draft202012Validator.check_schema(loads(json_schema))
struct_type = JSONSchemaConverter().to_recap(json_schema)
assert isinstance(struct_type, StructType)
assert struct_type.fields == [
UnionType([NullType(), StringType()], name="required_nullable_no_default"),
UnionType(
[NullType(), StringType()],
name="required_nullable_with_null_default",
default=None,
),
UnionType(
[NullType(), StringType()],
name="required_nullable_with_default",
default="default_value",
),
UnionType([NullType(), StringType()], name="nullable_no_default", default=None),
UnionType(
[NullType(), StringType()],
name="nullable_with_null_default",
default=None,
),
UnionType(
[NullType(), StringType()],
name="nullable_with_default",
default="default_value",
),
]


def test_union_types():
json_schema = """
{
"type": "object",
"properties": {
"union": {"type": ["null", "string", "boolean", "number"]}
},
"required": ["union"]
}
"""
Draft202012Validator.check_schema(loads(json_schema))
struct_type = JSONSchemaConverter().to_recap(json_schema)
assert isinstance(struct_type, StructType)
assert struct_type.fields == [
UnionType(
[NullType(), StringType(), BoolType(), FloatType(bits=64)], name="union"
),
]


def test_nested_objects():
json_schema = """
{
Expand Down
Loading