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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1fd345
Handle "type" being an array of strings
adrianisk Jan 29, 2024
193e411
Fix optional logic
adrianisk Feb 6, 2024
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
18 changes: 16 additions & 2 deletions recap/converters/json_schema.py
View file
Open in desktop
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 @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call

types = [self._parse(s, alias_strategy) for s in json_schema["type"]]
return UnionType(types, **extra_attrs)

match json_schema:
case {"type": "object", "properties": properties}:
fields = []
for name, prop in properties.items():
field = self._parse(prop, alias_strategy)
if name not in json_schema.get("required", []):
# 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@adrianisk adrianisk Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah good catch, you are correct. Updated the logic here, and the relevant test

):
field = field.make_nullable()
field.extra_attrs["name"] = name
fields.append(field)
Expand Down
8 changes: 8 additions & 0 deletions recap/types.py
View file
Open in desktop
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
View file
Open in desktop
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"},
adrianisk marked this conversation as resolved.
Show resolved Hide resolved
"nullable_no_default": {"type": ["null", "string"]},
"nullable_with_null_default": {"type": ["null", "string"], "default": null},
"nullable_with__default": {"type": ["null", "string"], "default": "default_value"}
adrianisk marked this conversation as resolved.
Show resolved Hide resolved
},
"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"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe a nullable_no_default should have defualt=None set since it's not listed in required. See my comment above.

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