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.

Sign up for GitHub

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

tonic: Refactor handling of comma-separated lists #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tonic: Refactor handling of comma-separated lists #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
language: go

go:
- "1.8.x"
- "1.9.x"
- "1.10.x"
- "1.11.x"
- "1.12.x"
- "master"

jobs:
Expand Down
10 changes: 5 additions & 5 deletions tonic/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,10 @@ func bind(c *gin.Context, v reflect.Value, tag string, extract extractor) error
}
// Set-up context for extractors.
// Query.
c.Set(ExplodeTag, true) // default
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if arrayVal, ok := ft.Tag.Lookup(ArrayTag); ok {
if array, err := strconv.ParseBool(arrayVal); err == nil && array {
c.Set(ArrayTag, true)
}
}
_, fieldValues, err := extract(c, tagValue)
Expand All @@ -209,7 +209,7 @@ func bind(c *gin.Context, v reflect.Value, tag string, extract extractor) error
// if no values were returned.
def, ok := ft.Tag.Lookup(DefaultTag)
if ok && len(fieldValues) == 0 {
if c.GetBool(ExplodeTag) {
if c.GetBool(ArrayTag) {
fieldValues = append(fieldValues, strings.Split(def, ",")...)
} else {
fieldValues = append(fieldValues, def)
Expand Down
22 changes: 16 additions & 6 deletions tonic/tonic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const (
RequiredTag = "required"
DefaultTag = "default"
ValidationTag = "validate"
ExplodeTag = "explode"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ArrayTag = "commalist"
)

const (
Expand Down Expand Up @@ -264,7 +264,7 @@ func extractQuery(c *gin.Context, tag string) (string, []string, error) {
var params []string
query := c.Request.URL.Query()[name]

if c.GetBool(ExplodeTag) {
if !c.GetBool(ArrayTag) {
// Delete empty elements so default and required arguments
// will play nice together. Append to a new collection to
// preserve order without too much copying.
Expand Down Expand Up @@ -303,18 +303,28 @@ func extractPath(c *gin.Context, tag string) (string, []string, error) {
if err != nil {
return "", nil, err
}
p := c.Param(name)

var params []string

if !c.GetBool(ArrayTag) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Owner Author

@loopfz loopfz Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

params = []string{c.Param(name)}
} else {
splitFn := func(c rune) bool {
return c == ','
}
params = strings.FieldsFunc(c.Param(name), splitFn)
}

// XXX: deprecated, use of "default" tag is preferred
if p == "" && defaultVal != "" {
if len(params) == 0 && defaultVal != "" {
return name, []string{defaultVal}, nil
}
// XXX: deprecated, use of "validate" tag is preferred
if p == "" && required {
if len(params) == 0 && required {
return "", nil, fmt.Errorf("missing path parameter: %s", name)
}

return name, []string{p}, nil
return name, params, nil
}

// extractHeader is an extractor that operates on the headers
Expand Down
48 changes: 28 additions & 20 deletions tonic/tonic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func TestMain(m *testing.M) {
g.GET("/scalar", tonic.Handler(scalarHandler, 200))
g.GET("/error", tonic.Handler(errorHandler, 200))
g.GET("/path/:param", tonic.Handler(pathHandler, 200))
g.GET("/path-list/:param-path-list", tonic.Handler(pathListHandler, 200))
g.GET("/query", tonic.Handler(queryHandler, 200))
g.GET("/query-old", tonic.Handler(queryHandlerOld, 200))
g.POST("/body", tonic.Handler(bodyHandler, 200))
Expand Down Expand Up @@ -87,13 +88,13 @@ func TestPathQuery(t *testing.T) {

tester.AddCall("query-complex", "GET", fmt.Sprintf("/query?param=foo&param-complex=%s", now), "").Checkers(iffy.ExpectStatus(200), expectString("param-complex", string(now)))

// Explode.
tester.AddCall("query-explode", "GET", "/query?param=foo&param-explode=a&param-explode=b&param-explode=c", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-explode", "a", "b", "c"))
tester.AddCall("query-explode-disabled-ok", "GET", "/query?param=foo&param-explode-disabled=x,y,z", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-explode-disabled", "x", "y", "z"))
tester.AddCall("query-explode-disabled-error", "GET", "/query?param=foo&param-explode-disabled=a&param-explode-disabled=b", "").Checkers(iffy.ExpectStatus(400))
tester.AddCall("query-explode-string", "GET", "/query?param=foo&param-explode-string=x,y,z", "").Checkers(iffy.ExpectStatus(200), expectString("param-explode-string", "x,y,z"))
tester.AddCall("query-explode-default", "GET", "/query?param=foo", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-explode-default", "1", "2", "3")) // default with explode
tester.AddCall("query-explode-disabled-default", "GET", "/query?param=foo", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-explode-disabled-default", "1,2,3")) // default without explode
// Array split
tester.AddCall("query-list-nosplit", "GET", "/query?param=foo&param-list-nosplit=a&param-list-nosplit=b&param-list-nosplit=c", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-list-nosplit", "a", "b", "c"))
tester.AddCall("query-list-split", "GET", "/query?param=foo&param-list-split=x,y,z", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-list-split", "x", "y", "z"))
tester.AddCall("query-list-split-repeated", "GET", "/query?param=foo&param-list-split=a&param-list-split=b", "").Checkers(iffy.ExpectStatus(400))
tester.AddCall("query-list-nosplit-single", "GET", "/query?param=foo&param-list-string-nosplit=x,y,z", "").Checkers(iffy.ExpectStatus(200), expectString("param-list-string-nosplit", "x,y,z"))
tester.AddCall("query-list-default", "GET", "/query?param=foo", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-list-default", "1", "2", "3")) // default with explode
tester.AddCall("path-list", "GET", "/path-list/1,2,3", "").Checkers(iffy.ExpectStatus(200), expectStringArr("param-path-list", "1", "2", "3"))

tester.Run()
}
Expand Down Expand Up @@ -150,20 +151,27 @@ func pathHandler(c *gin.Context, in *pathIn) (*pathIn, error) {
return in, nil
}

type pathListIn struct {
ParamPathList []string `path:"param-path-list" json:"param-path-list" commalist:"true"`
}

func pathListHandler(c *gin.Context, in *pathListIn) (*pathListIn, error) {
return in, nil
}

type queryIn struct {
Param string `query:"param" json:"param" validate:"required"`
ParamOptional string `query:"param-optional" json:"param-optional"`
Params []string `query:"params" json:"params"`
ParamInt int `query:"param-int" json:"param-int"`
ParamBool bool `query:"param-bool" json:"param-bool"`
ParamDefault string `query:"param-default" json:"param-default" default:"default" validate:"required"`
ParamPtr *string `query:"param-ptr" json:"param-ptr"`
ParamComplex time.Time `query:"param-complex" json:"param-complex"`
ParamExplode []string `query:"param-explode" json:"param-explode" explode:"true"`
ParamExplodeDisabled []string `query:"param-explode-disabled" json:"param-explode-disabled" explode:"false"`
ParamExplodeString string `query:"param-explode-string" json:"param-explode-string" explode:"true"`
ParamExplodeDefault []string `query:"param-explode-default" json:"param-explode-default" default:"1,2,3" explode:"true"`
ParamExplodeDefaultDisabled []string `query:"param-explode-disabled-default" json:"param-explode-disabled-default" default:"1,2,3" explode:"false"`
Param string `query:"param" json:"param" validate:"required"`
ParamOptional string `query:"param-optional" json:"param-optional"`
Params []string `query:"params" json:"params"`
ParamInt int `query:"param-int" json:"param-int"`
ParamBool bool `query:"param-bool" json:"param-bool"`
ParamDefault string `query:"param-default" json:"param-default" default:"default" validate:"required"`
ParamPtr *string `query:"param-ptr" json:"param-ptr"`
ParamComplex time.Time `query:"param-complex" json:"param-complex"`
ParamListNoSplit []string `query:"param-list-nosplit" json:"param-list-nosplit"`
ParamListSplit []string `query:"param-list-split" json:"param-list-split" commalist:"true"`
ParamListStringNoSplit string `query:"param-list-string-nosplit" json:"param-list-string-nosplit"`
ParamListDefault []string `query:"param-list-default" json:"param-list-default" default:"1,2,3" commalist:"true"`
*DoubleEmbedded
}

Expand Down