fix: check name existed when creating or updating a resource #1606

nic-chen · 2021-03-17T02:57:31Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues

Bugfix

Description

When creating route, server and upstream in the Dashboard, the FE queries whether the same name exists through API, and does not run the addition if it exists. But the backend lacks this limitation.

How to fix?

Add a judgment on whether the name exists in the backend, if it exists, it is not allowed to create or update.

codecov-io · 2021-03-17T03:12:31Z

Codecov Report

Merging #1606 (63a798f) into master (4b8774f) will decrease coverage by 0.47%.
The diff coverage is 96.49%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   71.37%   70.90%   -0.48%     
==========================================
  Files         133       47      -86     
  Lines        5384     3073    -2311     
  Branches      592        0     -592     
==========================================
- Hits         3843     2179    -1664     
+ Misses       1297      651     -646     
+ Partials      244      243       -1

Flag	Coverage Δ
backend-e2e-test	`61.56% <92.98%> (+0.50%)`	⬆️
backend-e2e-test-ginkgo	`47.08% <92.98%> (+0.45%)`	⬆️
backend-unit-test	`51.26% <43.85%> (-0.36%)`	⬇️
frontend-e2e-test	`?`

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

Impacted Files	Coverage Δ
api/internal/handler/handler.go	`77.77% <92.59%> (+14.81%)`	⬆️
api/internal/handler/route/route.go	`78.77% <100.00%> (+0.53%)`	⬆️
api/internal/handler/service/service.go	`92.00% <100.00%> (+0.51%)`	⬆️
api/internal/handler/upstream/upstream.go	`89.38% <100.00%> (+0.59%)`	⬆️
...eb/src/pages/PluginTemplate/components/Preview.tsx
web/src/components/Plugin/data.tsx
web/src/access.ts
web/src/components/PluginOrchestration/service.ts
web/src/pages/User/Login.tsx
web/src/pages/Route/transform.ts
... and 81 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 4b8774f...63a798f. Read the comment docs.

api/internal/handler/route/route.go

tokers · 2021-03-17T09:13:26Z

api/internal/handler/route/route.go

+		return &data.SpecCodeResponse{StatusCode: http.StatusInternalServerError}, err
+	}
+	if ret.TotalSize > 0 {
+		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, errors.New("route name is existed")


typo: "route name is existed" => "route name exists".

tokers · 2021-03-17T09:16:18Z

api/internal/handler/route/route.go

@@ -489,12 +503,32 @@ func (h *Handler) Update(c droplet.Context) (interface{}, error) {
 		}
 	}

-	ret, err := h.routeStore.Update(c.Context(), &input.Route, true)
+	// check name existed
+	ret, err := h.routeStore.List(c.Context(), store.ListInput{


All these logics are similar, can we abstract them to a common interface/method, so that we can reduce the code complexity and redundency.

We can use reflection to get the name filed.

updated. thanks.

@tokers @starsz please review again when you have time.

moonming · 2021-03-17T12:04:49Z

api/test/e2enew/route/route_test.go

+			Method: http.MethodPut,
+			Path:   "/apisix/admin/routes/r2",
+			Body: `{
+                                "name": "route1",


bad indentation

moonming · 2021-03-17T12:05:42Z

api/test/e2enew/route/route_test.go

+			}`,
+			Headers:      map[string]string{"Authorization": base.GetToken()},
+			ExpectStatus: http.StatusBadRequest,
+			ExpectBody:   `route name is existed`,


I don't think the grammar is right

fixed. @moonming

tokers · 2021-03-17T12:06:35Z

api/internal/handler/handler.go

+			case "upstream":
+				objID = obj.(*entity.Upstream).ID
+				objName = obj.(*entity.Upstream).Name
+			}


I think a default arm can be specified here:

default: panic("bad resource") }

fixed. @tokers

moonming

code style is good to me，but I am not check the logic

Jaycean · 2021-03-17T14:15:00Z

api/test/e2enew/route/route_test.go

+			Path:         "/apisix/admin/routes/r2",
+			Headers:      map[string]string{"Authorization": base.GetToken()},
+			ExpectStatus: http.StatusOK,
+		}),


IMHO, we should add test cases that verify the data is deleted after deleting route data.

Jaycean · 2021-03-17T14:16:09Z

api/test/e2enew/route/route_test.go

+		table.Entry("hit route2", base.HttpTestCase{
+			Object:       base.APISIXExpect(),
+			Method:       http.MethodGet,
+			Path:         "/hello_",


The path of route2 is updated to "/hello". This test case cannot be tested "hit route2".

fixed. thanks @Jaycean

Jaycean

LGTM

liuxiran · 2021-03-17T21:45:20Z

Hi @nic-chen , I found that currently, fe called /apisix/admin/notexist/routes to check unique name of route, we can remove calling this api after this pr merged, right?

nic-chen · 2021-03-17T23:55:56Z

Hi @nic-chen , I found that currently, fe called /apisix/admin/notexist/routes to check unique name of route, we can remove calling this api after this pr merged, right?

I think we could keep it. In this way, the user can know whether the name is available on the first step of creating a route.

liuxiran · 2021-03-18T00:25:38Z

Hi @nic-chen , I found that currently, fe called /apisix/admin/notexist/routes to check unique name of route, we can remove calling this api after this pr merged, right?

I think we could keep it. In this way, the user can know whether the name is available on the first step of creating a route.

ok

johzchen added 4 commits March 17, 2021 08:16

feat: check name existed when create or update a resource

4b437db

fix: unit test cases

161b749

test: add e2e test cases

251672a

test: add e2e test cases

8bccd65

nic-chen changed the title ~~fix: check name existed when creating a resource~~ fix: check name existed when creating or updating a resource Mar 17, 2021

johzchen added 4 commits March 17, 2021 12:07

fix: e2e test failed

94269b3

fix: e2e test failed

49968b6

fix: e2e test failed

c934178

fix: e2e test failed

3ed1a67

nic-chen requested review from starsz, imjoey, Jaycean, gxthrj and tokers and removed request for starsz, imjoey and Jaycean March 17, 2021 09:03

tokers reviewed Mar 17, 2021

View reviewed changes

fix: code style

1460dfb

moonming requested changes Mar 17, 2021

View reviewed changes

tokers reviewed Mar 17, 2021

View reviewed changes

johzchen added 2 commits March 17, 2021 21:35

fix style

fc02009

fix: review

c45f7f9

moonming approved these changes Mar 17, 2021

View reviewed changes

Jaycean reviewed Mar 17, 2021

View reviewed changes

fix: review

8d201d3

starsz approved these changes Mar 17, 2021

View reviewed changes

Jaycean approved these changes Mar 17, 2021

View reviewed changes

Merge branch 'master' into name-exist-check

63a798f

liuxiran approved these changes Mar 18, 2021

View reviewed changes

nic-chen merged commit 824c7fc into apache:master Mar 18, 2021

nic-chen deleted the name-exist-check branch March 18, 2021 00:38

nic-chen mentioned this pull request Mar 18, 2021

feat:Support duplicate one existing Route #1558

Merged

5 tasks

Jaycean mentioned this pull request Mar 20, 2021

Discussion: Is the name field required or not when BE create a route #1624

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check name existed when creating or updating a resource #1606

fix: check name existed when creating or updating a resource #1606

nic-chen commented Mar 17, 2021

codecov-io commented Mar 17, 2021 •

edited

Loading

tokers Mar 17, 2021

nic-chen Mar 17, 2021

tokers Mar 17, 2021

starsz Mar 17, 2021

starsz Mar 17, 2021

nic-chen Mar 17, 2021

nic-chen Mar 17, 2021

moonming Mar 17, 2021

nic-chen Mar 17, 2021

moonming Mar 17, 2021

nic-chen Mar 17, 2021

tokers Mar 17, 2021

nic-chen Mar 17, 2021

moonming left a comment

Jaycean Mar 17, 2021

nic-chen Mar 17, 2021

Jaycean Mar 17, 2021

nic-chen Mar 17, 2021

Jaycean left a comment

liuxiran commented Mar 17, 2021

nic-chen commented Mar 17, 2021

liuxiran commented Mar 18, 2021

fix: check name existed when creating or updating a resource #1606

fix: check name existed when creating or updating a resource #1606

Conversation

nic-chen commented Mar 17, 2021

Bugfix

codecov-io commented Mar 17, 2021 • edited Loading

Codecov Report

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

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

moonming left a 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

Jaycean left a comment

Choose a reason for hiding this comment

liuxiran commented Mar 17, 2021

nic-chen commented Mar 17, 2021

liuxiran commented Mar 18, 2021

codecov-io commented Mar 17, 2021 •

edited

Loading