fix: unable to export route with nil methods field #1673

Jaycean · 2021-03-26T03:15:22Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

What changes will this PR take into?

Fixed the bug that when creating route, the methods field is nil or [] and the data cannot be exported.
When there is no plugin data in the route, the empty field is not exported.

Related issues

fix/resolve #1668

Checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

codecov-io · 2021-03-26T09:16:53Z

Codecov Report

Merging #1673 (0fb644b) into master (c6c5d14) will decrease coverage by 10.24%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #1673       +/-   ##
===========================================
- Coverage   72.59%   62.34%   -10.25%     
===========================================
  Files         133       47       -86     
  Lines        5728     3126     -2602     
  Branches      666        0      -666     
===========================================
- Hits         4158     1949     -2209     
+ Misses       1327      865      -462     
- Partials      243      312       +69

Flag	Coverage Δ
backend-e2e-test	`62.34% <100.00%> (+0.21%)`	⬆️
backend-e2e-test-ginkgo	`48.36% <0.00%> (+0.10%)`	⬆️
backend-unit-test	`?`
frontend-e2e-test	`?`

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

Impacted Files	Coverage Δ
api/internal/handler/data_loader/route_export.go	`72.46% <100.00%> (+3.29%)`	⬆️
api/internal/utils/runtime/runtime.go	`0.00% <0.00%> (-64.29%)`	⬇️
api/internal/core/store/validate_mock.go	`0.00% <0.00%> (-50.00%)`	⬇️
api/internal/filter/authentication.go	`47.22% <0.00%> (-30.56%)`	⬇️
api/internal/handler/service/service.go	`62.60% <0.00%> (-29.57%)`	⬇️
api/internal/core/store/store.go	`59.63% <0.00%> (-27.11%)`	⬇️
api/internal/filter/ip_filter.go	`48.71% <0.00%> (-23.08%)`	⬇️
api/internal/handler/global_rule/global_rule.go	`64.51% <0.00%> (-19.36%)`	⬇️
...pi/internal/handler/plugin_config/plugin_config.go	`59.57% <0.00%> (-18.09%)`	⬇️
api/internal/utils/json_patch.go	`44.82% <0.00%> (-13.80%)`	⬇️
... and 101 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 c6c5d14...0fb644b. Read the comment docs.

tokers · 2021-03-27T07:36:15Z

api/internal/handler/data_loader/route_export.go

+		if route.Methods != nil && len(route.Methods) > 0 {
+			routeMethods = route.Methods
+		} else {
+			routeMethods = []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete, http.MethodPatch, http.MethodHead, http.MethodConnect, http.MethodTrace, http.MethodOptions}


Can we use a pre-defined unexportable variable to indicate this default method set?

Yeah,Because these methods such as "GET", "POST" and so on are immutable, the use here is more uniform, and "Get", "get" and "GET" will not appear.
They don't affect the output.

I mean creating a variable _allHTTPMethods, which is:

var ( _allHTTPMethods = []string{http.MethodGet, http.MethodPost, ...} )

Sorry, I misunderstood.
Done. Thks

starsz · 2021-03-28T02:49:12Z

web/cypress/fixtures/export-route-dataset.json

@@ -131,7 +129,6 @@
            }
          },
          "x-apisix-enable_websocket": false,
-          "x-apisix-plugins": {},


Just want to know why delete it?

The design of import and export is to export when there is data, and not export when there is no data. Here, no data is empty. I don't think it's necessary to export, so I delete it.

If it is not deleted, Fe ci will report an error.

@Jaycean do we have some test case for x-apisix-plugins not empty ?

Yes, most of the other test cases have data in x-apisix-plugins
For example, this test case:

apisix-dashboard/api/internal/handler/data_loader/route_export_test.go

Line 34 in 73e9635

func TestExportRoutes1(t *testing.T) {

Thank you for the explanation.Got it.

starsz · 2021-03-28T02:51:50Z

api/test/e2e/route_export_test.go

+							"172.16.238.20:1980": 1
+						},
+						"type": "roundrobin"
+					}


They are the same. Can we use a variable instead?

"requestBody": {}, "responses": { "default": { "description": "" } }, "x-apisix-enable_websocket": false, "x-apisix-hosts": ["test.com"], "x-apisix-priority": 0, "x-apisix-status": 1, "x-apisix-upstream": { "nodes": { "172.16.238.20:1980": 1 }, "type": "roundrobin" }

Here, I intend to modify it uniformly when using ginkgo rewriting.

…hod set

api/internal/handler/data_loader/route_export.go

Jaycean added 2 commits March 26, 2021 11:11

fix: unable to export route with nil methods field

6dd8578

fix ci failed

0fb644b

juzhiyuan requested a review from starsz March 26, 2021 05:07

Jaycean requested a review from nic-chen March 26, 2021 09:41

fix fe ci failed

73e9635

Jaycean requested review from gxthrj, imjoey and tokers March 26, 2021 16:02

tokers reviewed Mar 27, 2021

View reviewed changes

starsz reviewed Mar 28, 2021

View reviewed changes

use a pre-defined unexportable variable to indicate this default met…

f81911c

…hod set

tokers approved these changes Mar 29, 2021

View reviewed changes

juzhiyuan requested a review from starsz March 29, 2021 15:12

nic-chen reviewed Mar 30, 2021

View reviewed changes

api/internal/handler/data_loader/route_export.go Show resolved Hide resolved

nic-chen approved these changes Mar 30, 2021

View reviewed changes

starsz approved these changes Mar 30, 2021

View reviewed changes

juzhiyuan approved these changes Mar 30, 2021

View reviewed changes

juzhiyuan merged commit 37f4cd8 into apache:master Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unable to export route with nil methods field #1673

fix: unable to export route with nil methods field #1673

Jaycean commented Mar 26, 2021

codecov-io commented Mar 26, 2021 •

edited

Loading

tokers Mar 27, 2021

Jaycean Mar 27, 2021 •

edited

Loading

tokers Mar 29, 2021

Jaycean Mar 29, 2021

starsz Mar 28, 2021

Jaycean Mar 28, 2021

Jaycean Mar 28, 2021

nic-chen Mar 28, 2021

Jaycean Mar 28, 2021

starsz Mar 29, 2021

starsz Mar 28, 2021

Jaycean Mar 28, 2021

starsz Mar 29, 2021

fix: unable to export route with nil methods field #1673

fix: unable to export route with nil methods field #1673

Conversation

Jaycean commented Mar 26, 2021

codecov-io commented Mar 26, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Jaycean Mar 27, 2021 • edited Loading

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

codecov-io commented Mar 26, 2021 •

edited

Loading

Jaycean Mar 27, 2021 •

edited

Loading