Improve model name determination by using title information #249

macisamuele · 2018-02-09T15:11:06Z

This CR improves the model name determination strategy used during model discovery.

The original approach was:

use key for models in definition
use x-model for models around the specs

The goal of this PR tries to unify this in a common approach.
The model name is determined as:

use x-model
use title
if the model is in #/definitions use the key

WARNING: this does CR not try to improve detection of duplicated models, but it should simplify future work on that direction

coveralls · 2018-02-09T15:14:33Z

Coverage increased (+0.03%) to 98.039% when pulling 711deed on macisamuele:maci-model-detection-uses-title into 017f10c on Yelp:master.

sjaensch

I'm fine with the changes as the test shows it does what you want it to do, and that's a good thing. However, I'm suspicious of the collect_models function (see comment below).

sjaensch · 2018-02-09T16:40:16Z

bravado_core/model.py

@@ -63,12 +72,11 @@ def collect_models(container, key, path, models, swagger_spec):
    :param models: created model types are placed here
    :type swagger_spec: :class:`bravado_core.spec.Spec`
    """
-    deref = swagger_spec.deref
-    if key == MODEL_MARKER and is_object(swagger_spec, container):
+    if (key == MODEL_MARKER or key == 'title') and is_object(swagger_spec, container):


While you didn't write this code, it is quite confusing to me what key really is. From looking at how tag_modelsand collect_models is called, it should be either the dict key with which the model instance is stored, or a list index. How can it ever be MODEL_MARKER or 'title'? And conceptually, I don't think it should be both - it should be either one or the other.

I've added key=='title' in order to recognize inline models (see example below), but I agree that this increases understanding complexity.
I've also noticed that bravado-core relies on MODEL_MARKER to determine if a model type can be used (check bravado_core.model.is_model, so the added complexity will not produce the expected behavior during response unmarshaling.

In order to keep the code simpler to read I'm removing key=='title' from the callback (so inline schemas are not tagged as models -> status-quo), adding docstrings to better expose the expected callback environment. I'll add an extra callback (called bless_models that will take care of tagging inline models).

Additionally, we don't need to check for key=='title' because tag_models will tag models that have no x-model defined.

macisamuele · 2018-02-12T08:27:37Z

I've added documentation for the callbacks methods and added a new callback method called bless_models.
The new callback is in charge of tagging models discovered during spec traversing.
The difference between tag_models and bless_models is that:

tag_models tags models present on ...#/definitions defaulting the model name (if not present) to the definition key
bless_models tags all the models identified during spec traversing (inline models). If an inline model is present but no model name is assigned (x-model and title are missing) no tag will be added

sjaensch · 2018-02-12T10:05:38Z

bravado_core/model.py

-    :type schema_object_spec: dict
+    :param object_spec: specification for a swagger object
+    :type object_spec: dict
+    :param ignore_config: ignore bravado-core 'default_type_to_object' configuration


You're not ignoring the config, you're behaving as if the setting were False. Maybe that would be a better way to name and describe this parameter. no_default_type? disable_default_type?

I'll rename this in no_default_type

sjaensch · 2018-02-12T10:07:29Z

bravado_core/model.py

+
+    if (
+        not is_dict_like(model_spec) or
+        not is_object(swagger_spec, model_spec, ignore_config=True) or


It is very much unclear as to why you'd need to ignore the config (i.e. not use a default object type) in this one specific instance.

If default_type_to_object is set to true then is_object(swagger_spec, model_spec) returns True for a big variety of objects (ie. #/definitions will always be seen an object).

Adding the parameter allows us to not duplicate the check on type=="object" or "allOf" in model_spec

I'll add a comment to better document this

sjaensch · 2018-02-12T10:10:34Z

bravado_core/swagger20_validator.py

@@ -150,14 +151,14 @@ def discriminator_validator(swagger_spec, validator, discriminator_attribute, in
            message='\'{}\' is not a recognized schema'.format(discriminator_value)
        )

-    if discriminator_value == schema['x-model']:
+    if discriminator_value == schema[MODEL_MARKER]:


I think using 'x-model' instead of the constant is a feature: Changing MODEL_MARKER should break all sorts of tests. Using these unchangeable constants in test code reduces the value of the test. I do not want to make sure that the code I test uses the constant. I want to make sure it uses 'x-model'.

Sorry, commented on the wrong piece of code, but you get the idea. :)

ack ... I'll revert this change from tests

macisamuele requested a review from sjaensch February 9, 2018 15:11

sjaensch reviewed Feb 9, 2018

View reviewed changes

macisamuele force-pushed the maci-model-detection-uses-title branch from 0c05a34 to 041966e Compare February 10, 2018 11:09

macisamuele added 2 commits February 10, 2018 18:01

Improved callbacks docstrings to better explain the callback environment

d1f696c

Improve model name determination by using title information

4479b71

macisamuele force-pushed the maci-model-detection-uses-title branch from 7d666e0 to 461422a Compare February 12, 2018 08:23

sjaensch suggested changes Feb 12, 2018

View reviewed changes

macisamuele added 2 commits February 12, 2018 16:27

Add tests

656fc92

PR feedbacks

711deed

macisamuele force-pushed the maci-model-detection-uses-title branch from 461422a to 711deed Compare February 12, 2018 15:32

sjaensch approved these changes Feb 19, 2018

View reviewed changes

macisamuele merged commit c7d8c5f into Yelp:master Feb 19, 2018

macisamuele deleted the maci-model-detection-uses-title branch February 19, 2018 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve model name determination by using title information #249

Improve model name determination by using title information #249

macisamuele commented Feb 9, 2018

coveralls commented Feb 9, 2018 •

edited

Loading

sjaensch left a comment

sjaensch Feb 9, 2018

macisamuele Feb 10, 2018

macisamuele commented Feb 12, 2018

sjaensch Feb 12, 2018

macisamuele Feb 12, 2018

sjaensch Feb 12, 2018

macisamuele Feb 12, 2018

sjaensch Feb 12, 2018

sjaensch Feb 12, 2018

macisamuele Feb 12, 2018

Improve model name determination by using title information #249

Improve model name determination by using title information #249

Conversation

macisamuele commented Feb 9, 2018

coveralls commented Feb 9, 2018 • edited Loading

sjaensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macisamuele commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 9, 2018 •

edited

Loading