tonic: Refactor handling of comma-separated lists #63
Conversation
loopfz
force-pushed
the
refactor-comma-lists
branch
from
c0711b3 to
05c0bf5
Compare
loopfz
force-pushed
the
refactor-comma-lists
branch
from
05c0bf5 to
7b6627b
Compare
| @@ -29,7 +29,7 @@ const ( | |||
| RequiredTag = "required" | |||
| DefaultTag = "default" | |||
| ValidationTag = "validate" | |||
| ExplodeTag = "explode" | |||
There was a problem hiding this comment.
Why do you make a change on the tag value ? I think we can keep explode, which is the term used in the OpenAPI spec, and it is widely used for this concept.
See https://swagger.io/docs/specification/serialization/, Path parameters section.
There was a problem hiding this comment.
This is open for debate, I just felt the explode semantics are really unclear.
It was extremely confusing again getting back into it for this PR even though I was familiar with the various permutations, so I cant imagine what it must be like fresh off.
|
|
||
| var params []string | ||
|
|
||
| if !c.GetBool(ArrayTag) { |
There was a problem hiding this comment.
OpenAPI 3 spec defines the path parameters serialization with explode being false by default. Do we want to follow that, and make comma-splitting an opt-in ?
There was a problem hiding this comment.
OpenAPI3 defines explode=false by default specifically for arrays.
Here, we assume a scalar.
I feel like an opt-in split mechanism, clearly defining the format as a comma-separated list, is clearer for both query & path handling.
| if explodeVal, ok := ft.Tag.Lookup(ExplodeTag); ok { | ||
| if explode, err := strconv.ParseBool(explodeVal); err == nil && !explode { | ||
| c.Set(ExplodeTag, false) | ||
| c.Set(ArrayTag, false) |
There was a problem hiding this comment.
The OpenAPI 3 spec defines the default serialization to be true by default for explode.
The default serialization method is style: form and explode: true.
Should we follow that ?
There was a problem hiding this comment.
That's actually the case here. The whole explode true/false semantics are insanely confusing.
But having the comma-separated list split logic off by default ends up meaning the same thing as explode=true by default.
|
I think we have to review the following, because it uses the current Line 211 in 66ad463 The code seems also useless. I don't think we need to take the ExplodeTag into account for default values. We could simply state that multiple values can be defined in a default tag, by delimiting them with a comma, and split that to feed the fieldValues slice. Later on, if the actual type of the field is not array or slice, we'll catch it and return an error. Line 236 in 66ad463 That will allow a user to specify multiple default values for arrays and slices, but we can't do much without adding complexity to prevent a misuse. |
|
Not closing this right now, but will need to reassess |
No description provided.