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

Multi-valued parameters in path or query are not checked against collectionFormat #63

Open
fredbi opened this issue Jan 12, 2018 · 18 comments

Comments

@fredbi
Copy link
Member

fredbi commented Jan 12, 2018

According to spec, a multi-valued parameter in path or query (simple parameter) MUST be accompanied by the appropriate collectionFormation definition.

This check is currently not carried on by validate [results in wrong go-swagger code generation]

@gregmarr
Copy link

Are you saying that validate isn't checking that it's there, or isn't checking that the type matches what comes in?

collectionFormat is not marked as "Required" and it has "Default value is csv."

@fredbi
Copy link
Member Author

fredbi commented Jan 12, 2018

Hello greg.
I am saying that a query param may be of type array and pass the validation without collectionFormat.
I am saying too that this setup produces invalid generated code in go-swagger.

The specification is ambiguous because it says two things:

  • for query or path parameters of type array, there MUST be a collectionFormat
  • and when looking at collectionFormat, they say the default is csv

Whatever, the spec package does not fill in the blanks with a "csv" value in this situation.

I also stumbled on another inconsistency: you may define simple params as arrays, and even arrays of arrays of arrays... It's legal for simple schema objects.
Problem here is that there is not corresponding collectionFormat for such nested structures.
This situation should also issue at least a warning.

@gregmarr
Copy link

"for query or path parameters of type array, there MUST be a collectionFormat"

Where are you seeing this? I can't find it.

@fredbi
Copy link
Member Author

fredbi commented Jan 12, 2018

You are right, I did NOT get it from the official spec on 2.0.
Got it from here, since I thought it would bring some clarification... Actually it didn't.
https://swagger.io/docs/specification/2-0/describing-parameters/#query-parameters

@fredbi
Copy link
Member Author

fredbi commented Jan 12, 2018

So if it is advisory only, I might add a warning some day in validate.
But right now I am in the template generating code for this part.
I'll take csv as default value if not defined and needed.

@fredbi
Copy link
Member Author

fredbi commented Jan 12, 2018

Sorry for misleading previous post: I believe you're right - it is NOT stated as mandatory in official 2.0 spec

@gregmarr
Copy link

I can see how "must be defined with type: array and the appropriate collectionFormat." can be confusing. It sounds like it is required. You need to realize that collectionFormat has a default, and thus not providing collectionFormat is appropriate for csv type.

@fredbi
Copy link
Member Author

fredbi commented Jan 12, 2018

How about examples and default values for such a param?
They should follow the collectionFormat syntax or be like regular arrays?
The former case is not supported by validate, the latter is.

@casualjim
Copy link
Member

it's already unpacked at runtime validation time

@fredbi
Copy link
Member Author

fredbi commented Jan 13, 2018

I understand that for runtime validation.
I was referring to spec validation of default and examples values.

Should the example value be expected in the collectionFormat form?

@casualjim
Copy link
Member

It should still follow the same rules. So when collectionFormat is not present it should use the default strategy which is csv

@fredbi
Copy link
Member Author

fredbi commented Jan 13, 2018

ok. This does not work like this currently. Same for default values.
Right now for simple array parameters (or headers), values are expected as provided as a slice, not a string to parse.
One more fix for one more corner case... :)

Thanks

@casualjim
Copy link
Member

sorry yes the collecitonformat is only a parsing hint. The example value in the spec is the collection not the string to which the collectionformat is applied

@fredbi
Copy link
Member Author

fredbi commented Jan 15, 2018

Hello @casualjim
If I follow this advice and try to generate code from go-swagger/go-swagger/fixtures/codegen/swagger-codegen-tests.json,
then we have a problem in generation with default value defined here.

Template server/parameter.gotmpl obvisously takes the value litterally to parse it.

Do you think I should attempt to fix the template and stick to the idea of defining default values as the types tells, no as the parsing hint tells?

@gregmarr
Copy link

The Swagger 2 spec says

Unlike JSON Schema this value MUST conform to the defined type for this parameter.

@gregmarr
Copy link

That tells me we need to fix the test, as its default doesn't match the type.

@fredbi
Copy link
Member Author

fredbi commented Jan 15, 2018

Done.
This is the generation that bails...
I started with a simple fix, but simple non-regression (trying to compile all available fixtures) is getting me much farther than I initially thought.

@fredbi
Copy link
Member Author

fredbi commented Jan 15, 2018

Template simply does NOT fit for default value in array params (not to say, nested arrays).

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

3 participants