Fixing support for matrix parameters when not present in the path definition

[ilgrosso]

Description

While already part of the OpanApi Spec 3.0, Matrix parameters are currently ignored by swagger-js and, by consequence, by swagger-ui.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• No code changes (changes to documentation, CI, metadata, etc)
• Dependency changes (any modification to dependencies in package.json)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[shockey]

Thanks for the PR @ilgrosso! In order to get this merged, some reworking needs to happen.

The exports in parameter-builders.js correspond to valid parameter in values defined by the OpenAPI specification (see their usage here, here, and here). Since matrix is a style value, it's actually handled in the OAS3 Style Serializer (specifically here, here, here, and here).

Beyond that, your PR needs to include new or modified tests for your changes. There are already some style: matrix tests for path parameters (the only type of parameter that uses matrix) here, so modifying those would be a good starting point.

Lastly, you'll also need to modify your code style to be compliant with our linter, as you'll notice in the failing CI report from your first commit. You can run npm run lint to get a linter report locally as you make your changes, or install an ESLint integration into your code editor.

Thanks again!

[ilgrosso]

Thanks for your indications @shockey and clarifications about the matrix parameter support in OAS3 - the current work was produced under bad supposition, it seems.
I'll come soon with an updated PR, complying with what you say above.

[shockey]

@ilgrosso no worries! there’s a lot of nuance in the code, it’s not hard to miss 😄

feel free to ping me here if you need any help.

[ilgrosso]

Please take a look @shockey - now npm run lint also runs fine.

[ilgrosso]

Ok, now it should be working, please @shockey have a look...

[ilgrosso]

@shockey as you can see from Travis CI, the checks on Node.js 6.9 went green but the ones on Node.js 4.7 failed because the distribution could not be downloaded

[shockey]

@ilgrosso thanks for the heads up, I just requeued the 4.7 one; looks like nodes.org was having some trouble yesterday.

[ilgrosso]

thx @shockey - now all is green it seems :-)

[shockey]

Thanks @ilgrosso! One last request - can you add some tests? See my review above for some pointers 😄

[ilgrosso]

@shockey test added, sorry for overlooking this.
Thanks for your review.

[ilgrosso]

Hi @shockey, what's missing to accept this PR?

[shockey]

Hi @ilgrosso!

The test cases you added illuminated what you meant in your PR title: It's my fault for not digging into that earlier in the process.

From the 3.0.0 Definitions section:

Path templating refers to the usage of curly braces ({}) to mark a section of a URL path as replaceable using path parameters.

and from the Parameter Object section:

path - Used together with Path Templating, where the parameter value is actually part of the operation's URL. This does not include the host or base path of the API. For example, in /items/{itemId}, the path parameter is itemId.

I reached out to an OpenAPI TSC (the committee that writes the OpenAPI Specification) member, and they confirmed for me that path parameters must be referenced in the path key name in order to be used - path parameter values should not be concatenated to request URLs if the parameter is not referenced in the path itself. As a result, the changes you want to make would go against the specification.

Again, my apologies for not addressing this earlier in the PR process! All of that being said, I'd be happy to accept any improvements to how we generate matrix path parameters when the parameter is being used in the path name.

[ilgrosso]

I see: then it seems there is a bigger problem as, being constrained to be just a "style" of path parameters, matrix parameters do not have yet an adequate definition in OpenAPI v3.

As far as I can tell, in fact, matrix parameters are used by JAX-RS as an optional part of the URL (similarly to query parameters), while path parameters are a mandatory part of the URL.

When I saw OAI/OpenAPI-Specification#69 closed, I though that finally the vexata quaestio was solved, and that people writing REST in Java could finally use freely matrix parameters; unfortunately, it seems we are not yet to that point.

Thanks for clarification, @shockey and sorry for the rant. I am closing this PR now.

[shockey]

@ilgrosso, I see where you're coming from. I don't really see a way to get around this, since as you mentioned (a) path parameters must be required and (b) required parameters can't provide a default value. The only way to do this, then, would be to always manually provide an empty value.

As I'm sure you know, there's not much we can do about that here. Consider raising this issue over at the specification repo - since matrix is supported now, I'd imagine this usability issue would get some attention.