refactor: OpenAPI 3 parse and convert #2460

bzp2010 · 2022-05-30T09:32:37Z

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?

Improved backend implementation of OpenAPI3 import to enable import of standard OpenAPI3 minimum use cases. In the current PR I will not do the integration work, this separate module will be checked by way of unit tests and in the next PR it will be integrated into the data import interface.

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 · 2022-06-01T04:35:19Z

Codecov Report

Merging #2460 (9d3ffd1) into master (5e624a5) will decrease coverage by 0.15%.
The diff coverage is 75.86%.

@@            Coverage Diff             @@
##           master    #2460      +/-   ##
==========================================
- Coverage   68.98%   68.82%   -0.16%     
==========================================
  Files         131      191      +60     
  Lines        3456     7547    +4091     
  Branches      845      842       -3     
==========================================
+ Hits         2384     5194    +2810     
- Misses       1072     2048     +976     
- Partials        0      305     +305

Flag	Coverage Δ
backend-e2e-test-ginkgo	`59.87% <ø> (?)`
backend-unit-test	`50.34% <75.86%> (?)`
frontend-e2e-test	`68.90% <ø> (-0.09%)`	⬇️

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

Impacted Files	Coverage Δ
...rnal/handler/data_loader/loader/openapi3/export.go	`0.00% <0.00%> (ø)`
...rnal/handler/data_loader/loader/openapi3/import.go	`78.57% <78.57%> (ø)`
web/src/pages/Consumer/Create.tsx	`76.92% <0.00%> (-5.77%)`	⬇️
web/src/pages/Consumer/service.ts	`90.00% <0.00%> (-2.31%)`	⬇️
web/src/pages/Consumer/List.tsx	`91.66% <0.00%> (ø)`
web/src/components/RawDataEditor/RawDataEditor.tsx	`38.96% <0.00%> (ø)`
api/internal/handler/migrate/migrate.go	`59.67% <0.00%> (ø)`
api/internal/filter/cors.go	`38.88% <0.00%> (ø)`
api/cmd/version.go	`57.14% <0.00%> (ø)`
api/internal/utils/runtime/runtime.go	`66.66% <0.00%> (ø)`
... and 56 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 5e624a5...9d3ffd1. Read the comment docs.

starsz

Do we have any proposal on this PR?

bzp2010 · 2022-06-06T03:55:57Z

Do we have any proposal on this PR?

@starsz Yes, I was created a proposal for these works, you can see it here. It contains current problems and current solutions, from which you can learn why some of the content in this PR is implemented in this way and facilitate your comments.

#2465

starsz · 2022-06-06T07:32:23Z

api/internal/handler/data_loader/loader/openapi3/import.go

+		// replace parameter in uri to wildcard
+		realUri := regURIVar.ReplaceAllString(uri, "*")
+		// generate route name
+		routeID := o.TaskName + "_" + strings.NewReplacer("/", "-", "{", "", "}", "").Replace(strings.TrimPrefix(uri, "/"))


Why not use base64 or hash to do this?

Oh, Thanks for your reminder.

What do you think about using the base64 or hashed content as ID and the original uri as Name?

Maybe base64 is better. That's more easily to debug (we can decode from the id to get the info)

After testing, using URI + base64 will generate some very long IDs, which may exceed the schema check limit of APISIX on the one hand, and confuse the format of ID display in the route list on the other. The symbols in base64 will also trigger the schema ID check of APISIX, while for Name only the length is required and the content is completely customizable.

So I took the following approach, instead of generating IDs, which are automatically generated by the dashboard storage layer, we store the information in the route name, which has this format TaskName_URI[_Method]. For a route with a URI of /consumer/{cid}, the method is GET and POST, it generates this TaskName_consumer/{cid}[_GET/POST]

taskName: test methodMerge: true Method + URI: GET|POST /consumer/{cid} => test_consumer/{cid} methodMerge: false Method + URI: GET|POST /consumer/{cid} => test_consumer/{cid}_GET + test_consumer/{cid}_POST

api/internal/handler/data_loader/loader/openapi3/import.go

starsz · 2022-06-06T07:38:43Z

api/internal/handler/data_loader/loader/openapi3/import.go

+		},
+	}
+
+	u, err := url.Parse(servers[0].URL)


Only parse servers[0] ?

This is based on the following considerations:

The first servers are usually the node that is logically "used first".

Different servers may have different global paths

What's "global paths"? servers: - url: https://z.com/test/ - url: https://x.com/non-test/ - url: https://c.com/test-now/ According to the principle of APISIX, we need to set /test, /non-test, /test-now, respectively, as the prefix of the route uri of APISIX, for example, set the original /user/* to /test/user/*, at this point we can't operate.

Do you have any better suggestions?

Create different routes for each server?

I don't think this is appropriate, it generates a lot of useless routes. 🤔

Why the other routes are useless?

hi @bzp2010, please check this, it affects the overall convert

I am a little confused about servers, should they be the URL exposed by the gateway or the URL of the API backend service, please confirm, thanks.

@nic-chen We can look at it this way, during API debugging it is the address actually requested by the debugger, so it should be the address provided by APISIX to the outside world, and the original value should be upstream.
So in effect, the debugger requests the open API of APISIX, while APISIX goes and requests the SERVERS in the original values.

我们可以这样看待这个问题，在API调试过程中，它是调试器实际请求的地址，因此它应当是APISIX对外提供的地址，而原始值应该是上游的。
因此实际上，调试器请求APISIX开放的API，而APISIX去请求原始值中的servers。

@bzp2010 I suggest that this PR not deal with servers first, and deal with it later if there is a strong demand; instead of dealing with imperfections now.

One of the choices to make here is whether to use servers as route.host or upstream.node, which is really critical.

@nic-chen Well, I decided to remove this feature and replace it with a default empty upstream that the user can simply modify to change the upstream associated with all routes.

bzp2010 · 2022-06-10T09:15:19Z

Update

Route IDs are no longer used (now generated by Dashboard itself)
The route name generation algorithm has been updated and it now uses the following rule

TaskName_consumer/{cid}[_GET/POST]

taskName: test

methodMerge: true
Method + URI:
GET|POST /consumer/{cid} => test_consumer/{cid}

methodMerge: false
Method + URI:
GET|POST /consumer/{cid} => test_consumer/{cid}_GET  +  test_consumer/{cid}_POST

Request for Comments

In response to this, I would like to seek more suggestions and I believe there are the following needs met:

Total length will not be long
Try not to have symbols other than _, such as /{}, etc.
Can contain URIs and methods
No dependency on specific fields of OpenAPI (as none of them may exist)

cc @nic-chen @starsz @tokers @tao12345666333

nic-chen · 2022-06-13T08:20:14Z

will review it later.

starsz · 2022-06-13T14:50:16Z

api/internal/handler/data_loader/loader/openapi3/import_test.go

+	// Route
+	assert.Equal(t, data.Upstreams[0].ID, data.Routes[0].UpstreamID)
+	for _, route := range data.Routes {
+		fmt.Println(route.Name)


Debug line?

Yes, removed

starsz · 2022-06-13T14:53:06Z

api/internal/handler/data_loader/loader/openapi3/import.go

+		// temporarily save the parsed data
+		data = &loader.DataSets{}
+		// global upstream ID
+		globalUpstreamID = o.TaskName


Use the taskName as upstream ID ?
It looks strange.

The taskname can be set to a service name, such as user or order, so that it will generate an upstream named order and generate a route name similar to order_user/list. I think this is clearer.

tokers · 2022-06-20T09:47:46Z

api/internal/handler/data_loader/loader/openapi3/import.go

+
+	d, ok := input.([]byte)
+	if !ok {
+		return nil, fmt.Errorf("input format error: expected []byte but it is %s", reflect.TypeOf(input).Kind().String())


Basically, it's a sort of programming faults so I would suggest using panic here.

@tokers 🤔

For the input data error, should we classify it as a 400 Bad Request instead of a 5xx server-related error? I don't think this should be a programming error on the server.
From a practical point of view, if we use panic here, the Manager API will simply treat it as a 500 Internal Server Error and not automatically select the appropriate response code or have us specify it.

For the input data error, should we classify it as a 400 Bad Request instead of a 5xx server-related error?

Is this an API and will be exposed to the client? If so, you need to use some standards to make sure the data type is correct, such as jsonschema. If not, it means this method will be called by another internal method, and it's that handler's responsibility to make sure it accepts correct data type from the client and pass them to this method. As a result, when this method accepts bad data type, it could be classified to the programming fault.

Is this an API and will be exposed to the client? If so, you need to use some standards to make sure the data type is correct, such as jsonschema. If not, it means this method will be called by another internal method, and it's that handler's responsibility to make sure it accepts correct data type from the client and pass them to this method. As a result, when this method accepts bad data type, it could be classified to the programming fault.

Get it, I will change it to panic and move the conversion check logic to the handler.

updated the input error to panic

tokers · 2022-06-20T09:48:49Z

api/internal/handler/data_loader/loader/openapi3/import.go

+
+	// no paths in OAS3 document
+	if len(swagger.Paths) <= 0 {
+		return nil, consts.ErrImportFile


This error message is not enough.

Use errors.Wrap to carry the context message. See https://github.com/pkg/errors.

Yes, it is not enough, I will add more error messages.

error context improved

tokers · 2022-06-20T09:53:23Z

api/internal/handler/data_loader/loader/openapi3/import.go

+		},
+	}
+
+	u, err := url.Parse(servers[0].URL)


It should be the URL exposed by the gateway I guess.

tokers · 2022-06-20T09:54:30Z

api/internal/handler/data_loader/loader/openapi3/import_test.go

+			assert.Contains(t, route.Methods, "DELETE")
+			assert.Equal(t, "Remove customer", route.Desc)
+			assert.Equal(t, entity.Status(0), route.Status)
+		}


Should add the default arm and run t.Fail since a route with bad name exists.

default branch added

api/internal/handler/data_loader/loader/openapi3/openapi3.go

api/internal/handler/data_loader/loader/openapi3/import.go

api/internal/handler/data_loader/loader/openapi3/import_test.go

starsz · 2022-06-20T14:01:07Z

api/test/testdata/import/Postman-API101.yaml

+    put:
+      tags:
+        - default
+      summary: Update customer


Do we support parse summary ?

@starsz After confirmation, they are written to the desc field of the route, which is now supported.

tao12345666333 · 2022-06-21T00:17:23Z

api/internal/handler/data_loader/loader/openapi3/import.go

+		// replace parameter in uri to wildcard
+		realUri := regURIVar.ReplaceAllString(uri, "*")
+		// generate route Name
+		routeName := o.TaskName + "_" + strings.TrimPrefix(uri, "/")


I think we can follow APISIX's schema to check resources.

@tao12345666333 Yes, the ability to check the schema is already built into the storage layer, so no matter what APISIX resource we try to create, it will perform the schema check.

BTW, I'm still not sure on the route name generation and can see from #2460 (comment), do you have any suggestions?

bzp2010 added 3 commits May 30, 2022 17:26

feat: implement DataLoader interface

5b1c65b

feat: enhance import

7b6985d

test: add API 101 cases

8ac880d

bzp2010 added 7 commits June 1, 2022 12:41

chore: add license

2c3d930

fix: lint

ed2aed1

fix: code style

8f488df

feat: add authentication plugin mapping

2658bf2

feat: add apikey plugin support

2e8a99c

test: add apikey plugin cases

6747450

chore: optimize code

a4adf65

bzp2010 self-assigned this Jun 2, 2022

bzp2010 requested review from starsz, tao12345666333 and tokers June 2, 2022 07:52

bzp2010 marked this pull request as ready for review June 2, 2022 07:52

starsz reviewed Jun 2, 2022

View reviewed changes

bzp2010 requested a review from starsz June 6, 2022 03:57

bzp2010 changed the title ~~refactor: OpenAPI 3 import~~ refactor: OpenAPI 3 parse and convert Jun 6, 2022

starsz reviewed Jun 6, 2022

View reviewed changes

bzp2010 requested a review from starsz June 7, 2022 02:09

feat: update route name generator rule

4834643

bzp2010 requested a review from nic-chen June 10, 2022 09:17

starsz reviewed Jun 13, 2022

View reviewed changes

chore: remove debug line

88f5d21

bzp2010 requested a review from starsz June 14, 2022 04:05

bzp2010 mentioned this pull request Jun 14, 2022

feat: migrate old OpenAPI implementation to Loader interface #2468

Open

8 tasks

bzp2010 mentioned this pull request Jun 20, 2022

openapi import will panic when requestbody is not empty but with empty content #2477

Closed

tokers reviewed Jun 20, 2022

View reviewed changes

nic-6443 reviewed Jun 20, 2022

View reviewed changes

api/internal/handler/data_loader/loader/openapi3/openapi3.go Outdated Show resolved Hide resolved

nic-6443 reviewed Jun 20, 2022

View reviewed changes

api/internal/handler/data_loader/loader/openapi3/import.go Show resolved Hide resolved

nic-6443 reviewed Jun 20, 2022

View reviewed changes

api/internal/handler/data_loader/loader/openapi3/import_test.go Show resolved Hide resolved

starsz reviewed Jun 20, 2022

View reviewed changes

tao12345666333 reviewed Jun 21, 2022

View reviewed changes

bzp2010 added 4 commits June 21, 2022 10:14

feat: add error warp for more context

574bf6c

test: add default branch for avoid bad route name

beaceb5

chore: remove plugin mapping

3e3cada

chore: move program fault error to panic

bd71046

bzp2010 requested review from starsz, nic-6443, tokers and tao12345666333 June 21, 2022 04:16

bzp2010 added 2 commits June 21, 2022 12:25

test: fix import test case

06144b5

chore: simplify uri var regexp

9a10f64

nic-6443 approved these changes Jun 21, 2022

View reviewed changes

bzp2010 added 2 commits June 21, 2022 16:09

feat: remove servers field feature

c879a13

test: remove servers field cases

9d3ffd1

nic-chen approved these changes Jun 21, 2022

View reviewed changes

bzp2010 requested review from Baoyuantop, guoqqqi, juzhiyuan and nic-chen June 21, 2022 09:22

SkyeYoung approved these changes Jun 21, 2022

View reviewed changes

nic-chen approved these changes Jun 21, 2022

View reviewed changes

juzhiyuan approved these changes Jun 21, 2022

View reviewed changes

juzhiyuan merged commit 360ef07 into apache:master Jun 21, 2022

bzp2010 mentioned this pull request Jun 22, 2022

feat: integrate data loader interface to import #2474

Merged

8 tasks

refactor: OpenAPI 3 parse and convert #2460

refactor: OpenAPI 3 parse and convert #2460

Conversation

bzp2010 commented May 30, 2022 • edited Loading

codecov-commenter commented Jun 1, 2022 • edited Loading

Codecov Report

starsz left a comment

Choose a reason for hiding this comment

bzp2010 commented Jun 6, 2022 • 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

bzp2010 Jun 6, 2022 • 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

bzp2010 Jun 21, 2022 • 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

bzp2010 commented Jun 10, 2022 • edited Loading

Update

Request for Comments

nic-chen commented Jun 13, 2022

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

This comment was marked as outdated.

bzp2010 Jun 21, 2022 • 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

This comment was marked as outdated.

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

bzp2010 commented May 30, 2022 •

edited

Loading

codecov-commenter commented Jun 1, 2022 •

edited

Loading

bzp2010 commented Jun 6, 2022 •

edited

Loading

bzp2010 Jun 6, 2022 •

edited

Loading

bzp2010 Jun 21, 2022 •

edited

Loading

bzp2010 commented Jun 10, 2022 •

edited

Loading

bzp2010 Jun 21, 2022 •

edited

Loading