Skip to content

fix: Fix bug parsing allOf in nested keys BNCH-20174 #29

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

Merged
merged 6 commits into from
Jan 14, 2021
Merged

fix: Fix bug parsing allOf in nested keys BNCH-20174 #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ef87d5
Remove continue
forest-benchling Jan 13, 2021
2a05ca1
Add top-level allOf check
forest-benchling Jan 13, 2021
637832a
Update poetry
forest-benchling Jan 13, 2021
3a90f2b
Add test
forest-benchling Jan 13, 2021
24a9ce6
Revert poetry.lock
forest-benchling Jan 14, 2021
c63d42f
Fix test
forest-benchling Jan 14, 2021
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
10 changes: 3 additions & 7 deletions openapi_python_client/parser/properties/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,6 @@ def build_model_property(

for key, value in all_props.items():
prop_required = key in required_set
if not isinstance(value, oai.Reference) and value.allOf:
Copy link

Choose a reason for hiding this comment

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

Hmm.. I remember this being important for some reason..
I think removing this might inlines all references rather than referencing them as objects.
You might have more context at this point though.

Copy link
Author

Choose a reason for hiding this comment

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

@dtkav Can you clarify what you mean that it might inline all references?

Copy link

Choose a reason for hiding this comment

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

I generated the benchling client with both branches, and I think this is actually fine.
I was just trying (but failing) to remember the context around the "resolved later" comment.

It look like your PR generates 40 additional models which seem to correspond to actual missing functionality. 👍

Copy link

Choose a reason for hiding this comment

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

There was a refactor upstream which I adapted the initial allOf implementation for. I think I shouldn't have included this there.
In that refactor, he changed the way property_from_data works such that it resolves references to models it has already built instead of returning a RefProperty that gets resolved later.

# resolved later
continue
prop, schemas = property_from_data(
name=key, required=prop_required, data=value, schemas=schemas, parent_name=class_name
)
Expand Down Expand Up @@ -463,9 +460,6 @@ def _property_from_data(
)
if data.anyOf or data.oneOf:
return build_union_property(data=data, name=name, required=required, schemas=schemas, parent_name=parent_name)
if not data.type:
return NoneProperty(name=name, required=required, nullable=False, default=None), schemas

if data.type == "string":
return _string_based_property(name=name, required=required, data=data), schemas
elif data.type == "number":
Expand Down Expand Up @@ -500,8 +494,10 @@ def _property_from_data(
)
elif data.type == "array":
return build_list_property(data=data, name=name, required=required, schemas=schemas, parent_name=parent_name)
elif data.type == "object":
elif data.type == "object" or data.allOf:
return build_model_property(data=data, name=name, schemas=schemas, required=required, parent_name=parent_name)
elif not data.type:
return NoneProperty(name=name, required=required, nullable=False, default=None), schemas
return PropertyError(data=data, detail=f"unknown type {data.type}"), schemas


Expand Down
76 changes: 76 additions & 0 deletions tests/test_parser/test_properties/test_model_property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,79 @@ def test_resolve_references(mocker):
assert all(p.required for p in model.required_properties)
assert sorted(p.name for p in model.optional_properties) == ["Enum", "Int"]
assert all(not p.required for p in model.optional_properties)


def test_resolve_references_nested_allof(mocker):
import openapi_python_client.schema as oai
from openapi_python_client.parser.properties import build_model_property

schemas = {
"RefA": oai.Schema.construct(
title=mocker.MagicMock(),
description=mocker.MagicMock(),
required=["String"],
properties={
"String": oai.Schema.construct(type="string"),
"Enum": oai.Schema.construct(type="string", enum=["aValue"]),
"DateTime": oai.Schema.construct(type="string", format="date-time"),
},
),
"RefB": oai.Schema.construct(
title=mocker.MagicMock(),
description=mocker.MagicMock(),
required=["DateTime"],
properties={
"Int": oai.Schema.construct(type="integer"),
"DateTime": oai.Schema.construct(type="string", format="date-time"),
"Float": oai.Schema.construct(type="number", format="float"),
},
),
# Intentionally no properties defined
"RefC": oai.Schema.construct(
title=mocker.MagicMock(),
description=mocker.MagicMock(),
),
}

model_schema = oai.Schema.construct(
type="object",
properties={
"Key": oai.Schema.construct(
allOf=[
oai.Reference.construct(ref="#/components/schemas/RefA"),
oai.Reference.construct(ref="#/components/schemas/RefB"),
oai.Reference.construct(ref="#/components/schemas/RefC"),
oai.Schema.construct(
title=mocker.MagicMock(),
description=mocker.MagicMock(),
required=["Float"],
properties={
"String": oai.Schema.construct(type="string"),
"Float": oai.Schema.construct(type="number", format="float"),
},
),
]
),
}
)

components = {**schemas, "Model": model_schema}

from openapi_python_client.parser.properties import ModelProperty, Schemas

schemas_holder = Schemas()
model, schemas_holder = build_model_property(
data=model_schema, name="Model", required=True, schemas=schemas_holder, parent_name=None
)
model.resolve_references(components, schemas_holder)
assert sorted(p.name for p in model.required_properties) == []
assert sorted(p.name for p in model.optional_properties) == ["Key"]
assert all(not p.required for p in model.optional_properties)

key_property = model.optional_properties[0]
assert isinstance(key_property, ModelProperty)
key_property.resolve_references(components, schemas_holder)
assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]
Copy link
Author

Choose a reason for hiding this comment

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

Currently fails with

>       assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]
E       AssertionError: assert ['Float'] == ['DateTime', ...at', 'String']
E         At index 0 diff: 'Float' != 'DateTime'
E         Right contains 2 more items, first extra item: 'Float'
E         Use -v to get the full diff

I can't figure out why.

Copy link

Choose a reason for hiding this comment

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

@forest-benchling I think this is because you only resolved references on the parent model, not on the key_property.

This should work if you do

Suggested change
assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]
assert isinstance(key_property, ModelProperty)
key_property.resolve_references(components, schemas_holder)
assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]

When you actually run the code generator, all models are resolved in build_schemas but we don't use that code path here.

Copy link
Author

Choose a reason for hiding this comment

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

wooo thanks @packyg

assert all(p.required for p in key_property.required_properties)
assert sorted(p.name for p in key_property.optional_properties) == ["Enum", "Int"]
assert all(not p.required for p in key_property.optional_properties)