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

Support for RFC6570 #541

Closed

Conversation

@dilipkrish
Copy link
Contributor

commented Aug 4, 2015

This is an attempt to provide a backwards compatible solution to support RFC 6570.

This still has a few kinks that might need to be ironed out, because its not clear to me the choices behind the following features that I've conveniently skipped. Soliciting feedback/discussion on the topics below

  • support for collectionFormat=brackets
  • support for query parameters with hyphens in them e.g. a-0

I've used a level 4 compatible library to help with the RFC 6570 support.

This relates to: OAI/OpenAPI-Specification#291

The motivation for this PR is to allow finder apis with the same resource path. For e.g. If we have a customer resource, there are cases where the query string can determine different unique operations based on mutually exclusive query parameters. For e.g. if we have a customers resource @ http://example.org/customers, one could imagine search operations like the following.

search example swagger path
by Name http://example.org/customers?fname=blah&lname=blah http://example.org/customers{?fname,lname}
by email http://example.org/customers?email=a@b.com http://example.org/customers{?email}

The way this feature is implemented it augments the path and query parameter resolution in a backward compatible fashion, So if an operation has an RFC 6570 complaint path, it will have its path/query parameters substituted.


if (Array.isArray(value)) {

This comment has been minimized.

Copy link
@fehguy

fehguy Aug 4, 2015

Contributor

I don't think it's safe to get rid of this code. When interacting directly with the javascript client, you can pass arrays directly instead of encoding them into strings

This comment has been minimized.

Copy link
@dilipkrish

dilipkrish Aug 4, 2015

Author Contributor

i think the diffs are not properly aligned. I replaced the hand rolled urlify with a delegation to a urltemplate aware substitution library. So I suspect that the library will take care of the substitutions?

@webron

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

Thanks for the PR, @dilipkrish, but I don't think we can merge it. The issue you refer to in swagger-spec is a feature request for the next version which we haven't even decided whether to support or not. I don't think we should add support for such suggestions in the official tool set until such a decision is taken and we actually start working on the next version.

We can keep the PR open until then, though the code my deviate.

@dilipkrish

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

@webron I'm a little 😕

Just to be clear this PR is only to support paths like http://example.org/{city}/customers{?email}.

Currently swagger-spec supports path templating but I could NOT find anything in the spec where RFC6570 paths are specifically precluded. So technically, swagger-js should support urls presented in the above format.

The motivation for this PR is that I've added ability in springfox to optionally support RFC 6570 compliant templates, so as to support findBy scenarios.

So this PR is to support that specific use case where the path is an valid URL template. Now if in the next version of the spec we do specify that the url templates will conform to RFC 6570 then great! we already have it supported in swagger-js!!

Now I do have 2 outstanding questions (as open tasks in the PR), that I'd like to be included as part discussion for the spec amendment.

Also, would you mind pointing me in the right direction, as regards, where/how one would be able to override the collectionFormat for a property to specify tsv, ssv etc.

Of course its up to you guys to figure out if/when you want to pull this in, but IMHO, this is orthogonal to the spec being amended.

@webron

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

I wish you would have asked that on the spec prior to it then. We definitely don't support that notation. Just because the spec doesn't explicitly say it isn't support, doesn't mean it is. We can't cover everything, but I can definitely make the language clearer. The paths used to describe the operation cannot have query parameters in them. This has been discussed in many issues and forum questions, and it was clarified there. If you feel there's room to add a specific clarification in the spec itself, I'd be happy to add it.

I'm afraid we can't start adding support for things like that as that would make it de facto part of the spec, and it would throw off other vendors out there.

@dilipkrish

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

Understood! Would've been nice to subvert the spec with this PR 😜

Seriously tho' I'd prefer to do it the right way. I thought I did bring it up, unfortunately I can't find the thread, but I believe it had to do with supporting multiple paths for finder operations and operations that support different response media types based on accept headers.

Is the swagger spec working group active? All this work is to facilitate hypermedia support that I've been working on. So I wanted to bounce some ideas I had to provide hypermedia support. I know had a brief conversation with @olensmar about it, but what is the right forum for that.

@webron

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

Thanks for understanding, @dilipkrish, I'm sure you can appreciate the challenges in keeping things aligned. Trust me, there are times we wish we could change things on the fly, and we can maybe get away with it with some minor changes, but this is a big one. I can go in length explaining, but this is probably not the place.

The issue of multiple operations per paths/content type negotiation/definition of an isolated path are all related and are (in my opinion) strong candidates to be handled in the next version of the spec. There are several ways to discuss the next spec. Can you send me an email? I'll provide with the alternatives.

Also, if you do feel there's a need for clarification regarding the path templating in the spec, please open an issue about it on swagger-spec and I'll try improving the wording (I'm not sure I can refer to the RFC directly, need to investigate).

@fehguy fehguy added this to the future milestone Aug 7, 2015

@dilipkrish dilipkrish referenced this pull request Sep 2, 2015
@chrisdostert

This comment has been minimized.

Copy link

commented Oct 9, 2015

+1 for RFC6570 support

@fehguy

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

until the spec supports it, we can't add this support.

@fehguy fehguy closed this Nov 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.