Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add collection response information on creation #1499

Merged
merged 5 commits into from
May 16, 2023
Merged

feat: Add collection response information on creation #1499

merged 5 commits into from
May 16, 2023

Conversation

jsimnz
Copy link
Member

@jsimnz jsimnz commented May 12, 2023 •
edited

Relevant issue(s)

Resolves #1498

Description

Updates the response of the CLI, HTTP, and client APIs to include collection information on schema add/creation.

Internally this is to just return the newly made collection descriptions. Externally this updates the HTTP API to include a collections key with an array of objects that include name and id values.

Coorresponding CLI changes to read new HTTP response state was also made.

I don't know if we want this to land for 0.5.1 as in my opinion its technically a feature, more than a refactor, and we're aiming to not have features in the 0.X.1 releases. I'm happy either way.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • 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).
  • 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?

Manually via the CLI and HTTP, CI

Specify the platform(s) on which this was tested:

  • Ubuntu (WSL2)

@jsimnz jsimnz added feature New feature or request area/api Related to the external API component area/db-system Related to the core system related components of the DB action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels May 12, 2023
@codecov
Copy link

codecov bot commented May 12, 2023 •
edited

Codecov Report

Merging #1499 (8aa317a) into develop (c4b33f0) will increase coverage by 0.07%.
The diff coverage is 51.21%.

❗ Current head 8aa317a differs from pull request most recent head cc00c22. Consider uploading reports for the commit cc00c22 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1499      +/-   ##
===========================================
+ Coverage    72.14%   72.22%   +0.07%     
===========================================
  Files          185      185              
  Lines        18271    18263       -8     
===========================================
+ Hits         13182    13190       +8     
+ Misses        4048     4035      -13     
+ Partials      1041     1038       -3
Impacted Files Coverage Δ
cli/schema_add.go 27.96% <0.00%> (-1.77%) ⬇️
db/txn_db.go 50.89% <44.44%> (-0.94%) ⬇️
api/http/handlerfuncs.go 83.13% <69.23%> (-0.88%) ⬇️
db/schema.go 72.72% <72.72%> (+0.58%) ⬆️

... and 8 files with indirect coverage changes

AndrewSisley
AndrewSisley approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change a lot, thanks John. One question inline RE some http stuff.

api/http/handlerfuncs.go
sendJSON(
req.Context(),
rw,
simpleDataResponse("result", "success"),
simpleDataResponse("result", "success", "collections", colResp),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Do we still want to return "result", "success"? Do we do this for all successful calls across the http api?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had a similar thought. Its a somewhat meaningless item to include, but I have no strong preference either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't meaningless when there was nothing else we were returning but now I think we can safely remove it.

shahzadlone
shahzadlone reviewed May 15, 2023
View reviewed changes
api/http/handlerfuncs_test.go
"result": "success",
"collections": []any{
map[string]any{
"name": "user",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol: 🤣 we still have a lowercase GQL type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol - we have a lot, I think the entire query/simple directory, and the P2P tests are largely still uppercase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah not sure why those didn't get picked up. The api folder make sense, def spaced that. But the query/simple types are perplexing xD

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have +1000 tests atm, would have been amazing if you/Orpheus managed to get all of them lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the cid related ones are done, others are easy to change as/when spotted

shahzadlone
shahzadlone approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, is a nice change for users.

@jsimnz jsimnz force-pushed the jsimnz/feat/add-collection-response branch from 8aa317a to cc00c22 Compare May 16, 2023 17:38
@jsimnz jsimnz merged commit d96624a into develop May 16, 2023
11 checks passed
@jsimnz jsimnz deleted the jsimnz/feat/add-collection-response branch May 16, 2023 17:53
@fredcarle fredcarle modified the milestone: DefraDB v0.6 Jul 17, 2023
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@jsimnz
feat: Add collection response information on creation (sourcenetwork#…
a583305 
…1499)

Updates the response of the CLI, HTTP, and client APIs to include
collection information on schema add/creation.

Internally this is to just return the newly made collection
descriptions. Externally this updates the HTTP API to include a
`collections` key with an array of objects that include `name` and `id`
values.

Corresponding CLI changes to read new HTTP response state was also
made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredcarle fredcarle fredcarle left review comments

@AndrewSisley AndrewSisley AndrewSisley approved these changes

@shahzadlone shahzadlone shahzadlone approved these changes

Assignees
No one assigned
Labels
action/no-benchmark Skips the action that runs the benchmark. area/api Related to the external API component area/cli Related to the CLI binary area/db-system Related to the core system related components of the DB feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When adding a schema include collection(s) name and ID (global)
4 participants
@jsimnz @fredcarle @shahzadlone @AndrewSisley