Skip to content

Allow implicit path parameter matching #132

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

Closed
DavidBiesack opened this issue Sep 26, 2014 · 12 comments
Closed

Allow implicit path parameter matching #132

DavidBiesack opened this issue Sep 26, 2014 · 12 comments

Comments

@DavidBiesack
Copy link

Say a Swagger 2.0 API has the following paths (I'm using YAML here, as it's easier to write and read)

...
paths:
  /products
  /products/{productId}
  /products/{productId}/design
  /products/{productId}/marketing
  /products/{productId}/engineering

I can define productId once in Parameters Definitions Object and $ref it as per Reference Object:

  /products/{productId}
        parameters:
           - { $ref: productId }
        get: ....
  /products/{productId}/design
        parameters:
           - { $ref: productId }
        get: ....
  /products/{productId}/marketing
        parameters:
           - { $ref: productId }
        get: ....
  /products/{productId}/engineering
        parameters:
           - { $ref: productId }
        get: ....
parameters:
  productId: 
       name: productId
       in: path
       type: string
       required: true

but this gets pretty verbose pretty quickly. (Note: I think the editor incorrectly flags this as an error.)

Swagger should automatically match a path parameter with a definition (by {paramName} and { name: paramName, in: path} )

I could still override it if I don't want the common def, which would be no more work than currently.

This would help greatly in cases like the above where the same path parameter is used in multiple separate paths, doubly so for paths that have multiple path parameters.

tony tam replied on this Swagger Google Groups thread that this is intentional to avoid ambiguity. I think this is unnecessarily cumbersome, repetitive, error prone, and a maintenance nightmare. I'd like this to be reconsidered, not just as a tooling convenience, but in the spec.

@webron
Copy link
Member

webron commented Sep 27, 2014

It should be noted that @fehguy suggested this should be a tooling feature rather than a spec, and I tend to agree with him.

For auto-generated specs (using the language libraries), this has no benefit. For the libraries that read the specs, it requires extra processing (to do the matching and so on).

The only tool I can think of that could benefit from it is indeed the swagger-editor for human-written specs, and there it may be a nice feature to add, but again, has no effect on the spec itself.

@DavidBiesack
Copy link
Author

Note that Swagger spec files are still source files that are read by humans and written by humans. Adding extra redundant but required code to a source file adds verbosity and dilutes the API, making it harder to grasp and understand. The API spec, as source file, is used to communicate with people, so it should include human factors in the design as well. "Convention over configuration" is very successful because of this.

@webron
Copy link
Member

webron commented Oct 17, 2014

That's where we disagree. To me, implicit declarations make it less human readable and not more, and not necessarily easier to maintain.
If you have a large file, or a set of files, and you need to check all of them to see if you indeed defined that implicit param. Or otherwise people who read it, need to try and guess where it is defined - I don't find that better, but far far worse. Again, the tools can help in avoiding repetitive declarations and even make it easier to read.

@DavidBiesack
Copy link
Author

Note that I'm not requesting an implicit declaration - the declaration is explicit in the "definitions" section. All that is implicit/default is the association from a path to that definition/declaration.

I don't think anyone would have to "guess" where it is defined; it would be in the "definitions" section, with a matching name and "in" : "path" (and tools could easily provide a mechanism to "jump" to that definition or lookup/popup the definition)

@mohsen1
Copy link
Contributor

mohsen1 commented Oct 20, 2014

I agree with @webron on this. Let's make tools nice for human and specs vebose and easy for machine.

I had this bug open in editor from a while back swagger-api/swagger-editor#96

The idea is if you type a path that has parameter in it editor recognizes it and after you hit enter, it will populate the parameters with a parameter from your path. With your example I got another idea. So now my plan is, if editor sees the path param it will lookup parametersDefinition and if sees it there, it will use $ref. It would be automatic, so not much typing. For editor I'm adding a lot autocomplete and smart suggestions. Please feel free to propose features like this.

@DavidBiesack
Copy link
Author

If you favor a "tooling" approach to this, then might I suggest a way to "hide" all the redundant autogen elements such as

  parameters:
           - { $ref: productId }

and

     required: true

that get in the way. (For an in:path parameter, required: true MUST be declared and it MUST be true so that is completely redundant/unnecessary in the spec file. Yes, I'm repeating myself but I feel the accumulated effect of these constructs obscures the more important and meaningful parts of the API spec.)

I don't mean just a folding editor as we currently see, but one that simply hides all the redundant, inferrable specification that does not add value or convey useful information to me, the person authoring the spec.

@mohsen1
Copy link
Contributor

mohsen1 commented Oct 22, 2014

Actually now that I'm thinking about it more I like your approach @DavidBiesack.

We already have a custom way of parsing $refs. In schema keys we do not require document author writes entire path if schema is local. For example $ref: Person is valid and it's equal to $ref: '#/definitions/Person'. From what I see, developers like this kind of shorthand syntax.
I will open another ticket to make this work for parameter too. It wouldn't be too hard to write a resolver for it.

Same for path parameters. We should focus developer happiness.

Here is how the resolver algorithm should work:

  1. If there is a path param in path. (/{thePath})
  2. look for parameters with name thePath
  3. If that doesn't exist in parameters array, lookup parameters for thePath and resolve to it

//cc @fehguy @earth2marsh

@DavidBiesack
Copy link
Author

+1 : " We should focus [on] developer happiness."

Thanks. As I like to say, developer experience matters. Your users are the API developers.

@hongxingli
Copy link

@mohsen1
Hi Mohsen, for actual parameter, I'd think we can allow developer to re-define it to another type, most likely a primitive type (similar as dataType in @ApiModelProperty), isn't that enough? Can we really have composite type in parameter? I'd think it's just a wrapper anyway.

Thanks,

@earth2marsh
Copy link
Member

@mohsen1 there is this old, related thread, too:
reverb#120

… which also points out how it could be applied to host and/or baseUrl

@webron
Copy link
Member

webron commented Mar 27, 2016

Parent #560.

@fehguy
Copy link
Contributor

fehguy commented Apr 11, 2016

see #633 for this issue

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

6 participants