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

[develop 2.0] add support for collection format with brackets #191

Closed
fehguy opened this issue Dec 30, 2014 · 16 comments
Closed

[develop 2.0] add support for collection format with brackets #191

fehguy opened this issue Dec 30, 2014 · 16 comments
Milestone

Comments

@fehguy
Copy link
Contributor

fehguy commented Dec 30, 2014

Per swagger-api/swagger-ui#661

need to add support for query params with ?param[]=1&param[]=2

@fehguy
Copy link
Contributor Author

fehguy commented Dec 31, 2014

addressed in 30eb4f3

@whitlockjc
Copy link
Contributor

Why was swagger-js updated to support this but the Swagger 2.0 JSON Schema files were not? Also, there is no mention of this feature in the Swagger 2.0 Specification. Supporting this in swagger-js but not officially means that if you intend to keep valid Swagger documents, per the JSON Schema and Specification Documentation, you either can't use this feature.

Mentioning @fehguy so he sees this.

@webron
Copy link
Contributor

webron commented Apr 14, 2015

I square brackets are in the name of the field and not the value. There's no restriction on that in the spec... Have you looked at the original issue? swagger-api/swagger-ui#661

@whitlockjc
Copy link
Contributor

I'm just the messenger but yes, I did look at the original issue and I even did some code surfing. If you search for brackets in the swagger-js project, you'll find that swagger-js clearly has explicit support for a collectionType of brackets. (Just click this link to see what I see.) In fact, one of the items found will be an explicit unit test for this. So like I said, it seems the fix for this issue added support for a collectionFormat that is not documented in the Swagger 2.0 Specification documentation or the Swagger 2.0 JSON Schema. This was originally discussed at apigee-127/swagger-tools/pull/192.

@ddm
Copy link

ddm commented Apr 14, 2015

I was actually confused because swagger-ui supports "brackets" as a collectionFormat value.

See here for example: 30eb4f3

The problem when using "myparam[]" as a name (and "multi" as a collectionFormat) is that swagger-ui will actually use "myparam%5B%5D" in the query string instead of "myparam[]" if you use "brackets" as a collectionFormat.

@webron
Copy link
Contributor

webron commented Apr 14, 2015

Okay, so to clarify, there's no 'brackets' collectionFormat. Perhaps the implementation is wrong.

@ddm
Copy link

ddm commented Apr 14, 2015

So the right way to do it is to put the brackets in the parameter name?

@webron
Copy link
Contributor

webron commented Apr 14, 2015

That is correct.

@whitlockjc
Copy link
Contributor

Based on the Swagger 2.0 JSON Schema and Specification Documentation, I agree with @webron but I definitely see explicit support for this in code. Now that we know this is an implementation issue, I will let @webron take over. :)

@webron
Copy link
Contributor

webron commented Apr 14, 2015

You want me to open a ticket to remove support for it? :)

@ddm
Copy link

ddm commented Apr 14, 2015

It seems swagger-ui does the right thing when you use "collectionFormat": "brackets"
=> http://myhost/route/?myparam[]=first&myparam[]=second

While when "name": "myparam[]" is used (with the allowed "collectionFormat": "multi") you get
=> http://myhost/route/?myparam%5B%5D=first&myparam%5B%5D=second
...on the current develop_2.0 branch

The later doesn't match the jQuery "traditional" convention:
https://api.jquery.com/jquery.param/

If there is no plan to add "brackets" support to the swagger 2.0 spec it is weird to allow it in swagger-ui.
Do you know how the discrepancy came to be?

@ddm
Copy link

ddm commented Apr 14, 2015

The "encodeQueryParam" method responsible for the excessive URL encoding is simply:

Operation.prototype.encodeQueryParam = function (arg) {
    return encodeURIComponent(arg);
};

The problem is: that method is actually part of https://www.npmjs.com/package/swagger-client

But the corresponding git repo yields a 404: https://github.com/swagger-api/swagger-client

Where is that code hosted now?

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2015

All I believe if you go through the workgroup discussions, this format was supposed to be in the spec. It's a very common array specifier for query params.

@ddm please look at swagger-js

@ddm
Copy link

ddm commented Apr 15, 2015

Well the thing is, the recommended method of using brackets in the name and 'multi' as a collectionFormat falls short because when using ...?param[]=first&param[]=second most web frameworks (express but also most PHP frameworks IIRC) will actually add a 'param' property to the request object, not a 'param[]' property. This means you now have a mismatch between the name of your parameter in the swagger definition and the name in your server code.

That makes the job of swagger-tools or other automated validation generators harder. @whitlockjc can correct me if I'm wrong here.

My understanding was that 'collectionFormat' was an indication of how the collection would be serialised in the queryString/path/body of the request while 'name' was the actual name of the parameter.

So 'multi' means that the parameter name can be repeated and 'brackets' means that [] is appended to the name to indicate a collection (with repetition adding items to the collection like with 'multi').

I still believe that 'brackets' has its place in the spec because it's a serialisation convention that is used in both server-side frameworks (express etc.) and client side ajax libraries (jQuery.ajax traditional and probably others).

@ddm
Copy link

ddm commented Apr 15, 2015

@fehguy Thanks for the pointer BTW, I see that the repo URL is fixed in the develop_2.0 branch so npm will eventually point to the correct repo once it's merged into master and published.

@birbird
Copy link

birbird commented Jul 21, 2015

Did the bug come back?
I have tried master and develop_2.0 branch, just use the dist directly. The bug is still there.

This is my swagger

                    {
                        "name": "circle[member_profile_itmes]",
                        "in": "formData",
                        "type": "array",
                        "items": {
                            "type": "string",
                        },
                        "collectionFormat": "multi"
                    }

And I input multi lines in the textarea, they are just joined with a comma.

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

No branches or pull requests

5 participants