You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by ju...@apache.org on 2020/12/11 08:09:38 UTC
[apisix-dashboard] branch master updated: fix: PATCH method bug
(#1005)
This is an automated email from the ASF dual-hosted git repository.
juzhiyuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/apisix-dashboard.git
The following commit(s) were added to refs/heads/master by this push:
new b157cc2 fix: PATCH method bug (#1005)
b157cc2 is described below
commit b157cc26aa0f2d01ae2911be697e320e084d9f41
Author: nic-chen <33...@users.noreply.github.com>
AuthorDate: Fri Dec 11 16:08:53 2020 +0800
fix: PATCH method bug (#1005)
* fix: PATCH method bug
* test: use sub path patch in e2e test
* fix: lint
* fix: naming stype
* fix: according to review
* fix: style
---
api/go.mod | 1 +
api/go.sum | 4 +
api/internal/handler/ssl/ssl.go | 42 +++----
api/internal/utils/json_patch.go | 75 ++++++++++++
api/internal/utils/json_patch_test.go | 211 ++++++++++++++++++++++++++++++++++
api/test/e2e/ssl_test.go | 56 ++++++++-
6 files changed, 358 insertions(+), 31 deletions(-)
diff --git a/api/go.mod b/api/go.mod
index 3c48371..b67cd95 100644
--- a/api/go.mod
+++ b/api/go.mod
@@ -11,6 +11,7 @@ require (
github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf // indirect
github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f // indirect
github.com/dgrijalva/jwt-go v3.2.0+incompatible
+ github.com/evanphx/json-patch/v5 v5.1.0
github.com/gin-contrib/pprof v1.3.0
github.com/gin-contrib/sessions v0.0.3
github.com/gin-contrib/static v0.0.0-20200916080430-d45d9a37d28e
diff --git a/api/go.sum b/api/go.sum
index 80c6f95..6798863 100644
--- a/api/go.sum
+++ b/api/go.sum
@@ -22,6 +22,9 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm
github.com/elazarl/go-bindata-assetfs v1.0.0/go.mod h1:v+YaWX3bdea5J/mo8dSETolEo7R71Vk1u8bnjau5yw4=
github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
+github.com/evanphx/json-patch v4.9.0+incompatible h1:kLcOMZeuLAJvL2BPWLMIj5oaZQobrkAqrL+WFZwQses=
+github.com/evanphx/json-patch/v5 v5.1.0 h1:B0aXl1o/1cP8NbviYiBMkcHBtUjIJ1/Ccg6b+SwCLQg=
+github.com/evanphx/json-patch/v5 v5.1.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4=
github.com/gin-contrib/pprof v1.3.0 h1:G9eK6HnbkSqDZBYbzG4wrjCsA4e+cvYAHUZw6W+W9K0=
github.com/gin-contrib/pprof v1.3.0/go.mod h1:waMjT1H9b179t3CxuG1cV3DHpga6ybizwfBaM5OXaB0=
github.com/gin-contrib/sessions v0.0.3 h1:PoBXki+44XdJdlgDqDrY5nDVe3Wk7wDV/UCOuLP6fBI=
@@ -77,6 +80,7 @@ github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+
github.com/gorilla/sessions v1.1.1/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w=
github.com/gorilla/sessions v1.1.3 h1:uXoZdcdA5XdXF3QzuSlheVRUvjl+1rKY7zBXL68L9RU=
github.com/gorilla/sessions v1.1.3/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w=
+github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI=
github.com/json-iterator/go v1.1.7/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4=
github.com/json-iterator/go v1.1.9/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4=
github.com/json-iterator/go v1.1.10/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4=
diff --git a/api/internal/handler/ssl/ssl.go b/api/internal/handler/ssl/ssl.go
index b49ecea..1900995 100644
--- a/api/internal/handler/ssl/ssl.go
+++ b/api/internal/handler/ssl/ssl.go
@@ -27,7 +27,6 @@ import (
"reflect"
"strings"
- "github.com/api7/go-jsonpatch"
"github.com/gin-gonic/gin"
"github.com/shiningrush/droplet"
"github.com/shiningrush/droplet/data"
@@ -38,6 +37,7 @@ import (
"github.com/apisix/manager-api/internal/core/entity"
"github.com/apisix/manager-api/internal/core/store"
"github.com/apisix/manager-api/internal/handler"
+ "github.com/apisix/manager-api/internal/utils"
"github.com/apisix/manager-api/internal/utils/consts"
)
@@ -62,13 +62,14 @@ func (h *Handler) ApplyRoute(r *gin.Engine) {
wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
r.PUT("/apisix/admin/ssl/:id", wgin.Wraps(h.Update,
wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
- r.PATCH("/apisix/admin/ssl/:id", wgin.Wraps(h.Patch,
- wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
r.DELETE("/apisix/admin/ssl/:ids", wgin.Wraps(h.BatchDelete,
wrapper.InputType(reflect.TypeOf(BatchDelete{}))))
r.POST("/apisix/admin/check_ssl_cert", wgin.Wraps(h.Validate,
wrapper.InputType(reflect.TypeOf(entity.SSL{}))))
+ r.PATCH("/apisix/admin/ssl/:id", consts.ErrorWrapper(Patch))
+ r.PATCH("/apisix/admin/ssl/:id/*path", consts.ErrorWrapper(Patch))
+
r.POST("/apisix/admin/check_ssl_exists", consts.ErrorWrapper(Exist))
}
@@ -216,40 +217,29 @@ func (h *Handler) Update(c droplet.Context) (interface{}, error) {
return nil, nil
}
-func (h *Handler) Patch(c droplet.Context) (interface{}, error) {
- input := c.Input().(*UpdateInput)
- arr := strings.Split(input.ID, "/")
- var subPath string
- if len(arr) > 1 {
- input.ID = arr[0]
- subPath = arr[1]
- }
+func Patch(c *gin.Context) (interface{}, error) {
+ reqBody, _ := c.GetRawData()
+ ID := c.Param("id")
+ subPath := c.Param("path")
- stored, err := h.sslStore.Get(input.ID)
+ sslStore := store.GetStore(store.HubKeySsl)
+ stored, err := sslStore.Get(ID)
if err != nil {
return handler.SpecCodeResponse(err), err
}
- var patch jsonpatch.Patch
- if subPath != "" {
- patch = jsonpatch.Patch{
- Operations: []jsonpatch.PatchOperation{
- {Op: jsonpatch.Replace, Path: subPath, Value: c.Input()},
- },
- }
- } else {
- patch, err = jsonpatch.MakePatch(stored, input.SSL)
- if err != nil {
- return handler.SpecCodeResponse(err), err
- }
+ res, err := utils.MergePatch(stored, subPath, reqBody)
+ if err != nil {
+ return handler.SpecCodeResponse(err), err
}
- err = patch.Apply(&stored)
+ var ssl entity.SSL
+ err = json.Unmarshal(res, &ssl)
if err != nil {
return handler.SpecCodeResponse(err), err
}
- if err := h.sslStore.Update(c.Context(), &stored, false); err != nil {
+ if err := sslStore.Update(c, &ssl, false); err != nil {
return handler.SpecCodeResponse(err), err
}
diff --git a/api/internal/utils/json_patch.go b/api/internal/utils/json_patch.go
new file mode 100644
index 0000000..d6bcd1d
--- /dev/null
+++ b/api/internal/utils/json_patch.go
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 utils
+
+import (
+ "encoding/json"
+ jsonpatch "github.com/evanphx/json-patch/v5"
+)
+
+func MergeJson(doc, patch []byte) ([]byte, error) {
+ out, err := jsonpatch.MergePatch(doc, patch)
+
+ if err != nil {
+ return nil, err
+ }
+ return out, nil
+}
+
+func PatchJson(doc []byte, path, val string) ([]byte, error) {
+ patch := []byte(`[ { "op": "replace", "path": "` + path + `", "value": ` + val + `}]`)
+ obj, err := jsonpatch.DecodePatch(patch)
+ if err != nil {
+ return nil, err
+ }
+
+ out, err := obj.Apply(doc)
+
+ if err != nil {
+ // try to add if field not exist
+ patch = []byte(`[ { "op": "add", "path": "` + path + `", "value": ` + val + `}]`)
+ obj, err = jsonpatch.DecodePatch(patch)
+ if err != nil {
+ return nil, err
+ }
+ out, err = obj.Apply(doc)
+ if err != nil {
+ return nil, err
+ }
+ }
+
+ return out, nil
+}
+
+func MergePatch(obj interface{}, subPath string, reqBody []byte) ([]byte, error) {
+ var res []byte
+ jsonBytes, err := json.Marshal(obj)
+ if err != nil {
+ return res, err
+ }
+
+ if subPath != "" {
+ res, err = PatchJson(jsonBytes, subPath, string(reqBody))
+ } else {
+ res, err = MergeJson(jsonBytes, reqBody)
+ }
+
+ if err != nil {
+ return res, err
+ }
+ return res, nil
+}
diff --git a/api/internal/utils/json_patch_test.go b/api/internal/utils/json_patch_test.go
new file mode 100644
index 0000000..8dd2540
--- /dev/null
+++ b/api/internal/utils/json_patch_test.go
@@ -0,0 +1,211 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 utils
+
+import (
+ "bytes"
+ "encoding/json"
+ "reflect"
+ "testing"
+)
+
+func compareJSON(a, b string) bool {
+ var objA, objB interface{}
+ json.Unmarshal([]byte(a), &objA)
+ json.Unmarshal([]byte(b), &objB)
+
+ return reflect.DeepEqual(objA, objB)
+}
+
+func formatJSON(j string) string {
+ buf := new(bytes.Buffer)
+
+ json.Indent(buf, []byte(j), "", " ")
+
+ return buf.String()
+}
+
+func TestMergeJson(t *testing.T) {
+ cases := []struct {
+ doc, patch, result, desc string
+ }{
+ {
+ desc: "simple merge",
+ doc: `{
+ "id": "1",
+ "status": 1,
+ "key": "fake key",
+ "cert": "fake cert",
+ "create_time": 1,
+ "update_time": 2
+ }`,
+ patch: `{
+ "id": "1",
+ "status": 0,
+ "key": "fake key1",
+ "cert": "fake cert1"
+ }`,
+ result: `{
+ "id": "1",
+ "status": 0,
+ "key": "fake key1",
+ "cert": "fake cert1",
+ "create_time": 1,
+ "update_time": 2
+ }`,
+ },
+ {
+ desc: `array merge`,
+ doc: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.215",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ patch: `{
+ "upstream": {
+ "nodes": [{
+ "host": "39.97.63.216",
+ "port": 80,
+ "weight" : 1
+ },{
+ "host": "39.97.63.217",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ result: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.216",
+ "port": 80,
+ "weight" : 1
+ },{
+ "host": "39.97.63.217",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ },
+ }
+ for _, c := range cases {
+ out, err := MergeJson([]byte(c.doc), []byte(c.patch))
+
+ if err != nil {
+ t.Errorf("Unable to merge patch: %s", err)
+ }
+
+ if !compareJSON(string(out), c.result) {
+ t.Errorf("Merge failed. Expected:\n%s\n\nActual:\n%s",
+ formatJSON(c.result), formatJSON(string(out)))
+ }
+ }
+}
+
+func TestPatchJson(t *testing.T) {
+ cases := []struct {
+ doc, path, value, result, desc string
+ }{
+ {
+ desc: "patch array",
+ doc: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.215",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ path: `/upstream/nodes`,
+ value: `[{
+ "host": "39.97.63.216",
+ "port": 80,
+ "weight" : 1
+ },{
+ "host": "39.97.63.217",
+ "port": 80,
+ "weight" : 1
+ }]`,
+ result: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.216",
+ "port": 80,
+ "weight" : 1
+ },{
+ "host": "39.97.63.217",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ },
+ {
+ desc: "patch field that non existent",
+ doc: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.215",
+ "port": 80,
+ "weight" : 1
+ }]
+ }
+ }`,
+ path: `/upstream/labels`,
+ value: `{"app": "test"}`,
+ result: `{
+ "uri": "/index.html",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "39.97.63.215",
+ "port": 80,
+ "weight" : 1
+ }],
+ "labels": {"app": "test"}
+ }
+ }`,
+ },
+ }
+ for _, c := range cases {
+ out, err := PatchJson([]byte(c.doc), c.path, c.value)
+ if err != nil {
+ t.Errorf("Unable to patch: %s", err)
+ }
+
+ if !compareJSON(string(out), c.result) {
+ t.Errorf("Patch failed. Expected:\n%s\n\nActual:\n%s",
+ formatJSON(c.result), formatJSON(string(out)))
+ }
+ }
+}
diff --git a/api/test/e2e/ssl_test.go b/api/test/e2e/ssl_test.go
index 85a332e..5290fa3 100644
--- a/api/test/e2e/ssl_test.go
+++ b/api/test/e2e/ssl_test.go
@@ -112,10 +112,13 @@ func TestSSL_Basic(t *testing.T) {
Sleep: sleepTime,
},
{
- caseDesc: "delete ssl",
- Object: ManagerApiExpect(t),
- Method: http.MethodDelete,
- Path: "/apisix/admin/ssl/1",
+ caseDesc: "disable SSL",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodPatch,
+ Path: "/apisix/admin/ssl/1",
+ Body: `{
+ "status": 0
+ }`,
Headers: map[string]string{"Authorization": token},
ExpectStatus: http.StatusOK,
},
@@ -125,6 +128,50 @@ func TestSSL_Basic(t *testing.T) {
testCaseCheck(tc)
}
+ // try again after disable SSL, make a HTTPS request
+ // If use the test framework, errors will cause failure, so we need to make a separate https request for testing.
+ time.Sleep(sleepTime)
+ _, err = http.Get("https://www.test2.com:9443")
+ assert.NotNil(t, err)
+ assert.EqualError(t, err, "Get https://www.test2.com:9443: remote error: tls: internal error")
+
+ // enable SSL again
+ tests = []HttpTestCase{
+ {
+ caseDesc: "enable SSL",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodPatch,
+ Path: "/apisix/admin/ssl/1/status",
+ Body: `1`,
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ },
+ {
+ caseDesc: "hit the route using HTTPS, make sure enable successful",
+ Object: APISIXHTTPSExpect(t),
+ Method: http.MethodGet,
+ Path: "/hello_",
+ Headers: map[string]string{"Host": "www.test2.com"},
+ ExpectStatus: http.StatusOK,
+ ExpectBody: "hello world\n",
+ Sleep: sleepTime,
+ },
+ }
+ for _, tc := range tests {
+ testCaseCheck(tc)
+ }
+
+ // delete SSL
+ delSSL := HttpTestCase{
+ caseDesc: "delete SSL",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodDelete,
+ Path: "/apisix/admin/ssl/1",
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ }
+ testCaseCheck(delSSL)
+
// try again after deleting SSL, make a HTTPS request
// If use the test framework, errors will cause failure, so we need to make a separate https request for testing.
time.Sleep(sleepTime)
@@ -142,5 +189,4 @@ func TestSSL_Basic(t *testing.T) {
ExpectStatus: http.StatusOK,
}
testCaseCheck(delRoute)
-
}