Repository: getkin/kin-openapi
Pull request #705 (2022-12-14 07:22:18)

Fix inconsistent processing of server variables in gorillamux router
I don't really know how to explain this issue. If an OpenAPI document has a server set to "/" then gorillamux router CANNOT find a route for a request. If an OpenAPI document has a server set to a single variable and that variable's default value is "/" then the gorillamux router CAN find a route for a request.  It seems to me that these two server values should be the same when it comes to finding routes.

This PR contains a new unit test with two test cases, one for the server set to "/", the other for the server set to a single variable and that variable's default value is "/" 

To make this new unit test as concise as possible I refactored router_test.go to extract the inline `expect` function into a standalone function so it could be shared between the existing `TestRouter` function and my new `TestRouter_Servers` function.

Can you help me understand why these two test cases do not work the same?


Patch:
diff --git a/openapi3filter/req_resp_decoder.go b/openapi3filter/req_resp_decoder.go
index 52c65af..870651c 100644
--- a/openapi3filter/req_resp_decoder.go
+++ b/openapi3filter/req_resp_decoder.go
@@ -895,7 +895,14 @@ func parsePrimitive(raw string, schema *openapi3.SchemaRef) (interface{}, error)
 	}
 	switch schema.Value.Type {
 	case "integer":
-		v, err := strconv.ParseFloat(raw, 64)
+		if schema.Value.Format == "int32" {
+			v, err := strconv.ParseInt(raw, 0, 32)
+			if err != nil {
+				return nil, &ParseError{Kind: KindInvalidFormat, Value: raw, Reason: "an invalid " + schema.Value.Type, Cause: err.(*strconv.NumError).Err}
+			}
+			return int32(v), nil
+		}
+		v, err := strconv.ParseInt(raw, 0, 64)
 		if err != nil {
 			return nil, &ParseError{Kind: KindInvalidFormat, Value: raw, Reason: "an invalid " + schema.Value.Type, Cause: err.(*strconv.NumError).Err}
 		}
diff --git a/routers/gorillamux/router.go b/routers/gorillamux/router.go
index 811ba7d..6977808 100644
--- a/routers/gorillamux/router.go
+++ b/routers/gorillamux/router.go
@@ -57,8 +57,6 @@ func NewRouter(doc *openapi3.T) (routers.Router, error) {
 	muxRouter := mux.NewRouter().UseEncodedPath()
 	r := &Router{}
 	for _, path := range orderedPaths(doc.Paths) {
-		servers := servers
-
 		pathItem := doc.Paths[path]
 		if len(pathItem.Servers) > 0 {
 			if servers, err = makeServers(pathItem.Servers); err != nil {
@@ -140,19 +138,13 @@ func makeServers(in openapi3.Servers) ([]srv, error) {
 			if lhs := strings.TrimSuffix(serverURL, server.Variables[sVar].Default); lhs != "" {
 				varsUpdater = func(vars map[string]string) { vars[sVar] = lhs }
 			}
-			servers = append(servers, srv{
-				base:        server.Variables[sVar].Default,
-				server:      server,
-				varsUpdater: varsUpdater,
-			})
-			continue
-		}
+			svr, err := newSrv(serverURL, server, varsUpdater)
+			if err != nil {
+				return nil, err
+			}
 
-		var schemes []string
-		if strings.Contains(serverURL, "://") {
-			scheme0 := strings.Split(serverURL, "://")[0]
-			schemes = permutePart(scheme0, server)
-			serverURL = strings.Replace(serverURL, scheme0+"://", schemes[0]+"://", 1)
+			servers = append(servers, svr)
+			continue
 		}
 
 		// If a variable represents the port "http://domain.tld:{port}/bla"
@@ -172,21 +164,11 @@ func makeServers(in openapi3.Servers) ([]srv, error) {
 			}
 		}
 
-		u, err := url.Parse(bEncode(serverURL))
+		svr, err := newSrv(serverURL, server, varsUpdater)
 		if err != nil {
 			return nil, err
 		}
-		path := bDecode(u.EscapedPath())
-		if len(path) > 0 && path[len(path)-1] == '/' {
-			path = path[:len(path)-1]
-		}
-		servers = append(servers, srv{
-			host:        bDecode(u.Host), //u.Hostname()?
-			base:        path,
-			schemes:     schemes, // scheme: []string{scheme0}, TODO: https://github.com/gorilla/mux/issues/624
-			server:      server,
-			varsUpdater: varsUpdater,
-		})
+		servers = append(servers, svr)
 	}
 	if len(servers) == 0 {
 		servers = append(servers, srv{})
@@ -195,6 +177,32 @@ func makeServers(in openapi3.Servers) ([]srv, error) {
 	return servers, nil
 }
 
+func newSrv(serverURL string, server *openapi3.Server, varsUpdater varsf) (srv, error) {
+	var schemes []string
+	if strings.Contains(serverURL, "://") {
+		scheme0 := strings.Split(serverURL, "://")[0]
+		schemes = permutePart(scheme0, server)
+		serverURL = strings.Replace(serverURL, scheme0+"://", schemes[0]+"://", 1)
+	}
+
+	u, err := url.Parse(bEncode(serverURL))
+	if err != nil {
+		return srv{}, err
+	}
+	path := bDecode(u.EscapedPath())
+	if len(path) > 0 && path[len(path)-1] == '/' {
+		path = path[:len(path)-1]
+	}
+	svr := srv{
+		host:        bDecode(u.Host), //u.Hostname()?
+		base:        path,
+		schemes:     schemes, // scheme: []string{scheme0}, TODO: https://github.com/gorilla/mux/issues/624
+		server:      server,
+		varsUpdater: varsUpdater,
+	}
+	return svr, nil
+}
+
 func orderedPaths(paths map[string]*openapi3.PathItem) []string {
 	// https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#pathsObject
 	// When matching URLs, concrete (non-templated) paths would be matched


Test patch:
diff --git a/openapi3filter/issue625_test.go b/openapi3filter/issue625_test.go
index d9e5bae..5642a7e 100644
--- a/openapi3filter/issue625_test.go
+++ b/openapi3filter/issue625_test.go
@@ -72,7 +72,7 @@ paths:
 			name:   "failed allof object array",
 			spec:   allOfArraySpec,
 			req:    `/items?test=1.2,3.1`,
-			errStr: `parameter "test" in query has an error: Error at "/0": value "1.2" must be an integer`,
+			errStr: `parameter "test" in query has an error: path 0: value 1.2: an invalid integer: invalid syntax`,
 		},
 		{
 			name: "success oneof object array",
diff --git a/openapi3filter/req_resp_decoder_test.go b/openapi3filter/req_resp_decoder_test.go
index 8bd62b1..709cdc9 100644
--- a/openapi3filter/req_resp_decoder_test.go
+++ b/openapi3filter/req_resp_decoder_test.go
@@ -175,7 +175,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:  "integer",
 					param: &openapi3.Parameter{Name: "param", In: "path", Schema: integerSchema},
 					path:  "/1",
-					want:  float64(1),
+					want:  int64(1),
 					found: true,
 				},
 				{
@@ -456,7 +456,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:  "integer",
 					param: &openapi3.Parameter{Name: "param", In: "query", Schema: integerSchema},
 					query: "param=1",
-					want:  float64(1),
+					want:  int64(1),
 					found: true,
 				},
 				{
@@ -522,7 +522,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:  "anyofSchema integer",
 					param: &openapi3.Parameter{Name: "param", In: "query", Schema: anyofSchema},
 					query: "param=1",
-					want:  float64(1),
+					want:  int64(1),
 					found: true,
 				},
 				{
@@ -548,7 +548,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:  "oneofSchema int",
 					param: &openapi3.Parameter{Name: "param", In: "query", Schema: oneofSchema},
 					query: "param=1122",
-					want:  float64(1122),
+					want:  int64(1122),
 					found: true,
 				},
 				{
@@ -724,7 +724,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:   "integer",
 					param:  &openapi3.Parameter{Name: "X-Param", In: "header", Schema: integerSchema},
 					header: "X-Param:1",
-					want:   float64(1),
+					want:   int64(1),
 					found:  true,
 				},
 				{
@@ -835,6 +835,13 @@ func TestDecodeParameter(t *testing.T) {
 					want:   map[string]interface{}{"id": "foo", "name": "bar"},
 					found:  true,
 				},
+				{
+					name:   "valid integer prop",
+					param:  &openapi3.Parameter{Name: "X-Param", In: "header", Schema: integerSchema},
+					header: "X-Param:88",
+					found:  true,
+					want:   int64(88),
+				},
 				{
 					name:   "invalid integer prop",
 					param:  &openapi3.Parameter{Name: "X-Param", In: "header", Schema: objectOf("foo", integerSchema)},
@@ -893,7 +900,7 @@ func TestDecodeParameter(t *testing.T) {
 					name:   "integer",
 					param:  &openapi3.Parameter{Name: "X-Param", In: "cookie", Schema: integerSchema},
 					cookie: "X-Param:1",
-					want:   float64(1),
+					want:   int64(1),
 					found:  true,
 				},
 				{
@@ -1180,7 +1187,7 @@ func TestDecodeBody(t *testing.T) {
 				WithProperty("a", openapi3.NewStringSchema()).
 				WithProperty("b", openapi3.NewIntegerSchema()).
 				WithProperty("c", openapi3.NewArraySchema().WithItems(openapi3.NewStringSchema())),
-			want: map[string]interface{}{"a": "a1", "b": float64(10), "c": []interface{}{"c1", "c2"}},
+			want: map[string]interface{}{"a": "a1", "b": int64(10), "c": []interface{}{"c1", "c2"}},
 		},
 		{
 			name: "urlencoded space delimited",
@@ -1193,7 +1200,7 @@ func TestDecodeBody(t *testing.T) {
 			encoding: map[string]*openapi3.Encoding{
 				"c": {Style: openapi3.SerializationSpaceDelimited, Explode: boolPtr(false)},
 			},
-			want: map[string]interface{}{"a": "a1", "b": float64(10), "c": []interface{}{"c1", "c2"}},
+			want: map[string]interface{}{"a": "a1", "b": int64(10), "c": []interface{}{"c1", "c2"}},
 		},
 		{
 			name: "urlencoded pipe delimited",
@@ -1206,7 +1213,7 @@ func TestDecodeBody(t *testing.T) {
 			encoding: map[string]*openapi3.Encoding{
 				"c": {Style: openapi3.SerializationPipeDelimited, Explode: boolPtr(false)},
 			},
-			want: map[string]interface{}{"a": "a1", "b": float64(10), "c": []interface{}{"c1", "c2"}},
+			want: map[string]interface{}{"a": "a1", "b": int64(10), "c": []interface{}{"c1", "c2"}},
 		},
 		{
 			name: "multipart",
diff --git a/routers/gorillamux/router_test.go b/routers/gorillamux/router_test.go
index 104056e..3e74400 100644
--- a/routers/gorillamux/router_test.go
+++ b/routers/gorillamux/router_test.go
@@ -6,6 +6,7 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/getkin/kin-openapi/openapi3"
@@ -249,7 +250,16 @@ func TestServerPath(t *testing.T) {
 			"http://example.com:{port}/path",
 			map[string]string{
 				"port": "8088",
-			})},
+			}),
+		newServerWithVariables(
+			"{server}",
+			map[string]string{
+				"server": "/",
+			}),
+		newServerWithVariables(
+			"/",
+			nil,
+		)},
 	})
 	require.NoError(t, err)
 }
@@ -325,6 +335,157 @@ func TestRelativeURL(t *testing.T) {
 	require.Equal(t, "/hello", route.Path)
 }
 
+func Test_makeServers(t *testing.T) {
+	type testStruct struct {
+		name    string
+		servers openapi3.Servers
+		want    []srv
+		wantErr bool
+		initFn  func(tt *testStruct)
+	}
+	tests := []testStruct{
+		{
+			name: "server is root path",
+			servers: openapi3.Servers{
+				newServerWithVariables("/", nil),
+			},
+			want: []srv{{
+				schemes:     nil,
+				host:        "",
+				base:        "",
+				server:      nil,
+				varsUpdater: nil,
+			}},
+			wantErr: false,
+			initFn: func(tt *testStruct) {
+				for i, server := range tt.servers {
+					tt.want[i].server = server
+				}
+			},
+		},
+		{
+			name: "server with single variable that evaluates to root path",
+			servers: openapi3.Servers{
+				newServerWithVariables("{server}", map[string]string{"server": "/"}),
+			},
+			want: []srv{{
+				schemes:     nil,
+				host:        "",
+				base:        "",
+				server:      nil,
+				varsUpdater: nil,
+			}},
+			wantErr: false,
+			initFn: func(tt *testStruct) {
+				for i, server := range tt.servers {
+					tt.want[i].server = server
+				}
+			},
+		},
+		{
+			name: "server is http://localhost:28002",
+			servers: openapi3.Servers{
+				newServerWithVariables("http://localhost:28002", nil),
+			},
+			want: []srv{{
+				schemes:     []string{"http"},
+				host:        "localhost:28002",
+				base:        "",
+				server:      nil,
+				varsUpdater: nil,
+			}},
+			wantErr: false,
+			initFn: func(tt *testStruct) {
+				for i, server := range tt.servers {
+					tt.want[i].server = server
+				}
+			},
+		},
+		{
+			name: "server with single variable that evaluates to http://localhost:28002",
+			servers: openapi3.Servers{
+				newServerWithVariables("{server}", map[string]string{"server": "http://localhost:28002"}),
+			},
+			want: []srv{{
+				schemes:     []string{"http"},
+				host:        "localhost:28002",
+				base:        "",
+				server:      nil,
+				varsUpdater: nil,
+			}},
+			wantErr: false,
+			initFn: func(tt *testStruct) {
+				for i, server := range tt.servers {
+					tt.want[i].server = server
+				}
+			},
+		},
+		{
+			name: "server with multiple variables that evaluates to http://localhost:28002",
+			servers: openapi3.Servers{
+				newServerWithVariables("{scheme}://{host}:{port}", map[string]string{"scheme": "http", "host": "localhost", "port": "28002"}),
+			},
+			want: []srv{{
+				schemes:     []string{"http"},
+				host:        "{host}:28002",
+				base:        "",
+				server:      nil,
+				varsUpdater: func(vars map[string]string) { vars["port"] = "28002" },
+			}},
+			wantErr: false,
+			initFn: func(tt *testStruct) {
+				for i, server := range tt.servers {
+					tt.want[i].server = server
+				}
+			},
+		},
+		{
+			name: "server with unparsable URL fails",
+			servers: openapi3.Servers{
+				newServerWithVariables("exam^ple.com:443", nil),
+			},
+			want:    nil,
+			wantErr: true,
+			initFn:  nil,
+		},
+		{
+			name: "server with single variable that evaluates to unparsable URL fails",
+			servers: openapi3.Servers{
+				newServerWithVariables("{server}", map[string]string{"server": "exam^ple.com:443"}),
+			},
+			want:    nil,
+			wantErr: true,
+			initFn:  nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.initFn != nil {
+				tt.initFn(&tt)
+			}
+			got, err := makeServers(tt.servers)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("makeServers() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			assert.Equal(t, len(tt.want), len(got), "expected and actual servers lengths are not equal")
+			for i := 0; i < len(tt.want); i++ {
+				// Unfortunately using assert.Equals or reflect.DeepEquals isn't
+				// an option because function pointers cannot be compared
+				assert.Equal(t, tt.want[i].schemes, got[i].schemes)
+				assert.Equal(t, tt.want[i].host, got[i].host)
+				assert.Equal(t, tt.want[i].host, got[i].host)
+				assert.Equal(t, tt.want[i].server, got[i].server)
+				if tt.want[i].varsUpdater == nil {
+					assert.Nil(t, got[i].varsUpdater, "expected and actual varsUpdater should point to same function")
+				} else {
+					assert.NotNil(t, got[i].varsUpdater, "expected and actual varsUpdater should point to same function")
+				}
+			}
+		})
+	}
+}
+
 func newServerWithVariables(url string, variables map[string]string) *openapi3.Server {
 	var serverVariables = map[string]*openapi3.ServerVariable{}