diff --git a/linodecli/baked/response.py b/linodecli/baked/response.py index fd20fbc72..901ece294 100644 --- a/linodecli/baked/response.py +++ b/linodecli/baked/response.py @@ -210,8 +210,10 @@ def _parse_response_model(schema, prefix=None, nested_list_depth=0): ) elif v.type == "object": attrs += _parse_response_model(v, prefix=pref) - elif v.type == "array" and v.items.type == "object": - # Parse arrays for objects recursively and increase the nesting depth + elif v.type == "array" and ( + v.items.type == "object" + or bool(_aggregate_schema_properties(v.items)[0]) + ): attrs += _parse_response_model( v.items, prefix=pref, diff --git a/linodecli/baked/util.py b/linodecli/baked/util.py index fef1a179c..d2106695c 100644 --- a/linodecli/baked/util.py +++ b/linodecli/baked/util.py @@ -8,6 +8,67 @@ from openapi3.schemas import Schema +# The maximum schema nesting depth `_schema_richness` will traverse before +# bailing out. This is purely a safety valve to guarantee termination on +# self-referential or pathologically deep schemas (which the recursion would +# otherwise follow forever); it is far deeper than any real Linode API response +# model, so it never affects scoring in practice. +_MAX_RICHNESS_DEPTH = 32 + + +def _schema_richness(schema: Any, _depth: int = 0) -> int: + """ + Estimates how complete a schema definition is, used to decide which + definition to keep when the same property appears in multiple composition + (oneOf/allOf/anyOf) branches. + + A branch that nulls a property out (e.g. ``{"type": "object", "nullable": + true}`` with no properties) should never overwrite a branch that fully + defines that property's nested structure. The score is a recursive measure + of how much structure a schema actually contains, so a fuller definition always outscores a + sparser one regardless of the + order the branches appear in. + + :param schema: The schema (or raw schema dict) to score. + :return: A non-negative integer; higher means more complete. + """ + + # Guard against pathologically deep or self-referential schemas. + if _depth > _MAX_RICHNESS_DEPTH: + return 0 + + def get(source: Any, attr: str) -> Any: + if isinstance(source, dict): + return source.get(attr) + return getattr(source, attr, None) + + score = 0 + + # Count each defined property, plus the richness of its own definition so + # that deeply-nested structure contributes to the total. + properties = get(schema, "properties") + if properties: + for _, prop in properties.items(): + score += 1 + _schema_richness(prop, _depth + 1) + + # Account for composite (oneOf/allOf/anyOf) definitions by summing the + # richness of each branch. + for composition_field in ("oneOf", "allOf", "anyOf"): + for branch in get(schema, composition_field) or []: + score += 1 + _schema_richness(branch, _depth + 1) + + # Account for array item schemas so arrays of objects are scored by their + # element structure. + array_items = get(schema, "items") + if array_items is not None: + score += _schema_richness(array_items, _depth + 1) + + additional_properties = get(schema, "additionalProperties") + if additional_properties is not None: + score += 1 + _schema_richness(additional_properties, _depth + 1) + + return score + def _aggregate_schema_properties( schema: Schema, @@ -48,7 +109,20 @@ def __inner( return # This is a valid option - properties.update(entry.properties) + for key, value in entry.properties.items(): + # When the same property is defined in multiple composition + # branches (e.g. a oneOf of interface variants that each define + # `public`, `vpc`, `vlan`, etc.), keep the most complete + # definition instead of letting a later, emptier branch overwrite + # it. Otherwise nested fields like `public.ipv6.ranges.range` + # would be silently dropped when a subsequent branch nulls the + # property out. + if key in properties and _schema_richness( + value + ) <= _schema_richness(properties[key]): + continue + + properties[key] = value nonlocal schema_count schema_count += 1 diff --git a/tests/fixtures/operation_oneof_property_overwrite.yaml b/tests/fixtures/operation_oneof_property_overwrite.yaml new file mode 100644 index 000000000..70bcb8f12 --- /dev/null +++ b/tests/fixtures/operation_oneof_property_overwrite.yaml @@ -0,0 +1,97 @@ +openapi: 3.0.1 +info: + title: API Specification + version: 1.0.0 +servers: + - url: http://localhost/v4 + +paths: + /foo/bar: + x-linode-cli-command: foo + put: + summary: Update something. + operationId: fooBarPut + description: This is description + requestBody: + description: Some description. + required: True + content: + application/json: + schema: + $ref: '#/components/schemas/Interface' + responses: + '200': + description: Successful response + content: + application/json: + schema: + $ref: '#/components/schemas/Interface' + +components: + schemas: + # This schema reproduces the real-world case where a response is a oneOf + # of variants, and every variant defines the SAME set of top-level keys, + # but only fully populates the one relevant to that variant while nulling + # out the others. A naive dict.update() merge lets the last branch + # overwrite the fully-populated definitions from earlier branches. + Interface: + oneOf: + - title: Variant A + type: object + properties: + variant_a: + type: object + properties: + ranges: + type: array + items: + type: object + properties: + range: + type: string + description: The variant A range. + variant_b: + type: object + nullable: true + # An array whose items are a oneOf of SCALAR types. This must be + # treated as a normal array attribute, not recursed into. + scalar_choices: + type: array + items: + oneOf: + - type: string + - type: integer + # This shared object is defined more richly in the LATER branch + # The fuller definition must win regardless of branch + # order, so all of its nested fields must survive aggregation. + shared_obj: + type: object + properties: + only_a: + type: string + description: Present in both branches. + - title: Variant B + type: object + properties: + variant_a: + type: object + nullable: true + variant_b: + type: object + properties: + label: + type: string + description: The variant B label. + shared_obj: + type: object + properties: + only_a: + type: string + description: Present in both branches. + extra_b1: + type: string + description: Only defined in the richer Variant B branch. + extra_b2: + type: string + description: Only defined in the richer Variant B branch. + diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 5a6dfc297..f8f9ef991 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -366,6 +366,28 @@ def post_operation_with_one_ofs() -> OpenAPIOperation: ) +@pytest.fixture +def put_operation_with_oneof_property_overwrite() -> OpenAPIOperation: + """ + Creates an OpenAPI operation whose request/response is a oneOf of variants + that each define the same top-level keys, but only fully populate the key + relevant to that variant (nulling the others). Used to verify that + aggregating oneOf branches does not let a later, emptier branch overwrite a + fully-defined property from an earlier branch. + """ + + spec = _get_parsed_spec("operation_oneof_property_overwrite.yaml") + + path = list(spec.paths.values())[0] + + return make_test_operation( + path.extensions.get("linode-cli-command", "default"), + getattr(path, "put"), + "put", + path.parameters, + ) + + @pytest.fixture def get_openapi_for_api_components_tests() -> OpenAPI: """ diff --git a/tests/unit/test_response.py b/tests/unit/test_response.py index 9f4670f42..f5e3632a3 100644 --- a/tests/unit/test_response.py +++ b/tests/unit/test_response.py @@ -118,6 +118,60 @@ def test_handle_one_ofs(self, post_operation_with_one_ofs): assert attr_map[k].datatype == v[0] assert attr_map[k].description == v[1] + def test_oneof_property_not_overwritten( + self, put_operation_with_oneof_property_overwrite + ): + """ + Regression test: when a response is a oneOf of variants that each define + the same top-level keys (fully populating only one per branch and nulling + the rest), aggregating the branches must not let a later, emptier branch + overwrite a fully-defined property from an earlier branch. + """ + model = put_operation_with_oneof_property_overwrite.response_model + + attr_paths = {attr.path for attr in model.attrs} + + # variant_a is fully defined only in the first branch and nulled in the + # second; its nested field must survive aggregation. + assert "variant_a.ranges.range" in attr_paths + # variant_b is fully defined only in the second branch. + assert "variant_b.label" in attr_paths + + def test_scalar_oneof_array_not_dropped( + self, put_operation_with_oneof_property_overwrite + ): + """ + Regression test: an array whose items are a oneOf of scalar types + (e.g. ``items: {oneOf: [{type: string}, {type: integer}]}``) aggregates + no object properties. It must remain a normal array attribute instead of + being recursed into and silently dropped from the response model. + """ + model = put_operation_with_oneof_property_overwrite.response_model + + attr_paths = {attr.path for attr in model.attrs} + + assert "scalar_choices" in attr_paths + + def test_richer_oneof_branch_wins_regardless_of_order( + self, put_operation_with_oneof_property_overwrite + ): + """ + Regression test: when the same object property is defined in multiple + branches, the branch with the most nested structure must win even if it + appears later. A presence-only richness score would tie and keep the + earlier, sparser definition, dropping the extra fields. + """ + model = put_operation_with_oneof_property_overwrite.response_model + + attr_paths = {attr.path for attr in model.attrs} + + # Defined in both branches. + assert "shared_obj.only_a" in attr_paths + # Only defined in the richer (later) Variant B branch; these would be + # missing if the earlier, sparser definition were kept. + assert "shared_obj.extra_b1" in attr_paths + assert "shared_obj.extra_b2" in attr_paths + def test_fix_json_string_type(self, list_operation_for_response_test): model = list_operation_for_response_test.response_model model.rows = ["foo.bar", "type"]