fix(import routes): merge route when route have the same name

[kevinw66]

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?

Please update this section with detailed description.

Related issues

fixes #2329

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-commenter]

Codecov Report

Merging #2330 (694da8f) into master (03a6cc1) will decrease coverage by 3.02%.
The diff coverage is 59.45%.

@@            Coverage Diff             @@
##           master    #2330      +/-   ##
==========================================
- Coverage   68.10%   65.08%   -3.03%     
==========================================
  Files         127      188      +61     
  Lines        3374     7358    +3984     
  Branches      830      828       -2     
==========================================
+ Hits         2298     4789    +2491     
- Misses       1076     2279    +1203     
- Partials        0      290     +290     

Flag	Coverage Δ
backend-e2e-test	37.38% <60.60%> (?)
backend-unit-test	48.73% <0.00%> (?)
frontend-e2e-test	68.57% <50.00%> (+0.46%)	⬆️

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

Impacted Files	Coverage Δ
web/src/components/Plugin/UI/limit-req.tsx	100.00% <ø> (ø)
web/src/pages/Route/List.tsx	83.03% <50.00%> (+0.19%)	⬆️
api/internal/handler/data_loader/route_import.go	66.55% <56.52%> (ø)
api/internal/utils/utils.go	68.51% <62.50%> (ø)
api/internal/handler/data_loader/route_export.go	75.17% <100.00%> (ø)
...omponents/Upstream/components/UpstreamSelector.tsx	85.71% <0.00%> (-14.29%)	⬇️
web/src/components/Upstream/UpstreamForm.tsx	90.12% <0.00%> (-2.29%)	⬇️
web/src/pages/PluginTemplate/List.tsx	71.42% <0.00%> (-1.08%)	⬇️
web/src/pages/Service/List.tsx	94.11% <0.00%> (-0.76%)	⬇️
web/src/pages/Consumer/Create.tsx	82.69% <0.00%> (ø)
... and 70 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 fc62ca3...694da8f. Read the comment docs.

[bzp2010]

Hi, I'm not sure if it's appropriate to merge based on name, as far as I understand it, ID should be the basis for merging, and sometimes even when there is overlapping content, no data should be merged.

[kevinw66]

Hi, I'm not sure if it's appropriate to merge based on name, as far as I understand it, ID should be the basis for merging, and sometimes even when there is overlapping content, no data should be merged.

@bzp2010 When creating a new route, the name cannot be the same, or else it will throw an error.
So I merge routes based on name, Because duplicated name was treated as dirty data.

[zaunist]

Hi, I'm not sure if it's appropriate to merge based on name, as far as I understand it, ID should be the basis for merging, and sometimes even when there is overlapping content, no data should be merged.

@bzp2010 When creating a new route, the name cannot be the same, or else it will throw an error. So I merge routes based on name, Because duplicated name was treated as dirty data.

Could we use based on ID ? I think ID is better.

[Baoyuantop]

Agree with ID

[kevinw66]

If we merge based on id, the bug described in issue cannot be fixed, the route with same name and different path value already assigned different id
unless we cache id with route name, and if we meet same route name, we use the same id.
but what's the different between this and merge directly with name ?

[jwrookie]

If we merge based on id, the bug described in issue cannot be fixed, the route with same name and different path value already assigned different id unless we cache id with route name, and if we meet same route name, we use the same id. but what's the different between this and merge directly with name ?

I think both import and export should be done through ID

[kevinw66]

@bzp2010 @zaunist @Baoyuantop What do you think?

[Baoyuantop]

Hi @kevinw66, maybe we can discuss this with more people in the community mail list, here is the subscription method: https://apisix.apache.org/docs/general/community#get-help-from-mailing-list-recommended

[zaunist]

Hi @kevinw66, maybe we can discuss this with more people in the community mail list, here is the subscription method: https://apisix.apache.org/docs/general/community#get-help-from-mailing-list-recommended

Agree. We need to discuss with others.

[nic-chen]

@kevinw66 hi
need some test cases for the change

[nic-chen]

FE test failed. @Baoyuantop @guoqqqi @bzp2010 please help when you have time, thanks.

[guoqqqi]

Hi, @kevinw66 looks like a new id field has been remove? We need to update the web/cypress/fixtures/export-route-dataset.json file

[kevinw66]

Hi, @kevinw66 looks like a new id field has been remove? We need to update the web/cypress/fixtures/export-route-dataset.json file

Is this id a fixed value?

[guoqqqi]

As the ID is not a fixed value, we need to do some processing. We need to remove the id field from the fileContent.
https://github.com/apache/apisix-dashboard/blob/master/web/cypress/integration/route/import_export_route.spec.js#L134
If you need help please leave me a message 😄

[kevinw66]

As the ID is not a fixed value, we need to do some processing. We need to remove the id field from the fileContent. https://github.com/apache/apisix-dashboard/blob/master/web/cypress/integration/route/import_export_route.spec.js#L134 If you need help please leave me a message 😄

Of course, It's hard for me to change frontend code 😢

[guoqqqi]

cc @oil-oil can you help to fix the E2E error?