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: map strings to UUIDs (#809) #840

Merged
merged 51 commits into from
Jul 27, 2022
Merged

feat: map strings to UUIDs (#809) #840

merged 51 commits into from
Jul 27, 2022

Conversation

zepatrik
Copy link
Member

@zepatrik zepatrik commented Feb 17, 2022 •
edited by hperl
Loading

Related issue(s)

closes #792

Further Comments

Current State:

  • tables and migrations for mapping

Next steps:

  • Use ory/x@6b68933 to finalize the migration process. We need to apply the migrator, then insert the migrated data into columns with type UUID. This probably needs working on a copy of the table.
  • Integrate the mapping step into all handlers so that it automatically happens on every request. Not yet sure if we should optimistically try to parse UUIDs, or use other keys to distinguish strings from UUIDs.
  • Restructure internals to use only UUIDs.
  • Documentation about this feature.

In the end, we will rebase.

@zepatrik zepatrik mentioned this pull request Feb 17, 2022
Closed
6 tasks
@aeneasr
Copy link
Member

aeneasr commented Mar 22, 2022

@zepatrik what about this PR?

@zepatrik
Copy link
Member Author

This is quite a lot of work. The part done already can't be merged yet 'cause it touches migrations and they might need adjustment.

@hperl hperl self-assigned this May 9, 2022
@hperl hperl force-pushed the feat/uuid-mapping branch 4 times, most recently from 208be6a to 01463b0 Compare May 12, 2022 12:05
zepatrik
zepatrik commented May 12, 2022
View reviewed changes
Copy link
Member Author

@zepatrik zepatrik 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, I just have some conceptual thing to discuss tomorrow.

contrib/docs-code-samples/list-api-display-objects/99-cleanup/index.js Outdated Show resolved Hide resolved
internal/expand/handler.go Outdated Show resolved Hide resolved
internal/expand/handler_test.go Outdated Show resolved Hide resolved
internal/expand/tree.go Outdated Show resolved Hide resolved
internal/persistence/sql/migrations/migratest/migration_test.go Outdated Show resolved Hide resolved
internal/relationtuple/test_helper.go Outdated Show resolved Hide resolved
internal/relationtuple/test_helper.go Outdated Show resolved Hide resolved
internal/relationtuple/transact_server_test.go Outdated Show resolved Hide resolved
internal/relationtuple/uuid_mapping.go Outdated Show resolved Hide resolved
internal/relationtuple/uuid_mapping.go Outdated Show resolved Hide resolved
hperl and others added 8 commits May 16, 2022 17:31
@hperl @zepatrik
feat: add table for mapping strings to UUIDs (#809)
75fb3ae 
Co-authored-by: Patrik <[email protected]>
@hperl
feat: map subject/object strings to UUIDs (#792)
46e25e0 
In relation tuples and related API objects such as queries, subjects
(including subject sets) and objects are now automatically mapped to and
from UUIDs.  The mapping is done in each handler and for each protocol
(HTTP, gRPC) separately.

Below the handler, all objects and subjects are now UUIDs, but still
handled as strings, for the following reasons:
 * No duplication of datastructures (one for type `string`, one for type
   `uuid.UUID`).
 * No unnecessary copying, the mapping is done in-place and batched
   across multiple objects.

Additionally, the migration (adding the string values to the mapping
table and replacing the values with UUIDs) is now performed
automatically as part of the migrations.  Tests are in `migratest`,
which now also handles `GoMigrations` properly.  This method replaces
the dedicated command in `cmd/migrate/...` for the UUID mappings.
@hperl
fix: sort expected outputs
5856ce8 
Sorting the expected output of the docs tests make the tests more
robust.
@zepatrik @hperl
fix: add transaction around mapping
5d9bca3 
resolves `table locked` errors with sqlite
@zepatrik @hperl
chore: format
23c36bf
@hperl
feat: add down migration for UUID mapping
b97f25f
@hperl
feat: add batching to UUID mapping
cb8509d
@hperl
tests: more robust test db connections
38c3533
@hperl hperl force-pushed the feat/uuid-mapping branch from 6a12fca to 38c3533 Compare May 16, 2022 15:47
@hperl hperl self-requested a review May 17, 2022 12:50
hperl added 2 commits May 17, 2022 15:30
@hperl
chore: bump ory/x and clean up
08b6631
@hperl
test: paralellize db tests
93940ea 
Database tests now run in separate databases, which opens up the
opportunity to run them in parallel.
@hperl hperl mentioned this pull request May 17, 2022
Merged
@zepatrik zepatrik mentioned this pull request May 27, 2022
Merged
@zepatrik
Merge remote-tracking branch 'origin/master' into feat/uuid-mapping
6900b51 
# Conflicts:
#	go.mod
#	go.sum
#	internal/driver/registry_default.go
#	internal/persistence/sql/migrations/migratest/migration_test.go
#	internal/persistence/sql/relationtuples.go
#	internal/x/dbx/dsn_cockroach.go
#	internal/x/dbx/dsn_mysql.go
#	internal/x/dbx/dsn_postgres.go
#	internal/x/dbx/dsn_sqlite.go
#	internal/x/dbx/dsn_testutils.go
@zepatrik zepatrik force-pushed the feat/uuid-mapping branch from e2bf8b3 to 2fc02fa Compare July 12, 2022 13:46
hperl
hperl reviewed Jul 12, 2022
View reviewed changes
config/keto.yml Outdated Show resolved Hide resolved
hperl
hperl reviewed Jul 19, 2022
View reviewed changes
cmd/relationtuple/output.go Outdated Show resolved Hide resolved
hperl
hperl reviewed Jul 19, 2022
View reviewed changes
ketoapi/enc_string.go Outdated Show resolved Hide resolved
hperl
hperl approved these changes Jul 19, 2022
View reviewed changes
Copy link
Collaborator

@hperl hperl left a comment

Choose a reason for hiding this comment

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

This looks very good! I have some minor comments/questions, but nothing that should hold this up for long.

zepatrik and others added 7 commits July 22, 2022 15:29
@zepatrik
chore: fixup unknown namespace mess
6278f2c
@zepatrik
Merge remote-tracking branch 'origin/master' into feat/uuid-mapping
766e461 
# Conflicts:
#	go.mod
#	go.sum
@hperl
feat: remove namespace ID
93e9a55 
Namespaces are now only identified by their name, not by their ID.

The 'ID' entries in the configuration are still supported, and used for
down-migrations when reverting to a schema that still uses the namespace
IDs.
@hperl
fix: doc sample test ordering
84a9ef0
@hperl
chore: clean up changes
f56429c
@zepatrik
chore: last cleanup
b3deb13
@zepatrik
Merge remote-tracking branch 'origin/master' into feat/uuid-mapping
637a5af
@zepatrik zepatrik merged commit add6577 into master Jul 27, 2022
@zepatrik zepatrik deleted the feat/uuid-mapping branch July 27, 2022 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr left review comments

@hperl hperl hperl approved these changes

Assignees

@hperl hperl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatic Subject/Object encoding
3 participants
@zepatrik @aeneasr @hperl