feat:Support duplicate one existing Route #1558

batman-ezio · 2021-03-05T08:24:44Z

Please answer these questions before submitting a pull request

Why submit this pull request?

Support duplicate one existing Route #1487

New feature or improvement
1: duplicate button from route list
2: check duplicate name from POST route api

Describe the details and related test reports.

1:Click duplicate button from the router list, the address will be routes/:rid/duplicate

2: update the name to make sure it's no duplicated

3: submit data, return to the list. The new route will be there

nic-chen · 2021-03-05T09:46:27Z

api/internal/handler/route/route.go

@@ -307,6 +307,12 @@ func generateLuaCode(script map[string]interface{}) (string, error) {

 func (h *Handler) Create(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*entity.Route)
+	// check duplicate name
+	if err := h.checkDuplicateName(c, input.Name, ""); err != nil {


should add this check for Update too. Thanks.

nic-chen · 2021-03-05T09:48:12Z

need some test cases for duplicate check, thanks. @batman-ezio

codecov-io · 2021-03-06T11:04:52Z

Codecov Report

Merging #1558 (a57e082) into master (8e009b0) will decrease coverage by 20.69%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1558       +/-   ##
===========================================
- Coverage   72.46%   51.77%   -20.70%     
===========================================
  Files         134       38       -96     
  Lines        5738     2650     -3088     
  Branches      666        0      -666     
===========================================
- Hits         4158     1372     -2786     
+ Misses       1337     1090      -247     
+ Partials      243      188       -55

Flag	Coverage Δ
backend-e2e-test	`?`
backend-e2e-test-ginkgo	`?`
backend-unit-test	`51.77% <ø> (-0.04%)`	⬇️
frontend-e2e-test	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/internal/utils/version.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/filter/request_id.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/core/entity/entity.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/core/store/storehub.go	`0.00% <0.00%> (-71.03%)`	⬇️
api/internal/filter/cors.go	`0.00% <0.00%> (-66.67%)`	⬇️
api/internal/filter/schema.go	`0.00% <0.00%> (-55.47%)`	⬇️
api/internal/utils/consts/api_error.go	`0.00% <0.00%> (-50.00%)`	⬇️
api/internal/handler/data_loader/route_import.go	`27.41% <0.00%> (-37.50%)`	⬇️
api/internal/handler/handler.go	`42.59% <0.00%> (-35.19%)`	⬇️
api/internal/handler/schema/schema.go	`66.66% <0.00%> (-33.34%)`	⬇️
... and 118 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 8e009b0...a57e082. Read the comment docs.

batman-ezio · 2021-03-08T01:14:19Z

need some test cases for duplicate check, thanks. @batman-ezio

@nic-chen after I check duplicate name in create/update route api. some test cases failed. I think they are use the same route name to test before. some errors throw from the route test are very strange , like this

=== RUN   TestRoute_Create/create_route_success
panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("List").Return(...) first, or remove the List() call.
	This method was unexpected:
		List(store.ListInput)
		0: store.ListInput{Predicate:(func(interface {}) bool)(0x11bf5a0), Format:(func(interface {}) interface {})(nil), PageSize:0, PageNumber:0, Less:(func(interface {}, interface {}) bool)(nil)}
	at: [store_mock.go:35 route.go:537 route.go:311 route_test.go:1005] [recovered]
	panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("List").Return(...) first, or remove the List() call.
	This method was unexpected:
		List(store.ListInput)
		0: store.ListInput{Predicate:(func(interface {}) bool)(0x11bf5a0), Format:(func(interface {}) interface {})(nil), PageSize:0, PageNumber:0, Less:(func(interface {}, interface {}) bool)(nil)}
	at: [store_mock.go:35 route.go:537 route.go:311 route_test.go:1005]

I have change the route name but still got this. can you help?

nic-chen · 2021-03-08T01:41:53Z

need some test cases for duplicate check, thanks. @batman-ezio

@nic-chen after I check duplicate name in create/update route api. some test cases failed. I think they are use the same route name to test before. some errors throw from the route test are very strange , like this

=== RUN   TestRoute_Create/create_route_success
panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("List").Return(...) first, or remove the List() call.
	This method was unexpected:
		List(store.ListInput)
		0: store.ListInput{Predicate:(func(interface {}) bool)(0x11bf5a0), Format:(func(interface {}) interface {})(nil), PageSize:0, PageNumber:0, Less:(func(interface {}, interface {}) bool)(nil)}
	at: [store_mock.go:35 route.go:537 route.go:311 route_test.go:1005] [recovered]
	panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("List").Return(...) first, or remove the List() call.
	This method was unexpected:
		List(store.ListInput)
		0: store.ListInput{Predicate:(func(interface {}) bool)(0x11bf5a0), Format:(func(interface {}) interface {})(nil), PageSize:0, PageNumber:0, Less:(func(interface {}, interface {}) bool)(nil)}
	at: [store_mock.go:35 route.go:537 route.go:311 route_test.go:1005]

I have change the route name but still got this. can you help?

hi @batman-ezio , you need to mock routeStore.List for checkDuplicateName

you could refer to:

apisix-dashboard/api/internal/handler/plugin_config/plugin_config_test.go

Lines 694 to 709 in 2de2c72

    
           pluginConfigStore := &store.MockInterface{} 
        
           pluginConfigStore.On("BatchDelete", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { 
        
           	getCalled = true 
        
           	input := args.Get(1).([]string) 
        
           	assert.Equal(t, tc.wantInput, input) 
        
           }).Return(tc.giveErr) 
        
           mockRouteStore := &store.MockInterface{} 
        
           mockRouteStore.On("List", mock.Anything).Run(func(args mock.Arguments) { 
        
           	getCalled = true 
        
           }).Return(tc.listRet, nil) 
        
           h := Handler{pluginConfigStore: pluginConfigStore, routeStore: mockRouteStore} 
        
           ctx := droplet.NewContext() 
        
           ctx.SetInput(tc.giveInput) 
        
           ret, err := h.BatchDelete(ctx)

batman-ezio · 2021-03-08T09:05:50Z

@batman-ezio
Hi, I see that in your E2E test and unit test, when creating a route, the error code returned is 400, which is usually the wrong data format or type of the route. Please check the data format or type.

hi @Jaycean I found some test case use the same route name or empty route name for e2e test, test cases begin to bump up after my fix one by one

Jaycean · 2021-03-08T09:10:29Z

@batman-ezio
Hi, I see that in your E2E test and unit test, when creating a route, the error code returned is 400, which is usually the wrong data format or type of the route. Please check the data format or type.

hi @Jaycean I found some test case use the same route name or empty route name for e2e test, test cases begin to bump up after my fix one by one

Yes, 400 is usually caused by wrong data format or type. In E2E test, a route ID r1 is usually created and passed, and the route with ID r1 will be deleted, so there is no problem with the same name.

nic-chen · 2021-03-08T10:18:51Z

@batman-ezio would you mind if we modify your code on your branch?

batman-ezio · 2021-03-08T10:35:33Z

@batman-ezio would you mind if we modify your code on your branch?

no, please
I found those test case in ece/route_with_* does not pass Name field in the body, so their route will create with a blank name and will failed for duplicate, can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

the frontend e2e test will be fixed after merge this PR #1559

nic-chen · 2021-03-08T11:14:49Z

can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

oh, no, we should not do that.
we could fix in test cases which do duplicate. I think there will not be a lot, because most of test cases will delete the routes at the end.

batman-ezio · 2021-03-08T11:27:11Z

can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

oh, no, we should not do that.
we could fix in test cases which do duplicate. I think there will not be a lot, because most of test cases will delete the routes at the end.

ok, can you help to fix the rest test cases, thanks

nic-chen · 2021-03-09T00:29:16Z

can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

oh, no, we should not do that.
we could fix in test cases which do duplicate. I think there will not be a lot, because most of test cases will delete the routes at the end.

ok, can you help to fix the rest test cases, thanks

sure, my pleasure. will update later.

batman-ezio · 2021-03-09T00:34:45Z

can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

oh, no, we should not do that.
we could fix in test cases which do duplicate. I think there will not be a lot, because most of test cases will delete the routes at the end.

ok, can you help to fix the rest test cases, thanks

sure, my pleasure. will update later.

thanks. I have squashed all my test case commits into one commit, and rebased from master (which includes the latest frontend e2e fix), but the test case still failed

nic-chen · 2021-03-09T04:19:20Z

can we add a logic in create/update API to set the name from url params if it does not exists in the request body?

oh, no, we should not do that.
we could fix in test cases which do duplicate. I think there will not be a lot, because most of test cases will delete the routes at the end.

ok, can you help to fix the rest test cases, thanks

sure, my pleasure. will update later.

thanks. I have squashed all my test case commits into one commit, and rebased from master (which includes the latest frontend e2e fix), but the test case still failed

I will try to fix backend CI this afternoon.
@LiteSun could you please help for the FE?

LiteSun · 2021-03-09T04:31:27Z

I will try to fix backend CI this afternoon.
@LiteSun could you please help for the FE?

ok

starsz · 2021-03-09T10:01:15Z

CI failed, wait for @nic-chen check this.

nic-chen · 2021-03-09T10:32:35Z

There are some logical problems in E2E test cases, need more time resolve

nic-chen · 2021-03-10T03:28:15Z

backend CI had been resolved.
please help check FE CI when you have time. @LiteSun

membphis · 2021-03-14T15:38:17Z

ping @LiteSun

web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

nic-chen · 2021-03-23T06:38:45Z

please resolve conflicts

batman-ezio · 2021-03-23T23:55:50Z

please resolve conflicts

fixed

juzhiyuan · 2021-04-02T01:09:02Z

web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

+  });
+
+  it('should delete the route', function () {
+    cy.visit('/routes/list');


@LiteSun Do we need to visit this page by clicking Sidebar Menu?

liuxiran

LGTM， thanks

and there is a UE enhancement suggestion:
when copying the route user in the page of first step and click [Next] button directly, they will get duplicate route nameerror, maybe leaving the route name blank would be a better choice.

juzhiyuan · 2021-04-02T02:07:52Z

ok, then let me merge.

juzhiyuan · 2021-04-02T02:08:42Z

thanks! @batman-ezio

batman-ezio force-pushed the issue_1487 branch 3 times, most recently from 38cc523 to fb85518 Compare March 5, 2021 09:40

nic-chen reviewed Mar 5, 2021

View reviewed changes

nic-chen requested review from LiteSun, juzhiyuan and starsz March 5, 2021 09:47

batman-ezio force-pushed the issue_1487 branch 3 times, most recently from d4753d8 to 65f3a7c Compare March 8, 2021 23:54

batman-ezio force-pushed the issue_1487 branch from 65f3a7c to fa29e19 Compare March 9, 2021 00:33

batman-ezio force-pushed the issue_1487 branch from 958fa79 to 0c30499 Compare March 14, 2021 23:58

batman-ezio force-pushed the issue_1487 branch 2 times, most recently from db8b1b2 to fda5d75 Compare March 20, 2021 01:08

LiteSun reviewed Mar 22, 2021

View reviewed changes

web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js Show resolved Hide resolved

batman-ezio force-pushed the issue_1487 branch from b1a68ee to 067ef94 Compare March 22, 2021 06:32

guoqqqi reviewed Mar 22, 2021

View reviewed changes

web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js Outdated Show resolved Hide resolved

batman-ezio force-pushed the issue_1487 branch from 92363ba to 8246bd7 Compare March 23, 2021 11:16

batman-ezio force-pushed the issue_1487 branch from 8246bd7 to a57e082 Compare March 23, 2021 23:59

batman-ezio requested review from juzhiyuan, LiteSun and nic-chen March 28, 2021 23:39

juzhiyuan requested a review from liuxiran March 29, 2021 15:04

nic-chen requested a review from bzp2010 March 30, 2021 05:23

LiteSun approved these changes Mar 31, 2021

View reviewed changes

batman-ezio force-pushed the issue_1487 branch from a57e082 to 4586684 Compare April 1, 2021 06:31

ezioruan added 6 commits April 1, 2021 14:33

feat: support duplicate one existing Route

9ba5add

feature: duplicate test case

610f10d

fix: test case

db0508f

fix: test case

8811720

fix: test case

164c92c

fix: test case

ade7b83

batman-ezio force-pushed the issue_1487 branch from 4586684 to ade7b83 Compare April 1, 2021 06:34

juzhiyuan reviewed Apr 2, 2021

View reviewed changes

liuxiran approved these changes Apr 2, 2021

View reviewed changes

juzhiyuan approved these changes Apr 2, 2021

View reviewed changes

juzhiyuan merged commit 9cf91b7 into apache:master Apr 2, 2021

juzhiyuan mentioned this pull request Apr 2, 2021

improve the Duplicate Route feature #1703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:Support duplicate one existing Route #1558

feat:Support duplicate one existing Route #1558

batman-ezio commented Mar 5, 2021 •

edited

Loading

nic-chen Mar 5, 2021

batman-ezio Mar 6, 2021

nic-chen commented Mar 5, 2021

codecov-io commented Mar 6, 2021 •

edited

Loading

batman-ezio commented Mar 8, 2021

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021

Jaycean commented Mar 8, 2021 •

edited

Loading

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021 •

edited

Loading

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021

nic-chen commented Mar 9, 2021

batman-ezio commented Mar 9, 2021

nic-chen commented Mar 9, 2021

LiteSun commented Mar 9, 2021

starsz commented Mar 9, 2021 •

edited

Loading

nic-chen commented Mar 9, 2021

nic-chen commented Mar 10, 2021

membphis commented Mar 14, 2021

nic-chen commented Mar 23, 2021

batman-ezio commented Mar 23, 2021

juzhiyuan Apr 2, 2021

liuxiran left a comment

juzhiyuan commented Apr 2, 2021

juzhiyuan commented Apr 2, 2021

feat:Support duplicate one existing Route #1558

feat:Support duplicate one existing Route #1558

Conversation

batman-ezio commented Mar 5, 2021 • edited Loading

nic-chen Mar 5, 2021

Choose a reason for hiding this comment

batman-ezio Mar 6, 2021

Choose a reason for hiding this comment

nic-chen commented Mar 5, 2021

codecov-io commented Mar 6, 2021 • edited Loading

Codecov Report

batman-ezio commented Mar 8, 2021

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021

Jaycean commented Mar 8, 2021 • edited Loading

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021 • edited Loading

nic-chen commented Mar 8, 2021

batman-ezio commented Mar 8, 2021

nic-chen commented Mar 9, 2021

batman-ezio commented Mar 9, 2021

nic-chen commented Mar 9, 2021

LiteSun commented Mar 9, 2021

starsz commented Mar 9, 2021 • edited Loading

nic-chen commented Mar 9, 2021

nic-chen commented Mar 10, 2021

membphis commented Mar 14, 2021

nic-chen commented Mar 23, 2021

batman-ezio commented Mar 23, 2021

juzhiyuan Apr 2, 2021

Choose a reason for hiding this comment

liuxiran left a comment

Choose a reason for hiding this comment

juzhiyuan commented Apr 2, 2021

juzhiyuan commented Apr 2, 2021

batman-ezio commented Mar 5, 2021 •

edited

Loading

codecov-io commented Mar 6, 2021 •

edited

Loading

Jaycean commented Mar 8, 2021 •

edited

Loading

batman-ezio commented Mar 8, 2021 •

edited

Loading

starsz commented Mar 9, 2021 •

edited

Loading