feat(BE): Export route from OpenAPI Specification3.0 #1245

Jaycean · 2021-01-08T08:47:25Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
[Proposal]: Import & Export route from OpenAPI Specification3.0 #825

New feature or improvement

Export route from OpenAPI Specification3.0

Related FE PR: #1240

codecov-io · 2021-01-11T01:10:24Z

Codecov Report

Merging #1245 (0af3fd8) into master (8852e63) will increase coverage by 0.10%.
The diff coverage is 68.67%.

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
+ Coverage   62.75%   62.85%   +0.10%     
==========================================
  Files          41       43       +2     
  Lines        2360     2660     +300     
==========================================
+ Hits         1481     1672     +191     
- Misses        703      783      +80     
- Partials      176      205      +29

Impacted Files	Coverage Δ
api/internal/core/entity/entity.go	`62.50% <ø> (ø)`
api/internal/handler/data_loader/route_export.go	`68.56% <68.56%> (ø)`
api/internal/route.go	`84.37% <100.00%> (+0.50%)`	⬆️
api/cmd/managerapi.go	`60.29% <0.00%> (-16.79%)`	⬇️
api/internal/conf/conf.go	`67.85% <0.00%> (ø)`
api/internal/utils/pid.go	`26.66% <0.00%> (ø)`
api/internal/core/store/store.go	`87.34% <0.00%> (+0.63%)`	⬆️

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 8852e63...0af3fd8. Read the comment docs.

api/internal/handler/route_export/route_export.go

starsz · 2021-01-11T02:24:03Z

Hi, @Jaycean. Need unit test.

Jaycean · 2021-01-11T02:46:56Z

Hi, @Jaycean. Need unit test.

At present, the function interface needs to export the existing route data and ensure the correctness of the exported data, so I think E2E should be written more perfectly. I think if the simple unit test only tests whether the interface can export data, it does not represent whether the function of the interface is correct, and if the relevant route data is not created, the interface will not work normally use.

starsz · 2021-01-11T03:36:46Z

Hi, @Jaycean. Need unit test.

At present, the function interface needs to export the existing route data and ensure the correctness of the exported data, so I think E2E should be written more perfectly. I think if the simple unit test only tests whether the interface can export data, it does not represent whether the function of the interface is correct, and if the relevant route data is not created, the interface will not work normally use.

Not agree with you.
First of all, unit testing is very important. It helps find bugs easily and quickly if someone changes the code.And unit testing can cover more code than e2e test.
For example, in this case.
We need to test these situations at least:

route can't found
Uris contain "*"
plugins with request-validation
different route method
...

and if the relevant route data is not created, the interface will not work normally use.

We can mock data in etcd.
See https://github.com/apache/apisix-dashboard/blob/master/api/internal/handler/server_info/server_info_test.go for more details.

Jaycean · 2021-01-11T05:28:49Z

Hi, @Jaycean. Need unit test.

At present, the function interface needs to export the existing route data and ensure the correctness of the exported data, so I think E2E should be written more perfectly. I think if the simple unit test only tests whether the interface can export data, it does not represent whether the function of the interface is correct, and if the relevant route data is not created, the interface will not work normally use.

Not agree with you.
First of all, unit testing is very important. It helps find bugs easily and quickly if someone changes the code.And unit testing can cover more code than e2e test.
For example, in this case.
We need to test these situations at least:

route can't found

Uris contain "*"

plugins with request-validation

different route method
...

and if the relevant route data is not created, the interface will not work normally use.

We can mock data in etcd.
See https://github.com/apache/apisix-dashboard/blob/master/api/internal/handler/server_info/server_info_test.go for more details.

Thank you for your advice. I also think unit testing is really necessary. I just thought that this interface test is almost the same as E2E, but unit testing should cover all situations as much as possible

api/internal/handler/route_export/route_export.go

api/test/e2e/route_export_test.go

api/internal/handler/route_export/route_export.go

Jaycean · 2021-01-25T04:54:22Z

@liuxiran @nic-chen
The unit test and test cases have been supplemented. In the process of testing, I found a problem：

At present, the path data structure of OpenAPI 3 is used to store URIs, and the data structure is map [string]. If the URI is repeated, the data will be covered, if the repeated URIs are directly covered, only one URI can be exported, which will lead to the loss of user data.
My idea is to add a uniform suffix after the same URI when exporting, so that the import can recognize the URI
For example：

{
	"components": {},
	"info": {
		"title": "RoutesExport",
		"version": "3.0.0"
	},
	"openapi": "3.0.0",
	"paths": {
		"/hello": {
			"get": {}
		}
		"/hello-repeaturi1": {
			"get": {}
		}
		"/hello-repeaturi2": {
			"get": {}
		}
	}
}

My method also has the risk of repeating the user named URI
Do you have any better suggestions? We can discuss them. Thks.

api/internal/handler/data_loader/route_export.go

starsz · 2021-01-25T05:48:56Z

api/internal/handler/data_loader/route_export.go

+	return &swagger, nil
+}
+
+func parseLabels(route *entity.Route, serviceLabels map[string]string, labels map[string]string) (map[string]string, error) {


What's the label args use for?

When service and route have labels at the same time, use route's label. When route has no label, service sometimes uses service's label. This function is used to process this logic

No, I didn't see the lables args was used.

I don't quite understand your question. Do you mean you don't see users or business scenarios using this field? I don't know if my understanding is correct, because more coverage possibilities are considered everywhere to ensure that the data is not lost. After discussing with nic-chen, this processing function is added.

api/internal/handler/data_loader/route_export.go

liuxiran · 2021-01-25T06:46:51Z

@liuxiran @nic-chen
The unit test and test cases have been supplemented. In the process of testing, I found a problem：

At present, the path data structure of OpenAPI 3 is used to store URIs, and the data structure is map [string]. If the URI is repeated, the data will be covered, if the repeated URIs are directly covered, only one URI can be exported, which will lead to the loss of user data.

My idea is to add a uniform suffix after the same URI when exporting, so that the import can recognize the URI
For example：
{
	"components": {},
	"info": {
		"title": "RoutesExport",
		"version": "3.0.0"
	},
	"openapi": "3.0.0",
	"paths": {
		"/hello": {
			"get": {}
		}
		"/hello-repeaturi1": {
			"get": {}
		}
		"/hello-repeaturi2": {
			"get": {}
		}
	}
}
My method also has the risk of repeating the user named URI

Do you have any better suggestions? We can discuss them. Thks.

OpenAPI defines a unique operation as a combination of a path and an HTTP method. This means that two GET or two POST methods for the same path are not allowed – even if they have different parameters (parameters have no effect on uniqueness)
refer from https://swagger.io/docs/specification/paths-and-operations/

After communicating with @Jaycean and @nic-chen there are two alternative solutions:

A: add a uniform suffix after the same URI when exporting, and add a description to the user guide.
This solution can meet the platform-wide data is not lost, the disadvantage is that when users need to use the swagger file to generate documents or sdk, they need to modified manually.
B: return error when exporting routes with same URI
In this way exported OAS3.0 file can be used directly, with the disadvantage that there may be data loss for APISIX. The remedy is that we provide additional import and export of platform-wide data, in accordance with the apisix data format.

Neither scheme is perfect though, which one do you perfer? any suggestions are welcome, thanks a lot

@Jaycean @nic-chen @membphis @starsz @juzhiyuan @imjoey

juzhiyuan · 2021-01-25T07:14:51Z

because it's using URI as paths's Key, then data gets covered would happen. I would prefer adding a global prefix to avoid data lost.

nic-chen · 2021-01-25T07:17:03Z

I vote for option A, at least it can be imported and exported normally on APISIX platform.

juzhiyuan · 2021-01-25T07:25:04Z

not sure how other platforms would resolve those issues, we should have a search.

Jaycean · 2021-01-25T07:31:10Z

In my opinion, from the perspective of apimix platform, I choose scheme a to satisfy the users of apimix platform A. The function that URI can be renamed is more open and friendly for users.

liuxiran · 2021-01-25T08:31:26Z

not sure how other platforms would resolve those issues, we should have a search.

I tried Aliyun API Gateway and eolinker( a API management platform):

In Aliyun API Gateway, when we export multiple APIs, it will split each API into a single file(It's also a good choice)
In eolinker, it does not support export OpenAPI.

As most people choose option A, let's solve the problem with A first, after this feature megerd and released, let's wait for more users feedback ^_^

api/internal/handler/data_loader/route_export.go

api/internal/handler/data_loader/route_export_test.go

… error

Jaycean · 2021-01-27T01:05:57Z

@starsz @nic-chen PTAL. Thks.

liuxiran

this pr has covers all the requirements according to #825

membphis · 2021-01-27T05:17:00Z

wow, Congratulations this PR was merged

Jaycean marked this pull request as draft January 8, 2021 08:47

Jaycean force-pushed the export_route_from_openapi branch from 3093f3c to 7c3b149 Compare January 8, 2021 08:53

Jaycean changed the title ~~Export route from OpenAPI Specification3.0~~ feat: Export route from OpenAPI Specification3.0 Jan 8, 2021

liuxiran mentioned this pull request Jan 8, 2021

feat(fe): import and export routes #1240

Merged

1 task

Jaycean changed the title ~~feat: Export route from OpenAPI Specification3.0~~ feat(BE): Export route from OpenAPI Specification3.0 Jan 8, 2021

liuxiran added this to the 2.4 milestone Jan 8, 2021

imjoey requested changes Jan 11, 2021

View reviewed changes

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

Jaycean force-pushed the export_route_from_openapi branch 2 times, most recently from 74ab4c5 to 0d191cb Compare January 11, 2021 01:24

imjoey reviewed Jan 11, 2021

View reviewed changes

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

liuxiran reviewed Jan 11, 2021

View reviewed changes

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

Jaycean force-pushed the export_route_from_openapi branch from a5d09d1 to 7668938 Compare January 11, 2021 07:39

liuxiran mentioned this pull request Jan 11, 2021

feat: import route from OpenAPI Specification3.0 #1102

Merged

3 tasks

Jaycean marked this pull request as ready for review January 12, 2021 03:18

liuxiran requested review from nic-chen, membphis, moonming and tokers January 12, 2021 03:21

starsz reviewed Jan 12, 2021

View reviewed changes

liuxiran reviewed Jan 13, 2021

View reviewed changes

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

Jaycean force-pushed the export_route_from_openapi branch 4 times, most recently from 62cf091 to b0b3c36 Compare January 14, 2021 07:50

tokers reviewed Jan 14, 2021

View reviewed changes

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

starsz requested changes Jan 25, 2021

View reviewed changes

Jaycean added 3 commits January 25, 2021 16:47

Fix format, add comments, package function of upstream data processing

d80d9e3

avoid use context.Background,use c.Context()

9d0e52e

Add suffixes and test cases when exporting the same URI

7c1b83e

starsz reviewed Jan 26, 2021

View reviewed changes

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

starsz requested changes Jan 26, 2021

View reviewed changes

Jaycean added 8 commits January 26, 2021 22:12

Fix the code format problem, add comments, and fix the unit test JSON…

b0da814

… error

Remove the comment code

8af7769

Modify test case upstream IP

2cb620e

Label variable declaration

fbf404a

Add closure to create pathnumber

934fab6

Update unit tests and E2E test cases

0af3fd8

Modify closure writing, modify test cases

3c16154

Remove the comment code

eef0f5e

starsz approved these changes Jan 27, 2021

View reviewed changes

juzhiyuan requested review from imjoey and tokers January 27, 2021 02:19

liuxiran approved these changes Jan 27, 2021

View reviewed changes

imjoey approved these changes Jan 27, 2021

View reviewed changes

juzhiyuan merged commit 9cb9aa7 into apache:master Jan 27, 2021

Jaycean deleted the export_route_from_openapi branch February 3, 2021 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(BE): Export route from OpenAPI Specification3.0 #1245

feat(BE): Export route from OpenAPI Specification3.0 #1245

Jaycean commented Jan 8, 2021 •

edited

Loading

codecov-io commented Jan 11, 2021 •

edited

Loading

starsz commented Jan 11, 2021

Jaycean commented Jan 11, 2021

starsz commented Jan 11, 2021

Jaycean commented Jan 11, 2021

Jaycean commented Jan 25, 2021 •

edited

Loading

starsz Jan 25, 2021

Jaycean Jan 25, 2021

starsz Jan 26, 2021

Jaycean Jan 26, 2021

liuxiran commented Jan 25, 2021 •

edited

Loading

juzhiyuan commented Jan 25, 2021 •

edited

Loading

nic-chen commented Jan 25, 2021

juzhiyuan commented Jan 25, 2021 •

edited

Loading

Jaycean commented Jan 25, 2021 •

edited

Loading

liuxiran commented Jan 25, 2021

Jaycean commented Jan 27, 2021

liuxiran left a comment

membphis commented Jan 27, 2021

feat(BE): Export route from OpenAPI Specification3.0 #1245

feat(BE): Export route from OpenAPI Specification3.0 #1245

Conversation

Jaycean commented Jan 8, 2021 • edited Loading

New feature or improvement

Related FE PR: #1240

codecov-io commented Jan 11, 2021 • edited Loading

Codecov Report

starsz commented Jan 11, 2021

Jaycean commented Jan 11, 2021

starsz commented Jan 11, 2021

Jaycean commented Jan 11, 2021

Jaycean commented Jan 25, 2021 • edited Loading

starsz Jan 25, 2021

Choose a reason for hiding this comment

Jaycean Jan 25, 2021

Choose a reason for hiding this comment

starsz Jan 26, 2021

Choose a reason for hiding this comment

Jaycean Jan 26, 2021

Choose a reason for hiding this comment

liuxiran commented Jan 25, 2021 • edited Loading

juzhiyuan commented Jan 25, 2021 • edited Loading

nic-chen commented Jan 25, 2021

juzhiyuan commented Jan 25, 2021 • edited Loading

Jaycean commented Jan 25, 2021 • edited Loading

liuxiran commented Jan 25, 2021

Jaycean commented Jan 27, 2021

liuxiran left a comment

Choose a reason for hiding this comment

membphis commented Jan 27, 2021

Jaycean commented Jan 8, 2021 •

edited

Loading

codecov-io commented Jan 11, 2021 •

edited

Loading

Jaycean commented Jan 25, 2021 •

edited

Loading

liuxiran commented Jan 25, 2021 •

edited

Loading

juzhiyuan commented Jan 25, 2021 •

edited

Loading

juzhiyuan commented Jan 25, 2021 •

edited

Loading

Jaycean commented Jan 25, 2021 •

edited

Loading