-
Notifications
You must be signed in to change notification settings - Fork 44
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: Schema list API #1625
feat: Schema list API #1625
Conversation
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1625 +/- ##
===========================================
- Coverage 76.15% 75.94% -0.21%
===========================================
Files 192 193 +1
Lines 19852 19964 +112
===========================================
+ Hits 15118 15161 +43
- Misses 3707 3768 +61
- Partials 1027 1035 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 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.
This is an nice functionality. I like the improvements from what you had for it on the playground PR :)
I do have a few things I'd like to address as you'll see bellow. Nothing major though and once they're resolved we should be good to merge.
Fields []fieldResponse `json:"fields,omitempty"` | ||
} | ||
|
||
func listSchemaHandler(rw http.ResponseWriter, req *http.Request) { |
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.
todo: I would be really nice to have a unit test for this handler to see what the expected output is and that we catch future changes to the schema structure. Ideally the error paths would also be tested. You can base yourself off of the other unit test for 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.
Good call. I added tests for the error path as well as the happy path.
api/http/handlerfuncs.go
Outdated
sendJSON( | ||
req.Context(), | ||
rw, | ||
simpleDataResponse("result", "success", "collections", colResp), |
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: You can probably remove the result
and success
for the response as those are superfluous in this case.
api/http/router.go
Outdated
@@ -30,6 +30,7 @@ const ( | |||
DumpPath string = versionedAPIPath + "/debug/dump" | |||
BlocksPath string = versionedAPIPath + "/blocks" | |||
GraphQLPath string = versionedAPIPath + "/graphql" | |||
SchemaListPath string = versionedAPIPath + "/schema/list" |
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 started changing out paths to be a bit more RESTful with the introduction of the index
handlers. If you don't mind, please convert the 3 schema paths to a single SchemaPath string = versionedAPIPath + "/schema"
and use the relevant http verbs in the setRoutes
method bellow (Get, Post, Patch).
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 simplified the paths like you mentioned. Do we need to make an announcement when changing the API?
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.
At this stage I don't think we do as most users probably use the cli for this but the python sdk might need to be updated.
cli/schema_list.go
Outdated
func MakeSchemaListCommand(cfg *config.Config) *cobra.Command { | ||
var cmd = &cobra.Command{ | ||
Use: "list", | ||
Short: "List schema types from DefraDB", |
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: List schema types with their respective fields
. No a strong opinion but I though it would be a bit more descriptive of what it does.
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.
Good job Keenan :) Thanks for the changes.
bug bash result: |
## Relevant issue(s) Resolves sourcenetwork#1624 ## Description This PR adds an API for listing schemas. The schema API routes have also been simplified to be more RESTful. ### HTTP `curl http://localhost:9181/api/v0/schema` #### Output ``` { "data": { "collections": [ { "name": "User", "id": "bafkreibpnvkvjqvg4skzlijka5xe63zeu74ivcjwd76q7yi65jdhwqhske", "fields": [ { "id": "1", "name": "age", "kind": "Int" }, { "id": "2", "name": "name", "kind": "String" }, { "id": "3", "name": "points", "kind": "Float" }, { "id": "4", "name": "verified", "kind": "Boolean" } ] } ], "result": "success" } } ``` ### CLI `defradb client schema list` #### Output ``` type User { name: String age: Int verified: Boolean points: Float } ``` ## 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? Automated test + manual testing Specify the platform(s) on which this was tested: - MacOS
Relevant issue(s)
Resolves #1624
Description
This PR adds an API for listing schemas.
HTTP
curl http://localhost:9181/api/v0/schema/list
Output
CLI
defradb client schema list
Output
Tasks
How has this been tested?
Manual testing
Specify the platform(s) on which this was tested: