Yelp / bravado-core

Other
109 stars 98 forks source link

Normalise URIs #313

Closed macisamuele closed 5 years ago

macisamuele commented 5 years ago

While investigating internal issues I did noticed that the result of spec flattening is dependent on how the references looks like.

Specs like

swagger: '2.0'
info:
  title: Test
  version: '1.0'
definitions:
  text:
    type: string
paths:
  /endpoint:
    get:
      responses:
        '200':
          description: ''
          schema:
            $ref: '#/definitions/text'
        '300':
          description: ''
          schema:
            $ref: '#definitions/text'

would be flattened in something like

definitions:
  'lfile:.|..definitions..text':
    type: string
  'lfile:.|definitions..text':
    type: string
info:
  title: Test
  version: '1.0'
paths:
  /endpoint:
    get:
      responses:
        '200':
          description: ''
          schema:
            $ref: '#/definitions/lfile:.|..definitions..text'
        '300':
          description: ''
          schema:
            $ref: '#/definitions/lfile:.|definitions..text'
swagger: '2.0'

instead of

definitions:
  'lfile:.|..definitions..text':
    type: string
info:
  title: Test
  version: '1.0'
paths:
  /endpoint:
    get:
      responses:
        '200':
          description: ''
          schema:
            $ref: '#/definitions/lfile:.|..definitions..text'
        '300':
          description: ''
          schema:
            $ref: '#/definitions/lfile:.|..definitions..text'
swagger: '2.0'

Note the duplicate definition of text (as lfile:.|..definitions..text and lfile:.|definitions..text)

This is caused by the fact that the original spec have two equivalent but not equal references to text (#/definitions/text and #definitions/text).

The goal of this PR is to ensure that once we're building a bravado_core.spec.Spec object we're performing some form of minimal normalisation of URIs such that duplication of definitions is reduced in case of flattening

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.007%) to 98.341% when pulling d8d7a397009d45db69656fadcf22e5a06ae1395a on macisamuele:maci-normalise-uris into cde813a6b89e2cf8322e0ad737f294955a512bad on Yelp:master.

sjaensch commented 5 years ago

I don't think what you're doing here is semantically correct. That first slash has a meaning: it identifies the value as a JSON pointer (RFC 6901). JSON schema does provide the possibility to define relative references without using JSON pointers, by using id (documentation - note that Swagger 2 is using JSON Schema draft v4, where $id was still called id).

So what should happen in this case is that we should raise an error for the value #definitions/text, probably by saying that we can't find that reference.

macisamuele commented 5 years ago

Going to drop this for now. I'm not 100% sure about the change after more carefully reading the JSON pointer RFC