fix(be): Modify the upstream to use the new JSON patch package #1395

Jaycean · 2021-01-29T15:20:45Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
feat: Modify the usage of upstream and service go-jsonpatch to json-patch/v5 #1392
feat: Improve testing coverage for the POST and PATCH methods of the resources #1332

Bugfix

Description
I find that the jsonpatch(github.com/api7/go-jsonpatch) used by upstream and service when calling the patch method is still the old package,updated to github.com/evanphx/json-patch/v5
How to fix?
Modify the upstream to use the new JSON patch package

imjoey · 2021-01-29T15:36:11Z

@Jaycean In order to be a semantic pull request, please change the title to fix: Modify the upstream to use the new JSON patch package . Thanks.

codecov-io · 2021-01-29T15:49:26Z

Codecov Report

Merging #1395 (1583435) into master (bff19b5) will increase coverage by 22.24%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           master    #1395       +/-   ##
===========================================
+ Coverage   44.72%   66.97%   +22.24%     
===========================================
  Files          35       44        +9     
  Lines        2522     2937      +415     
===========================================
+ Hits         1128     1967      +839     
+ Misses       1233      729      -504     
- Partials      161      241       +80

Impacted Files	Coverage Δ
api/internal/handler/upstream/upstream.go	`83.05% <78.57%> (+62.88%)`	⬆️
api/cmd/manager/main.go	`0.00% <0.00%> (ø)`
api/internal/conf/conf.go	`67.85% <0.00%> (ø)`
api/internal/core/storage/etcd.go	`48.48% <0.00%> (ø)`
api/internal/route.go	`84.84% <0.00%> (ø)`
api/internal/core/storage/storage_mock.go	`0.00% <0.00%> (ø)`
...l/handler/route_online_debug/route_online_debug.go	`73.80% <0.00%> (ø)`
api/internal/handler/healthz/healthz.go	`66.66% <0.00%> (ø)`
api/internal/log/log.go	`60.00% <0.00%> (ø)`
api/cmd/managerapi.go	`60.29% <0.00%> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bff19b5...1583435. Read the comment docs.

api/internal/handler/upstream/upstream.go

imjoey

@Jaycean Since #1396 has been merged already, the Upstream is the only resource that's using github.com/api7/go-jsonpatch. So within this PR, we would better completely remove the dependency of github.com/api7/go-jsonpatch.

Please rebase the master branch with the latest code and run go mod tidy for that. Thanks.

Jaycean · 2021-01-30T02:53:25Z

@Jaycean Since #1396 has been merged already, the Upstream is the only resource that's using github.com/api7/go-jsonpatch. So within this PR, we would better completely remove the dependency of github.com/api7/go-jsonpatch.

Please rebase the master branch with the latest code and run go mod tidy for that. Thanks.

Thanks for the reminder.done.

imjoey · 2021-01-30T03:15:40Z

@nic-chen Backend E2E Test seems broken since this morning, prompted with the following error. Could you please have a check? Thanks.

--- FAIL: TestBalancer_roundrobin_with_weight (0.66s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/upstreams/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_upstream_(roundrobin_with_same_weight) (0.06s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/routes/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_route_using_the_upstream_just_created (0.31s)
    base.go:150: 
        	Error Trace:	base.go:150
        	            				balancer_test.go:77
        	Error:      	Expected nil, but got: &url.Error{Op:"Get", URL:"http://127.0.0.1:9081/server_port", Err:(*net.OpError)(0xc0001b61e0)}
        	Test:       	TestBalancer_roundrobin_with_weight
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x8c8eed]

tokers · 2021-01-30T06:20:05Z

api/internal/handler/upstream/upstream.go

-	}
+	input := c.Input().(*PatchInput)
+	reqBody := input.Body
+	ID := input.ID


we should use variables started with letter in small case.

tokers · 2021-01-30T06:20:38Z

api/internal/handler/upstream/upstream.go

-	if err := patch.Apply(&stored); err != nil {
+	var upstream entity.Upstream
+	err = json.Unmarshal(res, &upstream)
+	if err != nil {


Can be merged into:

if err := json.Unmarshal(res, &upstream); err != nil {

tokers · 2021-01-30T06:20:57Z

api/internal/handler/upstream/upstream.go

-	if err := patch.Apply(&stored); err != nil {
+	var upstream entity.Upstream
+	err = json.Unmarshal(res, &upstream)
+	if err != nil {


Can be merged into:

if err := json.Unmarshal(res, &upstream); err != nil {

tokers · 2021-01-30T06:21:18Z

api/internal/handler/upstream/upstream.go

@@ -314,3 +314,4 @@ func (h *Handler) listUpstreamNames(c droplet.Context) (interface{}, error) {

 	return output, nil
 }
+


Why keep an empty line here?

This is my problem. Maybe I forgot to delete it when using vim. Done. Thks

nic-chen · 2021-01-30T13:17:25Z

@nic-chen Backend E2E Test seems broken since this morning, prompted with the following error. Could you please have a check? Thanks.

--- FAIL: TestBalancer_roundrobin_with_weight (0.66s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/upstreams/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_upstream_(roundrobin_with_same_weight) (0.06s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/routes/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_route_using_the_upstream_just_created (0.31s)
    base.go:150: 
        	Error Trace:	base.go:150
        	            				balancer_test.go:77
        	Error:      	Expected nil, but got: &url.Error{Op:"Get", URL:"http://127.0.0.1:9081/server_port", Err:(*net.OpError)(0xc0001b61e0)}
        	Test:       	TestBalancer_roundrobin_with_weight
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x8c8eed]

@imjoey @Jaycean hi, have you found the reason of the failure of ci?

Jaycean · 2021-01-30T13:21:39Z

@nic-chen Backend E2E Test seems broken since this morning, prompted with the following error. Could you please have a check? Thanks.

--- FAIL: TestBalancer_roundrobin_with_weight (0.66s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/upstreams/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_upstream_(roundrobin_with_same_weight) (0.06s)
    printer.go:54: PUT http://127.0.0.1:9000/apisix/admin/routes/1
    --- PASS: TestBalancer_roundrobin_with_weight/create_route_using_the_upstream_just_created (0.31s)
    base.go:150: 
        	Error Trace:	base.go:150
        	            				balancer_test.go:77
        	Error:      	Expected nil, but got: &url.Error{Op:"Get", URL:"http://127.0.0.1:9081/server_port", Err:(*net.OpError)(0xc0001b61e0)}
        	Test:       	TestBalancer_roundrobin_with_weight
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x8c8eed]

@imjoey @Jaycean hi, have you found the reason of the failure of ci?
At present, we can only locate there

func BatchTestServerPort(t *testing.T, times int) map[string]int {
	url := APISIXSingleWorkerHost + "/server_port"
	req, err := http.NewRequest(http.MethodGet, url, nil)
	assert.Nil(t, err)

APISIXSingleWorkerHost = "http://127.0.0.1:9081"

This requesthttp://127.0.0.1:9081/server_port
A new route is created, which is exposed by apimix. It looks like a network problem here, but it can't be reproduced locally. No further problem is found.

imjoey · 2021-01-30T13:39:12Z

@imjoey @Jaycean hi, have you found the reason of the failure of ci?

@nic-chen sorry, not get the solution yet. 😢

nic-chen · 2021-01-30T15:56:19Z

@Jaycean @imjoey
It seems to be affected by the last merged commit of APISIX

Jaycean · 2021-01-31T01:50:54Z

@Jaycean @imjoey
It seems to be affected by the last merged commit of APISIX

Yes, just now I pulled the latest code of apisix, and the error has been reproduced locally.

nic-chen · 2021-02-01T03:07:38Z

@Jaycean CI passed

imjoey · 2021-02-01T03:13:06Z

@Jaycean CI passed

@nic-chen great thanks.

imjoey

/lgtm

Thanks @Jaycean .

Jaycean · 2021-02-01T03:23:56Z

@Jaycean CI passed

Great Thks.

Jaycean changed the title ~~Modify the upstream to use the new JSON patch package~~ fix: Modify the upstream to use the new JSON patch package Jan 29, 2021

Jaycean changed the title ~~fix: Modify the upstream to use the new JSON patch package~~ fix(be): Modify the upstream to use the new JSON patch package Jan 29, 2021

membphis approved these changes Jan 30, 2021

View reviewed changes

nic-chen reviewed Jan 30, 2021

View reviewed changes

api/internal/handler/upstream/upstream.go Show resolved Hide resolved

imjoey requested changes Jan 30, 2021

View reviewed changes

Jaycean added 2 commits January 30, 2021 10:45

Modify the upstream to use the new JSON patch package

67953ba

go mod tidy

bfdea43

Jaycean force-pushed the Modify_the_upstream_to_use_the_new_json-patch_package branch from 657431f to bfdea43 Compare January 30, 2021 02:48

tokers reviewed Jan 30, 2021

View reviewed changes

Fix formatting and naming errors

39790df

johzchen added 2 commits January 30, 2021 21:23

test: debug

da8eb20

test: debug

642a78c

johzchen added 3 commits January 30, 2021 22:36

test: debug

d8fceeb

fix path

286893a

fix CI failed

61bc5f8

johzchen added 2 commits January 31, 2021 10:52

fix: revert debug

6962344

fix: e2e test failed

1583435

imjoey approved these changes Feb 1, 2021

View reviewed changes

liuxiran merged commit 94d952f into apache:master Feb 1, 2021

Jaycean mentioned this pull request Feb 1, 2021

TestBalancer_roundrobin_with_weight failed #1401

Closed

Jaycean deleted the Modify_the_upstream_to_use_the_new_json-patch_package branch February 3, 2021 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(be): Modify the upstream to use the new JSON patch package #1395

fix(be): Modify the upstream to use the new JSON patch package #1395

Jaycean commented Jan 29, 2021 •

edited

Loading

imjoey commented Jan 29, 2021

codecov-io commented Jan 29, 2021 •

edited

Loading

imjoey left a comment

Jaycean commented Jan 30, 2021

imjoey commented Jan 30, 2021

tokers Jan 30, 2021

Jaycean Jan 30, 2021

tokers Jan 30, 2021

Jaycean Jan 30, 2021

tokers Jan 30, 2021

Jaycean Feb 1, 2021

tokers Jan 30, 2021

Jaycean Jan 30, 2021

nic-chen commented Jan 30, 2021

Jaycean commented Jan 30, 2021

imjoey commented Jan 30, 2021

nic-chen commented Jan 30, 2021

Jaycean commented Jan 31, 2021

nic-chen commented Feb 1, 2021

imjoey commented Feb 1, 2021

imjoey left a comment

Jaycean commented Feb 1, 2021

		@@ -314,3 +314,4 @@ func (h *Handler) listUpstreamNames(c droplet.Context) (interface{}, error) {

		return output, nil
		}

fix(be): Modify the upstream to use the new JSON patch package #1395

fix(be): Modify the upstream to use the new JSON patch package #1395

Conversation

Jaycean commented Jan 29, 2021 • edited Loading

Bugfix

imjoey commented Jan 29, 2021

codecov-io commented Jan 29, 2021 • edited Loading

Codecov Report

imjoey left a comment

Choose a reason for hiding this comment

Jaycean commented Jan 30, 2021

imjoey commented Jan 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nic-chen commented Jan 30, 2021

Jaycean commented Jan 30, 2021

imjoey commented Jan 30, 2021

nic-chen commented Jan 30, 2021

Jaycean commented Jan 31, 2021

nic-chen commented Feb 1, 2021

imjoey commented Feb 1, 2021

imjoey left a comment

Choose a reason for hiding this comment

Jaycean commented Feb 1, 2021

Jaycean commented Jan 29, 2021 •

edited

Loading

codecov-io commented Jan 29, 2021 •

edited

Loading