feat: Add OpenAPI route #1960
feat: Add OpenAPI route #1960
Conversation
nasdf
added
documentation
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.
|
| "net/http" | ||
|
|
||
| "github.com/getkin/kin-openapi/openapi3" | ||
| "github.com/getkin/kin-openapi/openapi3gen" |
There was a problem hiding this comment.
question: Did you have a good shop around for alternatives? What made you pick this library?
There was a problem hiding this comment.
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.
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.
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.
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. |
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: