From f5e0df39047e550e3bcc61d7389d7cbf09f47be9 Mon Sep 17 00:00:00 2001 From: Samuele Maci Date: Fri, 29 Mar 2019 01:17:23 +0100 Subject: [PATCH 1/5] Ensure that model discovery, even if the spec_url is not defined, takes care of referenced files --- bravado_core/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bravado_core/model.py b/bravado_core/model.py index 5b55f850..df99cef6 100644 --- a/bravado_core/model.py +++ b/bravado_core/model.py @@ -808,7 +808,7 @@ def _call_post_process_spec(spec_dict): if uri == spec.origin_url or re.match(r'http://json-schema.org/draft-\d+/schema', uri) } additional_uri = _get_unprocessed_uri(spec, processed_uris) - while additional_uri: + while additional_uri is not None: # Post process each referenced specs to identify models in definitions of linked files with spec.resolver.in_scope(additional_uri): _call_post_process_spec( From 33fb7ccfeb59cd8bd3fe28c6e69c2b9b96e44cd5 Mon Sep 17 00:00:00 2001 From: Samuele Maci Date: Fri, 29 Mar 2019 11:36:12 +0100 Subject: [PATCH 2/5] Ensure that inherited_name uses the shared logic to extract the model name --- bravado_core/model.py | 2 +- tests/model/create_model_type_test.py | 40 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/bravado_core/model.py b/bravado_core/model.py index df99cef6..2e3b334d 100644 --- a/bravado_core/model.py +++ b/bravado_core/model.py @@ -584,7 +584,7 @@ def create_model_type(swagger_spec, model_name, model_spec, bases=(Model,), json inherits_from = [] if 'allOf' in model_spec: for schema in model_spec['allOf']: - inherited_name = swagger_spec.deref(schema).get(MODEL_MARKER, None) + inherited_name = _get_model_name(swagger_spec.deref(schema)) if inherited_name: inherits_from.append(inherited_name) diff --git a/tests/model/create_model_type_test.py b/tests/model/create_model_type_test.py index 1984b49d..5ce9e346 100644 --- a/tests/model/create_model_type_test.py +++ b/tests/model/create_model_type_test.py @@ -3,6 +3,7 @@ import pytest from bravado_core.model import create_model_type +from bravado_core.schema import is_ref def test_pet_model(empty_swagger_spec, pet_spec): @@ -70,3 +71,42 @@ def test_deprecated_marshal_and_unmarshal(petstore_spec): assert unmarshalled_marshalled_model.id == pet_id assert unmarshalled_marshalled_model.name == pet_name assert unmarshalled_marshalled_model.photoUrls == pet_photo_urls + + +@pytest.mark.parametrize( + 'deref_value, expected_inherits', + [ + [{'type': 'object'}, []], + [{'type': 'object', 'x-model': 'GenericPet'}, ['GenericPet']], + [{'type': 'object', 'title': 'GenericPet'}, ['GenericPet']], + [{'type': 'object', 'x-model': 'GenericPet', 'title': 'overridden'}, ['GenericPet']], + ], +) +def test_create_model_type_properly_extracts_model_name(deref_value, expected_inherits): + swagger_spec = mock.Mock( + name='swagger-spec', + deref=lambda schema: deref_value if is_ref(schema) else schema, + ) + model_type = create_model_type( + swagger_spec=swagger_spec, + model_name='Dog', + model_spec={ + 'type': 'object', + 'title': 'Dog', + 'allOf': [ + { + '$ref': '#/definitions/GenericPet' + }, + { + 'properties': { + 'birth_date': { + 'type': 'string', + 'format': 'date', + }, + }, + 'required': ['birth_date'], + }, + ], + } + ) + assert model_type._inherits_from == expected_inherits From bb11090957405e6fd2f72e2c4827a87ea7902e19 Mon Sep 17 00:00:00 2001 From: Samuele Maci Date: Fri, 29 Mar 2019 01:26:47 +0100 Subject: [PATCH 3/5] Enhance flattening to filter out un-referenced models and to include polymorphic models --- bravado_core/spec_flattening.py | 54 +++++++++++--- ...tened-multi-file-multi-directory-spec.json | 32 ++++----- test-data/2.0/multi-file-recursive/aux.json | 35 ++++++++- .../flattened-multi-file-recursive-spec.json | 71 +++++++------------ .../flattened-multi-file-with-no-xmodel.json | 9 --- .../2.0/specs-with-None-in-ref/flattened.json | 4 -- 6 files changed, 114 insertions(+), 91 deletions(-) diff --git a/bravado_core/spec_flattening.py b/bravado_core/spec_flattening.py index bc167821..fd21a85e 100644 --- a/bravado_core/spec_flattening.py +++ b/bravado_core/spec_flattening.py @@ -316,20 +316,51 @@ def model_discovery(self): }, )) - def add_original_models_into_known_mappings(self): + def include_discriminated_models(self): + """ + This function ensures that discriminated models, present on the original Spec object + but not directly referenced by the flattened schema (because there is no direct $ref + attribute) are included in the final flattened specs + + NOTE: The method re-run model_discovery in case additional models have been added to + the flattened models + """ flatten_models = { # schema objects might not have a "type" set so they won't be tagged as models definition.get(MODEL_MARKER) for definition in itervalues(self.known_mappings['definitions']) } - for model_name, model_type in iteritems(self.swagger_spec.definitions): - if model_name in flatten_models: - continue - model_url = urlparse(model_type._json_reference) - self.known_mappings['definitions'][model_url] = self.descend( - value=model_type._model_spec, - ) + def unflattened_models(): + for m_name, m_type in iteritems(self.swagger_spec.definitions): + if m_name not in flatten_models: + yield m_name, m_type + + def register_unflattened_models(): + """ + :return: True if new models have been added + """ + initial_number_of_models = len(self.known_mappings['definitions']) + + modified = True + while modified: + modified = False + for model_name, model_type in unflattened_models(): + if any( + parent in flatten_models + for parent in model_type._inherits_from + ): + model_url = urlparse(model_type._json_reference) + flatten_models.add(model_name) + self.known_mappings['definitions'][model_url] = self.descend( + value=model_type._model_spec, + ) + modified = True + + return len(self.known_mappings['definitions']) != initial_number_of_models + + while register_unflattened_models(): + self.model_discovery() @cached_property def resolved_specs(self): @@ -340,9 +371,10 @@ def resolved_specs(self): self.model_discovery() # Add the identified models that are not available on the know_mappings definitions - # This could happen in case of models that have been discovered because on - # '#/definitions' of a referenced file, but not directly referenced by the specs - self.add_original_models_into_known_mappings() + # but that are related, via polymorphism (discriminator), to flattened models + # This could happen in case discriminated models are not directly referenced by the specs + # but is fair to assume that they should be on the final artifact due to polymorphism + self.include_discriminated_models() for mapping_key, mappings in iteritems(self.known_mappings): self.warn_if_uri_clash_on_same_marshaled_representation(mappings) diff --git a/test-data/2.0/multi-file-multi-directory-spec/flattened-multi-file-multi-directory-spec.json b/test-data/2.0/multi-file-multi-directory-spec/flattened-multi-file-multi-directory-spec.json index cc0311e6..cd6c30ce 100644 --- a/test-data/2.0/multi-file-multi-directory-spec/flattened-multi-file-multi-directory-spec.json +++ b/test-data/2.0/multi-file-multi-directory-spec/flattened-multi-file-multi-directory-spec.json @@ -59,28 +59,21 @@ "$ref": "#/definitions/ErrorResponse" }, { - "$ref": "#/definitions/endpoint_v1_HTTP_FORBIDDEN_action_part" + "properties": { + "action": { + "description": "name of the forbidden action", + "type": "string" + } + }, + "required": [ + "action" + ], + "type": "object", + "x-model": "endpoint_v1_HTTP_FORBIDDEN_action_part" } ], "type": "object", "x-model": "endpoint_v1_HTTP_FORBIDDEN" - }, - "endpoint_v1_HTTP_FORBIDDEN_action_part": { - "properties": { - "action": { - "description": "name of the forbidden action", - "type": "string" - } - }, - "required": [ - "action" - ], - "type": "object", - "x-model": "endpoint_v1_HTTP_FORBIDDEN_action_part" - }, - "endpoint_v1_HTTP_OK": { - "type": "object", - "x-model": "endpoint_v1_HTTP_OK" } }, "info": { @@ -126,7 +119,8 @@ "lfile:endpoint..v1..responses.yaml|..200": { "description": "HTTP/200", "schema": { - "$ref": "#/definitions/endpoint_v1_HTTP_OK" + "type": "object", + "x-model": "endpoint_v1_HTTP_OK" } }, "lfile:endpoint..v1..responses.yaml|..403": { diff --git a/test-data/2.0/multi-file-recursive/aux.json b/test-data/2.0/multi-file-recursive/aux.json index fbf9f02f..cd94f423 100644 --- a/test-data/2.0/multi-file-recursive/aux.json +++ b/test-data/2.0/multi-file-recursive/aux.json @@ -1,5 +1,33 @@ { "definitions": { + "mobile_ping": { + "allOf": [ + {"$ref": "#/definitions/ping"}, + { + "properties": { + "text": { + "type": "string" + } + } + } + ], + "type": "object", + "x-model": "mobile_ping" + }, + "phone_ping": { + "allOf": [ + {"$ref": "#/definitions/ping"}, + { + "properties": { + "duration": { + "type": "number" + } + } + } + ], + "type": "object", + "x-model": "phone_ping" + }, "ping": { "properties": { "message": { @@ -7,11 +35,16 @@ }, "pong": { "$ref": "swagger.json#/definitions/pong" + }, + "type": { + "type": "string" } }, "required": [ - "message" + "message", + "type" ], + "discriminator": "type", "type": "object", "x-model": "ping" }, diff --git a/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json b/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json index 1962cf38..4bb45310 100644 --- a/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json +++ b/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json @@ -1,61 +1,53 @@ { "definitions": { - "inline_model": { - "type": "object", - "x-model": "inline_model" - }, - "model_with_allOf_recursive": { + "mobile_ping": { "allOf": [ { - "properties": { - "pong": { - "$ref": "#/definitions/pong" - } - } + "$ref": "#/definitions/ping" }, { "properties": { - "ping": { - "$ref": "#/definitions/ping" + "text": { + "type": "string" } } } ], - "x-model": "model_with_allOf_recursive" - }, - "not_referenced_models_in_not_directly_linked_file": { - "properties": { - "inline_model": { - "$ref": "#/definitions/inline_model" - } - }, - "type": "object", - "x-model": "not_referenced_models_in_not_directly_linked_file" - }, - "not_used": { "type": "object", - "x-model": "not_used" + "x-model": "mobile_ping" }, - "not_used_remote_reference": { - "properties": { - "random_number": { - "$ref": "#/definitions/random_integer" + "phone_ping": { + "allOf": [ + { + "$ref": "#/definitions/ping" + }, + { + "properties": { + "duration": { + "type": "number" + } + } } - }, + ], "type": "object", - "x-model": "not_used_remote_reference" + "x-model": "phone_ping" }, "ping": { + "discriminator": "type", "properties": { "message": { "type": "string" }, "pong": { "$ref": "#/definitions/pong" + }, + "type": { + "type": "string" } }, "required": [ - "message" + "message", + "type" ], "type": "object", "x-model": "ping" @@ -74,21 +66,6 @@ ], "type": "object", "x-model": "pong" - }, - "random_integer": { - "properties": { - "max-value": { - "type": "integer" - }, - "min-value": { - "type": "integer" - }, - "value": { - "type": "integer" - } - }, - "type": "object", - "x-model": "random_integer" } }, "info": { diff --git a/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json b/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json index e69e60d1..890a5a64 100644 --- a/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json +++ b/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json @@ -1,14 +1,5 @@ { "definitions": { - "object": { - "properties": { - "property": { - "$ref": "#/definitions/referenced_object" - } - }, - "type": "object", - "x-model": "object" - }, "referenced_object": { "type": "object", "x-model": "referenced_object" diff --git a/test-data/2.0/specs-with-None-in-ref/flattened.json b/test-data/2.0/specs-with-None-in-ref/flattened.json index 347c6a98..6944b28e 100644 --- a/test-data/2.0/specs-with-None-in-ref/flattened.json +++ b/test-data/2.0/specs-with-None-in-ref/flattened.json @@ -8,10 +8,6 @@ } ], "x-model": "model1" - }, - "model2": { - "type": "object", - "x-model": "model2" } }, "info": { From 72de6d03124c58f7b00c33863248c5f3fcf0bd00 Mon Sep 17 00:00:00 2001 From: Samuele Maci Date: Mon, 1 Apr 2019 19:46:53 +0200 Subject: [PATCH 4/5] Preserve models defined in swagger.json#/definitions --- bravado_core/spec_flattening.py | 12 ++++++++++ .../flattened-multi-file-recursive-spec.json | 23 +++++++++++++++++++ .../flattened-multi-file-with-no-xmodel.json | 9 ++++++++ .../2.0/specs-with-None-in-ref/flattened.json | 4 ++++ 4 files changed, 48 insertions(+) diff --git a/bravado_core/spec_flattening.py b/bravado_core/spec_flattening.py index fd21a85e..5897acf4 100644 --- a/bravado_core/spec_flattening.py +++ b/bravado_core/spec_flattening.py @@ -10,6 +10,7 @@ from six import iterkeys from six import itervalues from six.moves.urllib.parse import ParseResult +from six.moves.urllib.parse import urldefrag from six.moves.urllib.parse import urlparse from six.moves.urllib.parse import urlunparse from swagger_spec_validator.ref_validators import in_scope @@ -362,6 +363,14 @@ def register_unflattened_models(): while register_unflattened_models(): self.model_discovery() + def include_root_definition(self): + self.known_mappings['definitions'].update({ + urlparse(v._json_reference): self.descend(value=v._model_spec) + for v in itervalues(self.swagger_spec.definitions) + # urldefrag(url)[0] returns the url without the fragment, it is guaranteed to be present + if urldefrag(v._json_reference)[0] == self.swagger_spec.origin_url + }) + @cached_property def resolved_specs(self): # Create internal copy of spec_dict to avoid external dict pollution @@ -370,6 +379,9 @@ def resolved_specs(self): # Perform model discovery of the newly identified definitions self.model_discovery() + # Ensure that all the root definitions, even if not referenced, are not lost due to flattening. + self.include_root_definition() + # Add the identified models that are not available on the know_mappings definitions # but that are related, via polymorphism (discriminator), to flattened models # This could happen in case discriminated models are not directly referenced by the specs diff --git a/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json b/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json index 4bb45310..9c538708 100644 --- a/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json +++ b/test-data/2.0/multi-file-recursive/flattened-multi-file-recursive-spec.json @@ -16,6 +16,29 @@ "type": "object", "x-model": "mobile_ping" }, + "model_with_allOf_recursive": { + "allOf": [ + { + "properties": { + "pong": { + "$ref": "#/definitions/pong" + } + } + }, + { + "properties": { + "ping": { + "$ref": "#/definitions/ping" + } + } + } + ], + "x-model": "model_with_allOf_recursive" + }, + "not_used": { + "type": "object", + "x-model": "not_used" + }, "phone_ping": { "allOf": [ { diff --git a/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json b/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json index 890a5a64..e69e60d1 100644 --- a/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json +++ b/test-data/2.0/multi-file-specs-with-no-x-model/flattened-multi-file-with-no-xmodel.json @@ -1,5 +1,14 @@ { "definitions": { + "object": { + "properties": { + "property": { + "$ref": "#/definitions/referenced_object" + } + }, + "type": "object", + "x-model": "object" + }, "referenced_object": { "type": "object", "x-model": "referenced_object" diff --git a/test-data/2.0/specs-with-None-in-ref/flattened.json b/test-data/2.0/specs-with-None-in-ref/flattened.json index 6944b28e..347c6a98 100644 --- a/test-data/2.0/specs-with-None-in-ref/flattened.json +++ b/test-data/2.0/specs-with-None-in-ref/flattened.json @@ -8,6 +8,10 @@ } ], "x-model": "model1" + }, + "model2": { + "type": "object", + "x-model": "model2" } }, "info": { From 177bc6efd6b3ac9d15601a4feb2a46cef21d7c0f Mon Sep 17 00:00:00 2001 From: Samuele Maci Date: Tue, 9 Apr 2019 17:59:34 +0200 Subject: [PATCH 5/5] Add test --- tests/spec/Spec/flattened_spec_test.py | 74 ++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/tests/spec/Spec/flattened_spec_test.py b/tests/spec/Spec/flattened_spec_test.py index d8a0ce65..192636c1 100644 --- a/tests/spec/Spec/flattened_spec_test.py +++ b/tests/spec/Spec/flattened_spec_test.py @@ -4,6 +4,7 @@ import mock import pytest +from six.moves.urllib.parse import urljoin from six.moves.urllib.parse import urlparse from swagger_spec_validator import validator20 @@ -12,12 +13,12 @@ from bravado_core.spec import Spec from bravado_core.spec_flattening import _marshal_uri from bravado_core.spec_flattening import _SpecFlattener +from tests.conftest import get_url -@pytest.fixture -def spec_flattener(minimal_swagger_spec): +def _spec_flattener(swagger_spec): return _SpecFlattener( - swagger_spec=minimal_swagger_spec, + swagger_spec=swagger_spec, marshal_uri_function=functools.partial( _marshal_uri, origin_uri=None, @@ -25,6 +26,11 @@ def spec_flattener(minimal_swagger_spec): ) +@pytest.fixture +def spec_flattener(minimal_swagger_spec): + return _spec_flattener(minimal_swagger_spec) + + @mock.patch('bravado_core.spec_flattening.warnings') def test_no_warning_for_clashed_uris(mock_warnings, spec_flattener): spec_flattener.warn_if_uri_clash_on_same_marshaled_representation({}) @@ -328,3 +334,65 @@ def test_referenced_and_discovered_models_are_not_lost_after_flattening(simple_c def test_specs_with_none_in_ref_spec(specs_with_none_in_ref_spec, flattened_specs_with_none_in_ref_dict): assert specs_with_none_in_ref_spec.flattened_spec == flattened_specs_with_none_in_ref_dict + + +def test_include_root_definition(minimal_swagger_dict, minimal_swagger_abspath): + minimal_swagger_dict['definitions'] = { + 'not_used_model': { + 'type': 'object', + }, + } + spec_flattener = _spec_flattener(Spec.from_dict(minimal_swagger_dict, origin_url=get_url(minimal_swagger_abspath))) + + spec_flattener.include_root_definition() + + fragment_uri = urljoin('file:', '{}#/definitions/not_used_model'.format(minimal_swagger_abspath)) + assert spec_flattener.known_mappings['definitions'] == { + urlparse(fragment_uri): minimal_swagger_dict['definitions']['not_used_model'], + } + + +def test_include_discriminated_models(minimal_swagger_dict, minimal_swagger_abspath): + minimal_swagger_dict['definitions'] = { + 'base': { + 'type': 'object', + 'properties': { + 'discriminator_field': {'type': 'string'}, + }, + 'discriminator': 'discriminator_field', + 'required': ['discriminator_field'], + }, + 'not_used_extend_base': { + 'allOf': [ + {'$ref': '#/definitions/base'}, + { + 'properties': { + 'text': {'type': 'string'}, + }, + }, + ], + }, + } + spec_flattener = _spec_flattener(Spec.from_dict(minimal_swagger_dict, origin_url=get_url(minimal_swagger_abspath))) + + base_fragment_uri = urljoin('file:', '{}#/definitions/base'.format(minimal_swagger_abspath)) + spec_flattener.known_mappings['definitions'] = { + urlparse(base_fragment_uri): minimal_swagger_dict['definitions']['base'], + } + + spec_flattener.include_discriminated_models() + + not_used_extend_base_fragment_uri = urljoin( + 'file:', + '{}#/definitions/not_used_extend_base'.format(minimal_swagger_abspath), + ) + assert spec_flattener.known_mappings['definitions'] == { + urlparse(base_fragment_uri): minimal_swagger_dict['definitions']['base'], + urlparse(not_used_extend_base_fragment_uri): { + 'allOf': [ + mock.ANY, # not checking the exact content as it contains a marshaled reference and x-scope + minimal_swagger_dict['definitions']['not_used_extend_base']['allOf'][1] + ], + 'x-model': 'not_used_extend_base', + }, + }