Skip to content

feat: deprecate autoincrement primary key in hydra_client#2784

Merged
aeneasr merged 7 commits intoory:v2.xfrom
grantzvolsky:feature/deprecate-autoincrement
Oct 11, 2021
Merged

feat: deprecate autoincrement primary key in hydra_client#2784
aeneasr merged 7 commits intoory:v2.xfrom
grantzvolsky:feature/deprecate-autoincrement

Conversation

@grantzvolsky
Copy link
Copy Markdown
Contributor

@grantzvolsky grantzvolsky commented Oct 7, 2021

This patch deprecates the hydra_client.pk autoincrement primary key in favour of a new UUID primary key column. The values of the new column are generated in application code instead of being generated in the database.

This is the first step towards resolving #2781. The issue will be resolved when we remove the deprecated column.

CAUTION: The patch includes a database migration that must be applied offline while simultaneously updating all hydra instances to the latest version.

Related issue(s)

#2781
@aeneasr

Checklist

  • I have read the contributing guidelines.
  • 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 green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • [Tests already existed] I have added tests that prove my fix is effective or that my feature
    works.
  • [Nothing to document other than an entry in the changelog] I have added or changed the documentation.

Further Comments

@grantzvolsky
feat: deprecate autoincrement primary key in hydra_client
26aa373 
This is the first step towards resolving ory#2781. The issue will be resolved when
we remove the deprecated column.
aeneasr
aeneasr reviewed Oct 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Looking good :)

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Oct 8, 2021

Are the test passing for you locally?

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Oct 8, 2021

If you need help with the DB tests LMK

@grantzvolsky
Copy link
Copy Markdown
Contributor Author

grantzvolsky commented Oct 8, 2021

Are the test passing for you locally?

The tests are passing most of the time, but I randomly get this[1] error with ~5% probability. Is this test randomly failing a known issue?

[1]:
Test: TestManagers/package=consent/janitor=mysql/case=flush-consent-request-limit/case=limit/step=validate
Erro: An error is expected but got nil.
https://app.circleci.com/pipelines/github/ory/hydra/2992/workflows/4fef669c-6357-4031-9902-38594d68b5a3/jobs/33318

@grantzvolsky
Copy link
Copy Markdown
Contributor Author

grantzvolsky commented Oct 8, 2021 •
edited
Loading

If you need help with the DB tests LMK

I've just added this1 test to make sure that the migrations result in a table with the correct columns. Should I add more tests, e.g. to make sure that the backfilled UUIDs are valid UUIDv4?

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (v2.x@8373bba). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             v2.x    #2784   +/-   ##
=======================================
  Coverage        ?   52.87%           
=======================================
  Files           ?      236           
  Lines           ?    14185           
  Branches        ?        0           
=======================================
  Hits            ?     7500           
  Misses          ?     6053           
  Partials        ?      632

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8373bba...1d20c54. Read the comment docs.

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Oct 8, 2021

to make sure that the backfilled UUIDs are valid UUIDv4?

I think that would make sense! It would also be good to ensure that new client creations use the new UUID key format. You could test this by just creating a client and also fetching it.

The tests are passing most of the time, but I randomly get this[1] error with ~5% probability. Is this test randomly failing a known issue?

Test: TestManagers/package=consent/janitor=mysql/case=flush-consent-request-limit/case=limit/step=validate

Yes, this is a known issue /cc @Benehiko @flavioleggio

@grantzvolsky grantzvolsky marked this pull request as ready for review October 8, 2021 12:43
@aeneasr aeneasr changed the base branch from master to v2.x October 11, 2021 14:01
aeneasr
aeneasr approved these changes Oct 11, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM :)

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Oct 11, 2021

@grantzvolsky I changed base to v2.x which we will use to have all these migration changes so that we can still do releases in case of bug fixes and maintenances :)

@aeneasr aeneasr merged commit 09ac91b into ory:v2.x Oct 11, 2021
grantzvolsky added a commit to grantzvolsky/hydra that referenced this pull request Feb 7, 2022
@grantzvolsky
feat: deprecate autoincrement primary key in hydra_client (ory#2784)
d8c5024 
Closes ory#2781
grantzvolsky added a commit to grantzvolsky/hydra that referenced this pull request Mar 8, 2022
@grantzvolsky
feat: deprecate autoincrement primary key in hydra_client (ory#2784)
47c928e 
Closes ory#2781
grantzvolsky added a commit that referenced this pull request May 19, 2022
@grantzvolsky
feat: deprecate autoincrement primary key in hydra_client (#2784)
701e4a5 
Closes #2781
aeneasr pushed a commit that referenced this pull request Jun 27, 2022
@grantzvolsky @aeneasr
feat: deprecate autoincrement primary key in hydra_client (#2784)
0262fce 
Closes #2781
grantzvolsky added a commit that referenced this pull request Aug 1, 2022
@grantzvolsky
feat: deprecate autoincrement primary key in hydra_client (#2784)
ed043b9 
Closes #2781
aeneasr pushed a commit that referenced this pull request Aug 1, 2022
@grantzvolsky @aeneasr
feat: deprecate autoincrement primary key in hydra_client (#2784)
cd24fb4 
Closes #2781
aeneasr pushed a commit that referenced this pull request Aug 18, 2022
@grantzvolsky @aeneasr
feat: deprecate autoincrement primary key in hydra_client (#2784)
8550517 
Closes #2781
aeneasr pushed a commit that referenced this pull request Sep 5, 2022
@grantzvolsky @aeneasr
feat: deprecate autoincrement primary key in hydra_client (#2784)
60e85cb 
Closes #2781
aeneasr pushed a commit that referenced this pull request Sep 7, 2022
@grantzvolsky @aeneasr
feat: deprecate autoincrement primary key in hydra_client (#2784)
6d01e2e 
Closes #2781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeneasr aeneasr aeneasr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@grantzvolsky @aeneasr