-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add OpenAPI route #1960
feat: Add OpenAPI route #1960
Conversation
Codecov ReportAttention:
@@ Coverage Diff @@
## develop #1960 +/- ##
===========================================
+ Coverage 74.13% 74.86% +0.73%
===========================================
Files 244 246 +2
Lines 23508 24298 +790
===========================================
+ Hits 17427 18189 +762
- Misses 4880 4899 +19
- Partials 1201 1210 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I have a couple of questions. Please let me know if there is any rush on this one (otherwise it looks like it will cause no trouble if it sits here for a couple of days)
"net/http" | ||
|
||
"github.com/getkin/kin-openapi/openapi3" | ||
"github.com/getkin/kin-openapi/openapi3gen" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Did you have a good shop around for alternatives? What made you pick this library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the go-swagger
library as well, but the documentation was lacking. There are a few things I liked about this library.
- supports openapi 3 (go-swagger is on 2)
- struct schema generation works very well
- request & response validation for testing
- type safety when writing the spec in go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :) Thanks for listing those Keenan :) v3 vs v2 is probably reason enough to use this lib (and not just for the support, but I see it as a sign of better lib health)
http/openapi.go
Outdated
}, | ||
} | ||
|
||
func init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We will now have 3 different files for doing this, cli.go
, handler.go
, and openapi.go
. Working with any one of them in isolation is error prone, working with all of them distributed as they currently are is even more so. It also feels quite important to me that they all follow the same structure (as much so as is reasonable).
Could you perhaps spend some time now looking at pulling these 3 files out to their own shared package? Called api
or something. And provide us with a single location in which we have to maintain these?
Longer term (I don't want to pressure you now) we could perhaps unify them further so that instead of splitting the functions in that new package by client, we could instead split them by function, for example (very roughy):
func BindTxCreate() {
// setup cli
MakeTxCreateCommand(cfg)
// setup http
tx.Post("/", tx_handler.NewTxn)
// setup openapi
txnCreate := openapi3.NewOperation()
txnCreate.AddParameter(txnReadOnlyQueryParam)
txnCreate.AddResponse(200, txnCreateResponse)
txnCreate.AddResponse(400, errorResponse)
}
func BindPatchSchema() {
... etc
This would also hopefully further open up the possibility of more magic refactorings in the future, that may remove having to manually map it 3 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've found a solution that will make the http routes simpler. I've moved the route registration to bind functions which now require OpenAPI definitions for each route. This should help enforce that all endpoints have basic descriptions. Validating client requests in the tests will be the final step to enforce the correctness of the definitions.
question: I am assuming that the validate call doesn't actually assert that the http endpoints conform to the given specification, do you have any plans for adding tests to make sure the two don't diverge? |
The specification validation only checks if the required fields exist. The test functions in the library can check if the paths and params are valid. I plan on getting those working shortly after. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I look forward to seeing the generation of SDKs with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool stuff, Cheers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :) Glad you managed to unify the openAPI and http bindings :)
Resolves sourcenetwork#510 This PR adds an HTTP endpoint that returns an OpenAPI specification for DefraDB. The definitions are part code generation and part hand written. This should work well for adding examples and more documentation in the future. - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... View endpoint in browser `localhost:9181/api/v0/openapi`. Double checked with OpenAPI validator tool. Specify the platform(s) on which this was tested: - MacOS
## Relevant issue(s) Resolves sourcenetwork#510 ## Description This PR adds an HTTP endpoint that returns an OpenAPI specification for DefraDB. The definitions are part code generation and part hand written. This should work well for adding examples and more documentation in the future. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? View endpoint in browser `localhost:9181/api/v0/openapi`. Double checked with OpenAPI validator tool. Specify the platform(s) on which this was tested: - MacOS
Relevant issue(s)
Resolves #510
Description
This PR adds an HTTP endpoint that returns an OpenAPI specification for DefraDB. The definitions are part code generation and part hand written. This should work well for adding examples and more documentation in the future.
Tasks
How has this been tested?
View endpoint in browser
localhost:9181/api/v0/openapi
.Double checked with OpenAPI validator tool.
Specify the platform(s) on which this was tested: