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

pipeDelimited and spaceDelimited style examples can be unresolvable #3737

Open
charjr opened this issue Apr 22, 2024 · 10 comments
Open

pipeDelimited and spaceDelimited style examples can be unresolvable #3737

charjr opened this issue Apr 22, 2024 · 10 comments
Assignees
Labels
clarification requests to clarify, but not change, part of the spec param serialization Issues related to parameter and/or header serialization
Milestone

Comments

@charjr
Copy link

charjr commented Apr 22, 2024

What the Specification states

According to the style examples:

style explode array object
form false color=blue,black,brown color=R,100,G,200,B,150
form true color=blue&color=black&color=brown R=100&G=200&B=150
spaceDelimited false blue%20black%20brown R%20100%20G%20200%20B%20150
pipeDelimited false blue|black|brown R|100|G|200|B|150

When I read the rows for form I observe that the parameter name is explicitly used within the examples: color=blue,black,brown for instance.

So naturally, when I do not see the name explicitly used in spaceDelimited or pipeDelimited examples my conclusion is that the names are not used.

Problem with statement

Imagine the following example:

  /pets:
    get:
      parameters:
        - name: type
          in: query
          style: pipeDelimited
          schema:
            type: array
            items: 
              type: string
        - name: personality
          in: query
          style: spaceDelimited
          schema:
            type: array
            items:
              type: string

A valid url according to the spec would be:

  • /pets?cat|shorthair|calico&confident%20greedy%20loud
    • not too bad, we could identify each parameter based on the styles.
  • /pets?cat|shorthair|calico&loud
    • Okay... no idea what style loud is... but if we can work out the first part then loud can be deduced by a process of elimination.
  • /pets?cat&loud
    • hm.

Using the Swagger Editor with this spec it seems to think that the urls would come out as follows:

  • /pets?type=cat|shorthair|calico&personality=confident%20greedy%20loud
  • /pets?type=cat|shorthair|calico&personality=loud
  • /pets?type=cat&personality=loud

All of which would immediately be identifiable based on the name. I realise the Swagger Editor is not the source of all knowledge, but the urls it generated matched what I would have expected.

Considering that form, spaceDelimited and pipeDelimited, are based off of 2.0's collectionFormat I was wondering if spaceDelimited, and pipeDelimited were meant to have the name explicitly used in the same way as form?

@handrews handrews added clarification requests to clarify, but not change, part of the spec param serialization Issues related to parameter and/or header serialization labels Apr 22, 2024
@handrews
Copy link
Contributor

@charjr My initial thought was that the style table only shows parameter values, but some of them clearly incorporate the name. I belive that the Swagger editor is correct and in compliance with the spec as written, but the table is somewhat misleading.

@karenetheridge have you run into this? I know you improved some things in this table recently.

@charjr
Copy link
Author

charjr commented Apr 22, 2024

@handrews @karenetheridge If the tables need updating, I can provide some PRs for each. Just let me know whether it would be better to:

  • add parameter names where missing
  • remove parameter names where specified

I also have to question the style:form explode:true type:object column.
The Swagger Editor and the OpenAPI Spec agree that the name of the object does not appear in the query string. Because of this, it's liable to conflict with any query parameter of any style if it has a property of the same name as another parameter.

Example:

      parameters:
        - name: pet
          in: query
          required: true
          schema:
            type: object
            properties:
              name:
                type: string
              id:
                type: integer
        - name: id
          in: query
          required: true
          schema:
            type: integer
  • pet expects a string like name=gato&id=5
  • id expects a string like id=1
  • Together they expect a string like name=gato&id=5&id=0

Default style and explode for both and it is impossible to differentiate which id is which.

PS: I'm not saying the above example is a good API, my current knowledge leads me to believe if you create an api like the above, then you should fully expect it to clash.
That said I'm working on a library that validates requests based on an OpenAPI, So I'm trying to clarify whether my knowledge is incorrect or if I should warn users against creating a conflicting API like the above example.

@handrews
Copy link
Contributor

See also:

@karenetheridge
Copy link
Contributor

I agree, I believe that for the examples given in the spec, the URI would look like https://example.com/endpoint?color=blue%20black%20brown and so on -- that is we would have ?<name>=<value> in all cases and the style/explode parameters are indicating how to parse the "value" portion, not the entire query string.

The examples at https://swagger.io/docs/specification/serialization/#query also back this up, that the parameter name does appear in the query portion of the URI.

@handrews
Copy link
Contributor

@karenetheridge would you be interested in writing a 3.0.4 PR for this? If so I can assign this issue to you and add it to the 3.0.4 milestone.

@karenetheridge
Copy link
Contributor

karenetheridge commented Apr 24, 2024

If it's not a direct copy-paste of what we'd do for 3.1.0, then I'm not the right person for that, as I've not ever used 3.0.x in any serious way and don't have good command of its differences from 3.1.

@handrews
Copy link
Contributor

handrews commented Apr 24, 2024

@karenetheridge the style examples table is identical in 3.0.3 and 3.1.0. I'd also be perfectly happy if you want to do a 3.1.1 PR and then leave porting it to 3.0.4 to me. We can also just raise this in the Thursday meeting and see if anyone else has bandwidth to do it.

@karenetheridge
Copy link
Contributor

following up on your point about style=form, explode=true, type=object:

I also have to question the style:form explode:true type:object column.
The Swagger Editor and the OpenAPI Spec agree that the name of the object does not appear in the query string. Because of this, it's liable to conflict with any query parameter of any style if it has a property of the same name as another parameter.

Yes, that was my interpretation as well when I implemented style=form parsing of query strings. When the indicated schema type is 'object', all query parameters in the URI are sucked up into that parameter. There could be another parameter as well, separately, but it would get a duplicate of that parameter. However, this is not reversible (

Following your example:

pet expects a string like name=gato&id=5 --> yes.
id expects a string like id=1 --> yes.
Together they expect a string like name=gato&id=5&id=0 -> no, because this is not reversible: I would expect, when attempting to deserialize this query string into data structures that could be validated with json schema, that we would either ignore the second id parameter or we would error that an unexpected duplicate is present. Therefore my hope would be that a serializer would detect that id appears twice (with different values) and generate an error.

Using this query string: name=gato&id=5 and the schema you presented above, I would expect to see this deserialization:

query parameter "pet":
{
  "name": "gato",
  "id": 5
}

query parameter "id": 5

We could also reconcile this in the specification by explicitly saying that if a parameter of type object is present, there shall be no other parameters allowed using the same name as one of these object's properties. e.g. if you use a style=form, explode=true, type=object parameter, the intent is that this parameter consumes all values in the query string and no other query parameters are permitted.

This issue does not extend to the other styles, because & is not used as a delimiter within those other styles. It is only the 'form' type that poses a problem because & is used as the delimiter between both the individual properties+values, and between the parameters themselves.

karenetheridge added a commit to karenetheridge/OpenAPI-Specification that referenced this issue Apr 25, 2024
These should have the parameter name in the resulting string (see issue OAI#3737).
karenetheridge added a commit to karenetheridge/OpenAPI-Specification that referenced this issue Apr 25, 2024
These should have the parameter name in the resulting string (see issue OAI#3737).
karenetheridge added a commit to karenetheridge/OpenAPI-Specification that referenced this issue Apr 25, 2024
These should have the parameter name in the resulting string (see issue OAI#3737).
@charjr
Copy link
Author

charjr commented Apr 25, 2024

We could also reconcile this in the specification by explicitly saying that if a parameter of type object is present, there shall be no other parameters allowed using the same name as one of these object's properties. e.g. if you use a style=form, explode=true, type=object parameter, the intent is that this parameter consumes all values in the query string and no other query parameters are permitted.

I agree, that clarification would be good and make it easier for tools to parse query strings while being compliant with the spec.

This issue does not extend to the other styles, because & is not used as a delimiter within those other styles. It is only the 'form' type that poses a problem because & is used as the delimiter between both the individual properties+values, and between the parameters themselves.

According to the Swagger docs It would also affect pipeDelimited and spaceDelimited with explode:true which has the effect of looking like style:form, explode:true.
Since the OpenAPI Specification does not state otherwise, anyone reading both will assume this is the case.

I think clarification, in one of two ways, would help:
1 - explicitly disallow explode:true on both styles (it's completely pointless and adds two lines of docs for no effect)
2 - explicitly state they would match style:form, explode:true, (thus share the same object swallowing property)

@karenetheridge
Copy link
Contributor

According to the Swagger docs It would also affect pipeDelimited and spaceDelimited with explode:true which has the effect of looking like style:form, explode:true.

We don't allow explode:true with those styles -- at least, that's been my interpretation of the spec, since examples for those aren't shown. If this is ambiguous we should definitely make that clear (and we can also change the schema to explicitly disallow that combination -- I'm a big fan of everything being in the schema that's possible to represent there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec param serialization Issues related to parameter and/or header serialization
Projects
None yet
Development

No branches or pull requests

3 participants