Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need a way to accommodate reusable YAML without violating schema or cluttering JSON #107

Open
earth2marsh opened this issue Aug 21, 2014 · 36 comments

Comments

@earth2marsh
Copy link

A benefit of YAML is that you can define a snippet of YAML once and refer back to it in many places.

However, that snippet is converted into JSON and then triggers a schema violation. Perhaps we should contain all snippets in a parent, such as snippets and then add that as an object in the JSON-schema. Furthermore, we should consider stripping the snippet element from the rendered JSON.

Here is an example:

ExampleQueryParam: &example
  name: example
  description: something something
  required: false

# refer to it later
myArray:
  - name: foo
  - <<: *example
@fehguy
Copy link

fehguy commented Aug 21, 2014

The problem I see with YAML anchors is several fold:

  1. They take a lot of getting used to
  2. They can cause "magic", where the source of the content can be hard to trace back
  3. They don't really encourage reuse--they encourage DRY and the expense of reuse

I think item 3 is very important. The JSON pointer DOES encourage both DRY and reuse, because the locations and resolution are constrained by the schema. So I could make everyone crazy by using anchors to refer to a parameter description. But json pointers will let the unit of reuse be something more predictable.

Perhaps there's a way to mix the JSON pointer idea into a YAML syntax that makes sense. I don't know if YAML anchors are a good idea, though.

@earth2marsh
Copy link
Author

I don't see the right answer yet. I will try to state the problem more succinctly, howeve…

It sucks to have to write the same thing over and over again, such as limit and offset parameters for paging through collections. Swagger 2.0 needs a way to define these cleanly once and then refer to them over and over again. It doesn't have to be native YAML-snippets, but it is arguably MOST important for YAML, since it is a top-down format where reusability is particularly important.

What do you suggest, @fehguy ?

@fehguy
Copy link

fehguy commented Aug 26, 2014

OK I've hit a snag. We can allow references as such:

"/pets/{id}": {
  "get": {
    "description": "Returns pets based on ID",
    "summary": "Find pets by ID",
    "operationId": "getPetsById",
    "parameters": [
      { "$ref": "#/definitions/skipParam" },
      { "$ref": "#/definitions/limitParam" }
    ],

however, we cannot define a skipParam in the schema, because the definitions key is restricted to models only.

I see a couple of options.

  1. We support the /definitions section of the schema for model schemas. We can add a /parameters section which allows valid parameter references to be added

  2. We can relax the constraints in the /definitions section to allow for anyOf, which means the /definitions can be either model schemas, parameter schemas, etc.

  3. We can support shared parameters on only external files.

@earth2marsh thoughts?

@jewest27
Copy link

@fehguy @earth2marsh I have been working on a few different models and hit this concern. My preference is to have a top-level 'parameters' section that can be validated against the parameter schema and if needed individual parameter definitions can reference the models in 'definitions'. That way if you want to reuse a model for multiple parameters you can do that. Although this may make Jeremy cringe...

WDYT?

@fehguy
Copy link

fehguy commented Aug 28, 2014

@jwest-apigee @earth2marsh two thoughts.

  1. we can structure the reusable components in the schema like I suggested earlier.
  2. we can leave the /definitions section "unvalidated" by itself, and require each object in there to be validated based on how it's used. So you can have parameters, models, operations all in there, however, they are validated by their types, which are determined by how they're referenced.

Thinking about it more, I think that's much cleaner, and more extensible.

@jewest27
Copy link

I understand the benefit of having parameters in /definitions and it is definitely a valid approach. However, I like having parameter definitions in #/parameters such that it can be clean and explicit. It makes tooling easier since it is explicit where to find the model for which type of 'thing'. It is much easier to validate, semantically and structurally. It may also be easier to understand for newcommers to Swagger.

Also, the model for a request/response is basically free-form and used by the API implementation being described. The model/schema for the structure of a parameter within a Swagger doc is more strict, used within the context of the Swagger doc and we expect certain fields to be there that are not used by the API implementation.

@jewest27
Copy link

Here is an example of how I envision it working:

swagger: 2
info:
  title: Petstore Sample API
  description: A sample API that uses a petstore as an example to demonstrate features in the swagger-2.0 specification
  version: 1.0.0
  termsOfService: Do whatever you want with this
  license:
    name: MIT
    url: "http://github.com/gruntjs/grunt/blob/master/LICENSE-MIT"
consumes:
  - application/json
paths:
  /pets/{petId}:
    get:
      description: Specific image for a specific pet
      parameters:
        - $ref: "#/parameters/petId"
          required: true
      responses:
        "200":
          description: to test 200
        default:
          description: to test default
  /pets/{petId}/images:
    get:
      description: "All images for a pet"
      parameters:
        - $ref: "#/parameters/petId
          required: true
      responses:
        "200":
          description: to test 200
        default:
          description: to test default
  /pets/{petId}/images/{imageId}:
    get:
      parameters:
        - $ref: "#/parameters/petId"
          required: true
        - name: imageId
          required: true
      description: Specific image for a specific pet
      responses:
        "200":
          description: to test 200
          schema:
            $ref: petImage
        default:
          description: to test default
parameters:
  petId:
    name: petId
    in: path
    description: "The UUID of a Pet"
definitions:
  petImage:
    required:
      - imageId
    properties:
      imageId:
        type: string
        description: Unique entity ID

@fehguy
Copy link

fehguy commented Aug 29, 2014

understood. I'm guessing that's the simplest way to go

@fehguy
Copy link

fehguy commented Aug 29, 2014

updated here:

#114

@earth2marsh
Copy link
Author

👍

@mohsen1
Copy link

mohsen1 commented Aug 29, 2014

OH now we are back to definitions vs models. With above changes, definitions is not JSON-Schema definitions anymore because it's not generally for definitions and it's restricted to models only. Now, shouldn't we rename it back to models?

We need to advise API composers that the shortcut for $ref where you didn't have to put explicti path is only for models.

In @jwest-apigee's example, we can't do the following, right?

...
paths:
  /pets/{petId}:
    get:
      description: Specific image for a specific pet
      parameters:
        - $ref: petId
          required: true
...

@fehguy
Copy link

fehguy commented Aug 29, 2014

no, we've undone that. there are two things:

EDIT: should be definitions schemas: this represents model schemas

parameters: this represents parameter definitions

please see this example:
https://github.com/reverb/swagger-spec/blob/master/fixtures/v2.0/json/resources/reusableParameters.json

@mohsen1
Copy link

mohsen1 commented Aug 29, 2014

Also, can $ref reference be cascaded the way Jeff's examples is written?

      parameters:
        - $ref: petId
          required: true #this will get ignored with current parser

@fehguy
Copy link

fehguy commented Aug 29, 2014

not quite, it will look like this:

        parameters: 
          - $ref: "#/parameters/skipParam"
          - $ref: "#/parameters/limitParam"

note: no overriding a referenced parameter

@earth2marsh earth2marsh reopened this Aug 29, 2014
@earth2marsh
Copy link
Author

@fehguy it should also be possible to say:

parameters: 
          - $ref: "skipParam"
          - $ref: "limitParam"

… right?

I love having a top level parameters element. One interesting implication is that we now have a potential improvement to how you represent path parameters. Since path params are always implied by {paramName}, it becomes possible to treat the braces as the implicit parameter key and look there automatically. (Ideally you wouldn't have to specify that it was required, too, based on in: path.)

… continuing the logic, we also have a mechanism for defining parameters in the host and baseUrl elements. :)

@mohsen1
Copy link

mohsen1 commented Aug 29, 2014

Now that we are at it, let's make one for responses too!

@jewest27
Copy link

@earth2marsh @mohsen1 lets open up separate issues for those things and close this one to keep it clean and focused :)

@earth2marsh
Copy link
Author

@mohsen1 I'm ok with co-mingling request and response objects. Both are JSON (potentially other formats), and there may be reuse. For example, I might post a user object, but i'll also expect that same user object when I do a GET.

@fehguy
Copy link

fehguy commented Aug 29, 2014

note, you can also do this:

parameters: 
   - $ref: http://yourcompany.com/definitions/skipParam
   - $ref: http://yourcompany.com/definitions/limitParam

the reference to #/parameters would need to be implicit in the tooling.

@earth2marsh
Copy link
Author

Do we need the name in a parameter definition?

parameters:
  petId:
    name: petId
    in: path
    description: "The UUID of a Pet"

Also, is it possible to make a parameter required at the definition level and then override it in the method? (I believe, yes)

@earth2marsh
Copy link
Author

@jwest-apigee @fehguy See https://gist.github.com/earth2marsh/264123aefc2e29551d9d#file-uber-yaml-L130 for an example using Uber.

Two things to notice:

  1. the parameter $ref is in its shortest form
  2. name key is omitted from the parameter definition

@jewest27
Copy link

jewest27 commented Sep 3, 2014

#/parameters/{name} refers to the element in the YAML document structure whereas ‘name: petId’ refers to the content that is interpreted by the tooling, no?

I believe ‘required’ only applies to the place where it is used, not necessarily globally.


Jeff West | Products | apigee | m: +1.214.450.9378 | twitter @jeffreyawest @apigee | Skype jeffrey.a.west
I ♥ APIs 2014 | September 8-10 | Fort Mason Center | San Francisco | #iloveapis | iloveapis2014.com

On Sep 3, 2014, at 2:19 PM, Marsh Gardiner [email protected] wrote:

@jwest-apigee @fehguy See https://gist.github.com/earth2marsh/264123aefc2e29551d9d#file-uber-yaml-L130 for an example using Uber.

Two things to notice:

  1. the parameter $ref is in its shortest form
  2. name key is omitted from the parameter definition


Reply to this email directly or view it on GitHub.

@mohsen1
Copy link

mohsen1 commented Sep 3, 2014

@jwest-apigee has a good point. Unfortunately JSON-Schema's $ref is not cascading. So if someone has a parameter which is required is some places they need to make two parameters, one required and one optional.

#....
      parameters:
        - $ref: required_foo
parameters:
  required_foo:
    in: query
    type: number
    required: true
    description: A foo.

I personally don't like it. Any better solutions?

@mohsen1
Copy link

mohsen1 commented Sep 3, 2014

One solution is to put required values outside of the parameters

#...
      parameters:
        - $ref: "start_latitude"
        - $ref: "start_longitude"
     requiredParams:
        - start_latitude
        - start_longitude
parameters:
  start_latitude:
    in: query
    type: number
    description: Latitude component of start location.
  start_longitude:
    in: query
    type: number
    description: Longitude component of start location.

@jewest27
Copy link

jewest27 commented Sep 3, 2014

see my previous example (snippet below). the point of the $ref is to have one definition of that parameter. Sometimes it may be required, sometimes not - therefore it doesn't make sense to me to include it in the #/parameters/petId section.

  /pets/{petId}/images/{imageId}:
    get:
      parameters:
        - $ref: "#/parameters/petId"
          required: true
  /pets/{petId}/something_else:
    get:
      parameters:
        - $ref: "#/parameters/petId"
          required: false
parameters:
  petId:
    name: petId
    in: path
    description: "The UUID of a Pet"

@mohsen1
Copy link

mohsen1 commented Sep 3, 2014

We can make this work:

        - $ref: "#/parameters/petId"
          required: true

But then it's not following JSON Schema standard.

that YAML translated to JSON is this:

{
      "$ref": "#/parameters/petId",
      "required": true
}

JSON-Schema says you need to replace the object with it's $ref. So the required key will get lost :(

@jewest27
Copy link

jewest27 commented Sep 3, 2014

I see. We’re in a difficult spot with the stage this change is being made in with regard to tooling. Given that this doesn’t work it looks like we need an alternative that we can agree on quickly.

Thoughts, @fehguy? @earth2marsh?


Jeff West | Products | apigee | m: +1.214.450.9378 | twitter @jeffreyawest @apigee | Skype jeffrey.a.west
I ♥ APIs 2014 | September 8-10 | Fort Mason Center | San Francisco | #iloveapis | iloveapis2014.com

On Sep 3, 2014, at 2:53 PM, Mohsen Azimi [email protected] wrote:

We can make this work:

    - $ref: "#/parameters/petId"
      required: true

But then it's not following JSON Schema standard.

that YAML translated to JSON is this:

{
"$ref": "#/parameters/petId",
"required": true
}
JSON-Schema says you need to replace the object with it's $ref. So the required key will get lost :(


Reply to this email directly or view it on GitHub.

@fehguy
Copy link

fehguy commented Sep 3, 2014

Yes, you cannot merge the required:true with the ref, or the json schema compliance is broken.

Think of the json pointers as just that--simply pointers, not mix-ins.

@earth2marsh
Copy link
Author

Might not work… but what about some riff on:

  /pets/{petId}/images/{imageId}:
    get:
      required: 
        - petId
      parameters:
        - $ref: petId
parameters:
  petId:
    name: petId
    in: path
    description: "The UUID of a Pet"

@mohsen1
Copy link

mohsen1 commented Sep 3, 2014

That's the same as my proposal. The problem is, it's not following semantic hierarchy the rest of the document is following. required belongs to parameters so it should be under it

@fehguy
Copy link

fehguy commented Sep 3, 2014

cascading will mean "detailed object clobbers general object", not a merge. In the long run, this will be much better than complex merge strategies.

@webron
Copy link

webron commented Sep 21, 2014

@earth2marsh, @mohsen1 - Is this still an open issue?

@earth2marsh
Copy link
Author

@webron, I'm uncertain. @fehguy has argued, this is a question of tooling, which may mean we should handle this in the Swagger Editor as an accommodation for authoring specs. If it has been ruled out of the Swagger spec, then perhaps this issue should be closed.

@webron
Copy link

webron commented Sep 21, 2014

It sounds like you're forcing me to read the issue ;)

@webron
Copy link

webron commented Sep 21, 2014

@earth2marsh - ok, that was a bit TLTR but I think I managed to get the gist of it. It looks like the original issue of references for parameters (and responses) has been added.
Is the current issue regarding the merging of top level definitions with local ones (such as the required and in and so on)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants