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

fix: readonly UUID mapping inserts #1190

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

hperl
Copy link
Collaborator

@hperl hperl commented Jan 14, 2023

This PR adds a read-only version of the UUID mapper that performs the mappings but does not write them to the database. This mapper suffices for all endpoints that do not add new tuples to the database, and is obviously much faster that the read-write version of the mapper.

Tests were added to show that the read-only mapper behaves the same as the read-write version, if all information is already mapped.

Related issue(s)

Fixes #1076

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Endpoints that do not mutate the database (such as list
or check) now use a read-only version of the UUID mapper
that does not write the mapping to the database (as all
relevant mapping information is already mapped).
@hperl hperl requested a review from zepatrik as a code owner January 14, 2023 20:06
@hperl hperl requested a review from alnr January 14, 2023 20:06
@hperl hperl self-assigned this Jan 14, 2023
@hperl hperl changed the title hperl/fix-readmode-uuid-mapping-inserts fix: readonly UUID mapping inserts Jan 14, 2023
Copy link
Member

@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.

Nice, I would just like to add one test that ensures the UUIDs are actually (not) persisted depending on the mapper.


reUnmapped, err := roMapper.ToQuery(ctx, mapped)
require.NoError(t, err)
assert.Empty(t, reUnmapped.Object)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe this case should instead return an error, as the RO mapper is unable to do the mapping? Just to not have bugs later on.

@hperl hperl force-pushed the hperl/fix-readmode-uuid-mapping-inserts branch from 30f0e47 to ef4ed58 Compare January 17, 2023 13:44
@zepatrik zepatrik merged commit a86db70 into master Jan 17, 2023
@zepatrik zepatrik deleted the hperl/fix-readmode-uuid-mapping-inserts branch January 17, 2023 14:56
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.

Querying ListRelationTuples with any subject results in row being added to keto_uuid_mappings
2 participants