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.

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 API to batch insert identities #3157

Merged
merged 4 commits into from
Mar 29, 2023
Merged

Conversation

hperl
Copy link
Contributor

@hperl hperl commented Mar 9, 2023

This PR does:

  • add a batch API for creating multiple identities in one request
  • add a generic batch.Create function to create multiple records in one INSERT statement
  • uses batch.Create in creating multiple identities (with up to four individual queries depending on the structure of the identitites)

All new code is tested extensively, because the single-identity create is now a special-case of the multiple-identities create.

Related issue(s)

Fixes ory/network#266

Checklist

  • Add new API endpoint and definitions
  • Implement a generic batch handler
    (Optional:) add delete action and dry_run paramenter

identity/identity.go Outdated Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
@hperl hperl force-pushed the hperl/batch-identities branch 2 times, most recently from f13d61a to bf3faaf Compare March 9, 2023 15:07
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #3157 (1cac084) into master (08a3447) will increase coverage by 0.24%.
The diff coverage is 90.96%.

❗ Current head 1cac084 differs from pull request most recent head 21605bf. Consider uploading reports for the commit 21605bf to get more accurate results

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
+ Coverage   77.68%   77.92%   +0.24%     
==========================================
  Files         319      320       +1     
  Lines       20162    20401     +239     
==========================================
+ Hits        15663    15898     +235     
- Misses       3301     3303       +2     
- Partials     1198     1200       +2     
Impacted Files Coverage Δ
persistence/sql/identity/persister_identity.go 80.03% <83.69%> (+1.71%) ⬆️
identity/handler.go 86.06% <87.03%> (+0.29%) ⬆️
persistence/sql/batch/create.go 91.04% <91.04%> (ø)
driver/config/config.go 82.87% <100.00%> (ø)
identity/extension_verify.go 97.56% <100.00%> (+0.33%) ⬆️
identity/identity.go 91.75% <100.00%> (+0.44%) ⬆️
identity/manager.go 80.17% <100.00%> (+2.07%) ⬆️
identity/test/pool.go 100.00% <100.00%> (ø)
x/xsql/sql.go 93.33% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hperl hperl force-pushed the hperl/batch-identities branch 4 times, most recently from c4d1884 to fce9933 Compare March 13, 2023 16:26
identity/identity.go Show resolved Hide resolved
identity/identity.go Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
identity/identity.go Show resolved Hide resolved
identity/handler.go Outdated Show resolved Hide resolved
identity/manager.go Fixed Show fixed Hide fixed
@hperl hperl force-pushed the hperl/batch-identities branch 5 times, most recently from 9d20941 to cd1389d Compare March 22, 2023 15:34
@hperl hperl marked this pull request as ready for review March 22, 2023 15:37
@hperl hperl requested a review from zepatrik as a code owner March 22, 2023 15:37
@hperl hperl requested a review from alnr March 23, 2023 07:55
@hperl hperl force-pushed the hperl/batch-identities branch 2 times, most recently from 4f852a6 to 758a567 Compare March 23, 2023 09:33
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! This looks pretty good already, but I had a hard time understanding how the persister creates the batches. I will need to do another pass.

All new code is tested extensively, because the single-identity create is now a special-case of the multiple-identities create.

Not on my watch ;) This code is essential and has security implications. There can not be confusion of credentials or addresses when creating things in batch. Yet, there is not a single test assuring that we are actually creating things correctly when creating multiple things at once. Please add appropriate tests to prove that your changes work as
expected.

Please add dedicated persister tests and manager tests as well. We will need to use them when adjusting the persister for our enterprise use cases in the Ory Network.

Thanks!

persistence/sql/batch/create.go Show resolved Hide resolved
identity/handler.go Outdated Show resolved Hide resolved
identity/handler_test.go Show resolved Hide resolved
identity/handler_test.go Outdated Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
identity/pool.go Show resolved Hide resolved
persistence/sql/update/update.go Outdated Show resolved Hide resolved
persistence/sql/update/update.go Outdated Show resolved Hide resolved
persistence/sql/update/update.go Outdated Show resolved Hide resolved
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Ok, took another look at the persister itself. I think the code falls into the classic Go "for range over a slice of pointers" trap. I think this could have serious implications when processing 2...n elements

persistence/sql/batch/create.go Show resolved Hide resolved
persistence/sql/batch/create.go Show resolved Hide resolved
persistence/sql/batch/create.go Show resolved Hide resolved
persistence/sql/batch/create.go Show resolved Hide resolved
persistence/sql/batch/create.go Show resolved Hide resolved
persistence/sql/identity/persister_identity.go Outdated Show resolved Hide resolved
@hperl hperl force-pushed the hperl/batch-identities branch 2 times, most recently from 350f76b to 2b9bfb0 Compare March 24, 2023 13:44
@hperl hperl requested a review from aeneasr March 24, 2023 14:27
identity/handler_test.go Outdated Show resolved Hide resolved
@hperl hperl force-pushed the hperl/batch-identities branch 2 times, most recently from 96da26f to 7fff0a0 Compare March 24, 2023 15:33
aeneasr
aeneasr previously approved these changes Mar 26, 2023
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Very nice :) One potential test could be added to ensure the merging of verification addresses works correctly :)

identity/extension_verify.go Show resolved Hide resolved
identity/handler.go Outdated Show resolved Hide resolved
@aeneasr aeneasr merged commit 829bda7 into master Mar 29, 2023
@aeneasr aeneasr deleted the hperl/batch-identities branch March 29, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow batch import of identities
3 participants