Repository: caicloud/nirvana
Pull request #385 (2020-11-19 10:08:43)

[release-0.3] perf(client): request support query in url
This is an automated cherry-pick of #381

/assign iawia002

caicloud-bot: @caicloud-bot: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our [release note process](https://git.k8s.io/community/contributors/guide/release-notes.md) to remove it.

<details>

Instructions for interacting with me using PR comments are available [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md).  If you have questions or suggestions related to my behavior, please file an issue against the [caicloud/tools](https://github.com/caicloud/tools/issues/new?title=Prow%20issue:) repository.
</details>
caicloud-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: 
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: **iawia002**

Assign the PR to them by writing `/assign @iawia002` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#bot-commands).

The pull request process is described [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#typical-workflow)

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/caicloud/nirvana/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["iawia002"]} -->
iawia002: /release-note-none
/lgtm

Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
new file mode 100644
index 0000000..24c13a8
--- /dev/null
+++ b/.github/workflows/ci.yml
@@ -0,0 +1,40 @@
+name: ci
+
+on: [pull_request]
+
+jobs:
+  ci:
+    runs-on: ${{ matrix.os }}
+    timeout-minutes: 10
+    strategy:
+      matrix:
+        go: ["1.13"]
+        os: [ubuntu-latest, macOS-latest]
+    name: Go ${{ matrix.go }} in ${{ matrix.os }}
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions/setup-go@v2
+        with:
+          go-version: ${{ matrix.go }}
+
+      - name: Environment
+        run: |
+          go version
+          go env
+      - name: Install tools
+        env:
+          GO111MODULE: on
+        run: |
+          go get github.com/golangci/golangci-lint/cmd/golangci-lint@v1.32.2
+          golangci-lint --version
+          go get github.com/mattn/goveralls
+      - name: Lint
+        run: |
+          hack/verify_boilerplate.py
+          golangci-lint run ./... -v
+      - name: Test
+        run: go test -race -covermode atomic -coverprofile=covprofile ./...
+      - name: Send coverage
+        env:
+          COVERALLS_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: goveralls -coverprofile=covprofile -service=github
diff --git a/.travis.yml b/.travis.yml
deleted file mode 100644
index 8a545dd..0000000
--- a/.travis.yml
+++ /dev/null
@@ -1,15 +0,0 @@
-language: go
-
-go:
-  - "1.13.x"
-
-before_install:
-  - go get github.com/mattn/goveralls
-  - curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.23.6
-
-script:
-  - hack/verify_boilerplate.py
-  - golangci-lint run
-  - GO_COVER_PROFILE=$(mktemp -u)
-  - go test ./... -race -covermode atomic -coverprofile $GO_COVER_PROFILE
-  - goveralls -service=travis-ci -coverprofile $GO_COVER_PROFILE
diff --git a/rest/rest.go b/rest/rest.go
index 6fd093f..9be1dc0 100644
--- a/rest/rest.go
+++ b/rest/rest.go
@@ -27,7 +27,7 @@ import (
 	"mime"
 	"mime/multipart"
 	"net/http"
-	"net/url"
+	neturl "net/url"
 	"reflect"
 	"strings"
 	"sync"
@@ -58,17 +58,22 @@ func (p *path) Path(values map[string]string) (string, error) {
 		if !ok {
 			return "", noPathParameter.Error(key, p.path)
 		}
-		segments[index] = url.PathEscape(value)
+		segments[index] = neturl.PathEscape(value)
 	}
 	return strings.Join(segments, ""), nil
 }
 
+type parsedURL struct {
+	path    *path
+	queries map[string][]string
+}
+
 // Client implements builder pattern for http client.
 type Client struct {
-	endpoint string
-	config   *Config
-	lock     sync.RWMutex
-	paths    map[string]*path
+	endpoint       string
+	config         *Config
+	lock           sync.RWMutex
+	parsedURLCache map[string]parsedURL
 }
 
 // NewClient creates a client.
@@ -78,33 +83,42 @@ func NewClient(cfg *Config) (*Client, error) {
 		return nil, err
 	}
 	client := &Client{
-		endpoint: fmt.Sprintf("%s://%s/", cfg.Scheme, strings.TrimRight(cfg.Host, "/\\")),
-		config:   cfg,
-		paths:    map[string]*path{},
+		endpoint:       fmt.Sprintf("%s://%s/", cfg.Scheme, strings.TrimRight(cfg.Host, "/\\")),
+		config:         cfg,
+		parsedURLCache: make(map[string]parsedURL),
 	}
 	return client, nil
 }
 
-func (c *Client) parseURL(url string) (*path, error) {
+func (c *Client) parseURL(rawurl string) (parsedURL, error) {
 	c.lock.RLock()
-	p, ok := c.paths[url]
+	p, ok := c.parsedURLCache[rawurl]
 	c.lock.RUnlock()
 	if ok {
 		return p, nil
 	}
 	c.lock.Lock()
 	defer c.lock.Unlock()
-	p, ok = c.paths[url]
+	p, ok = c.parsedURLCache[rawurl]
 	if ok {
 		return p, nil
 	}
-	segments, err := router.Split(url)
+
+	url, err := neturl.Parse(rawurl)
+	if err != nil {
+		return parsedURL{}, invalidPath.Error(url, err.Error())
+	}
+
+	segments, err := router.Split(url.Path)
 	if err != nil {
-		return nil, invalidPath.Error(url, err.Error())
+		return parsedURL{}, invalidPath.Error(url, err.Error())
 	}
-	p = &path{
-		path:  url,
-		names: map[string]int{},
+	p = parsedURL{
+		path: &path{
+			path:  url.Path,
+			names: map[string]int{},
+		},
+		queries: url.Query(),
 	}
 	for i, segment := range segments {
 		if strings.HasPrefix(segment, "{") {
@@ -114,30 +128,30 @@ func (c *Client) parseURL(url string) (*path, error) {
 			if index > 0 {
 				segment = segment[:index]
 			}
-			if _, ok := p.names[segment]; ok {
-				return nil, duplicatedPathParameter.Error(segment, p.path)
+			if _, ok := p.path.names[segment]; ok {
+				return parsedURL{}, duplicatedPathParameter.Error(segment, p.path)
 			}
-			p.names[segment] = i
+			p.path.names[segment] = i
 		}
-		p.segments = append(p.segments, segment)
+		p.path.segments = append(p.path.segments, segment)
 	}
-	c.paths[url] = p
+	c.parsedURLCache[rawurl] = p
 	return p, nil
 }
 
 // Request creates an request with specific method and url path.
 // The code is only for checking if status code of response is right.
 func (c *Client) Request(method string, code int, url string) *Request {
-	path, err := c.parseURL(url)
+	parsedURL, err := c.parseURL(url)
 	req := &Request{
 		err:      err,
 		method:   method,
 		code:     code,
 		endpoint: c.endpoint,
-		path:     path,
+		path:     parsedURL.path,
 		client:   c.config.Executor,
 		paths:    map[string]string{},
-		queries:  map[string][]string{},
+		queries:  parsedURL.queries,
 		headers:  map[string][]string{},
 		forms:    map[string][]string{},
 		files:    map[string]interface{}{},
@@ -260,7 +274,7 @@ func (r *Request) request(ctx context.Context) (*http.Request, error) {
 		return nil, err
 	}
 	path := r.endpoint + strings.TrimLeft(urlPath, "/")
-	urlVal := url.Values(r.queries)
+	urlVal := neturl.Values(r.queries)
 	if len(urlVal) > 0 {
 		path += "?" + urlVal.Encode()
 	}
@@ -324,7 +338,7 @@ func (r *Request) request(ctx context.Context) (*http.Request, error) {
 		} else if len(r.forms) > 0 {
 			// Write form data to buffer.
 			contentType = definition.MIMEURLEncoded
-			formVal := url.Values(r.forms)
+			formVal := neturl.Values(r.forms)
 			_, err := buf.WriteString(formVal.Encode())
 			if err != nil {
 				return nil, unwritableForms.Error(r.path.String(), err.Error())


Test patch:
diff --git a/rest/rest_test.go b/rest/rest_test.go
new file mode 100644
index 0000000..e870797
--- /dev/null
+++ b/rest/rest_test.go
@@ -0,0 +1,119 @@
+/*
+Copyright 2018 Caicloud Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rest
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestClient_parseURL(t *testing.T) {
+	cases := []struct {
+		rawurl    string
+		parsedURL parsedURL
+	}{
+		{
+			rawurl: "/apis/v1/messages/{message}",
+			parsedURL: parsedURL{
+				path: &path{
+					path: "/apis/v1/messages/{message}",
+					names: map[string]int{
+						"message": 1,
+					},
+					segments: []string{
+						"/apis/v1/messages/",
+						"message",
+					},
+				},
+				queries: map[string][]string{},
+			},
+		},
+		{
+			rawurl: "/?Action=GetMessage&Version=2020-01-01",
+			parsedURL: parsedURL{
+				path: &path{
+					path:     "/",
+					names:    map[string]int{},
+					segments: []string{"/"},
+				},
+				queries: map[string][]string{
+					"Action":  {"GetMessage"},
+					"Version": {"2020-01-01"},
+				},
+			},
+		},
+		{
+			rawurl: "/apis/v1/messages/{message}?Action=GetMessage&Version=2020-01-01",
+			parsedURL: parsedURL{
+				path: &path{
+					path: "/apis/v1/messages/{message}",
+					names: map[string]int{
+						"message": 1,
+					},
+					segments: []string{
+						"/apis/v1/messages/",
+						"message",
+					},
+				},
+				queries: map[string][]string{
+					"Action":  {"GetMessage"},
+					"Version": {"2020-01-01"},
+				},
+			},
+		},
+		// test cache return
+		{
+			rawurl: "/apis/v1/messages/{message}?Action=GetMessage&Version=2020-01-01",
+			parsedURL: parsedURL{
+				path: &path{
+					path: "/apis/v1/messages/{message}",
+					names: map[string]int{
+						"message": 1,
+					},
+					segments: []string{
+						"/apis/v1/messages/",
+						"message",
+					},
+				},
+				queries: map[string][]string{
+					"Action":  {"GetMessage"},
+					"Version": {"2020-01-01"},
+				},
+			},
+		},
+	}
+
+	cli, err := NewClient(&Config{
+		Scheme:   "",
+		Host:     "test",
+		Executor: nil,
+	})
+	if err != nil {
+		t.Fatalf("new client: %v", err)
+	}
+	for _, cs := range cases {
+		parsedURL, err := cli.parseURL(cs.rawurl)
+		if err != nil {
+			t.Errorf("parse url: %v", err)
+			continue
+		}
+		if !reflect.DeepEqual(parsedURL, cs.parsedURL) {
+			t.Errorf("unexpected result, parsedURL is %v", parsedURL)
+			continue
+		}
+	}
+}