search

Yelp / bravado-core

Other
109 stars 98 forks source link

Enhance flattening #324

Closed macisamuele closed 5 years ago

macisamuele commented 5 years ago

This PR has 2 main objectives:

The first point was noticed while doing some testing where specs are defined as dictionary with a reference to an external resource. Not setting the origin_url in bravado_core.spec.Spec constructor causes Spec.origin_url to be set to an empty string. Due to the fact that the empty string evaluates False we have the risk of not discovering models from the referenced files (as described in the documentation). An other bug is related to model name determination, during inherits discovery.

About the flattening instead the current status is that all the models in the original Spec object need to be transposed in the flattening specs and this was ensured by add_original_models_into_known_mappings (that has been removed). I'm updating this logic because it causes all the models defined into the referenced files of the specs to end up in the final flattening result even if the model definition is not used. This PR replace add_original_models_into_known_mappings with include_discriminated_models to ensure that only "useful" model definitions are propagated to the flattened specs. "useful" models are models that are used by the specs:

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.02%) to 98.36% when pulling 177bc6efd6b3ac9d15601a4feb2a46cef21d7c0f on macisamuele:maci-enhance-flattening into 229b890b24cc7aa98e4c651d2e4359b75f7582be on Yelp:master.

sjaensch commented 5 years ago

What's your thought process behind the removal of unused models? It seems like it requires more work, and the win is small. For me as a developer it would be surprising to not find a model in the Spec although it is present in the source files. There might not be a use case for us internally right now, but given that bravado-core is a general library for working with OpenAPI 2 specs I would prefer to not remove content from our internal representation unless there's a strong reason for it.

macisamuele commented 5 years ago

The thoughts are debatable and I'm open to discussion. I was working on a personal experimentation with loaders and I noticed that the resulting flattened specs do include all the objects that are discovered even if they are not used.

An example is for specs like

swagger: '2.0'
info:
  title: Test
  version: '1.0'
paths:
  /a_random_endpoint:
    get:
      responses:
        '200':
          description: 'OK'
          schema:
            $ref: 'https://petstore.swagger.io/v2/swagger.json#/definitions/Category'

flattening such specs includes a big number of models/definitions that are never used. Consider that this happens with petstore, which is a quite small spec. What would happen if we load specs of a bigger service (loading kubernetes specs for example)? A more detailed example is available on: https://gist.github.com/macisamuele/72c1b0a1dc933e2dff691f8f9814cf0d

Why am I consider this now? Working on an hackathon I was looking to approaches to load parts of swagger specs from swagger specs of other services. The starting service does already define a quite good amount of models and the other services are doing the same, which make the final list of objects huge and a lot of them are not needed at all. Additionally internally we use Yelp/swagger-gradle-codegen to generate client code starting from the swagger specs, and the presence of not used models could cause increased time for compilation and bigger artifacts (which leads to big footprint on the final apps)

Do you think that we should avoid this type of approach or revisiting it? (maybe making this behaviour configurable could be an alternative)

sjaensch commented 5 years ago

Hm, yes that makes a lot of sense - we probably don't want all of the objects defined in such external specs to be included. On the other hand we should not remove any object defined in the root definitions section, even if it's unreferenced. What do you think?

macisamuele commented 5 years ago

Seems fair to me, I'll try to have a look to it

macisamuele commented 5 years ago

@sjaensch I need to make sure that we have appropriate testing before merging this. Spec changes are related to the fact that unused models (defined into root #/definitions) are flattened, but without valid testing it would be easy to break in the future.