feat: re-enable legacy client IDs#3628
Merged
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3628 +/- ##
==========================================
- Coverage 76.18% 76.06% -0.13%
==========================================
Files 133 132 -1
Lines 10043 9971 -72
==========================================
- Hits 7651 7584 -67
+ Misses 1868 1866 -2
+ Partials 524 521 -3
|
aeneasr
reviewed
Sep 6, 2023
aeneasr
reviewed
Sep 8, 2023
Comment on lines
-56
to
-57
| // set the internal primary key | ||
| cl.ID = o.ID |
Member
Author
There was a problem hiding this comment.
The previous UUID (pk) is basically not used. For cockroach, we can just use a hash-sharded index to avoid hot-spots. Mysql, postgres, and sqlite are fine with sequential keys (mysql actually prefers them).
bcd0f5c to
233c8df
Compare
alnr
previously approved these changes
Sep 13, 2023
alnr
left a comment
Contributor
There was a problem hiding this comment.
Wow, lots of tedious work. Well done!
Contributor
|
Couple more failing tests... LMK if you want help. |
alnr
previously approved these changes
Sep 15, 2023
aeneasr
reviewed
Sep 18, 2023
aeneasr
left a comment
Member
There was a problem hiding this comment.
Please check all foreign key relations, and check that pk has no foreign key relations either.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE hydra_client ALTER PRIMARY KEY USING COLUMNS (id, nid) USING HASH; | |||
Member
There was a problem hiding this comment.
Now that id can be user-set, this can lead to ID collisions. We need to ensure that all foreign key lookups consider the full primary key.
Member
Author
There was a problem hiding this comment.
I checked, and all foreign-keys on the client table use (id, nid):
select col.constraint_name, col.table_name, column_name, ordinal_position, referenced_table_name from information_schema.key_column_usage as col join information_schema.referential_constraints as constr on col.constraint_name = constr.constraint_name where referenced_table_name != 'networks';
constraint_name | table_name | column_name | ordinal_position | referenced_table_name
--------------------------------------------------------------+------------------------------------------------+------------------+------------------+--------------------------------------
hydra_oauth2_obfuscated_authentication_session_client_id_fk | hydra_oauth2_obfuscated_authentication_session | client_id | 1 | hydra_client
hydra_oauth2_obfuscated_authentication_session_client_id_fk | hydra_oauth2_obfuscated_authentication_session | nid | 2 | hydra_client
hydra_oauth2_logout_request_client_id_fk | hydra_oauth2_logout_request | client_id | 1 | hydra_client
hydra_oauth2_logout_request_client_id_fk | hydra_oauth2_logout_request | nid | 2 | hydra_client
hydra_oauth2_access_challenge_id_fk | hydra_oauth2_access | challenge_id | 1 | hydra_oauth2_flow
hydra_oauth2_access_client_id_fk | hydra_oauth2_access | client_id | 1 | hydra_client
hydra_oauth2_access_client_id_fk | hydra_oauth2_access | nid | 2 | hydra_client
hydra_oauth2_refresh_challenge_id_fk | hydra_oauth2_refresh | challenge_id | 1 | hydra_oauth2_flow
hydra_oauth2_refresh_client_id_fk | hydra_oauth2_refresh | client_id | 1 | hydra_client
hydra_oauth2_refresh_client_id_fk | hydra_oauth2_refresh | nid | 2 | hydra_client
hydra_oauth2_code_challenge_id_fk | hydra_oauth2_code | challenge_id | 1 | hydra_oauth2_flow
hydra_oauth2_code_client_id_fk | hydra_oauth2_code | client_id | 1 | hydra_client
hydra_oauth2_code_client_id_fk | hydra_oauth2_code | nid | 2 | hydra_client
hydra_oauth2_oidc_challenge_id_fk | hydra_oauth2_oidc | challenge_id | 1 | hydra_oauth2_flow
hydra_oauth2_oidc_client_id_fk | hydra_oauth2_oidc | client_id | 1 | hydra_client
hydra_oauth2_oidc_client_id_fk | hydra_oauth2_oidc | nid | 2 | hydra_client
hydra_oauth2_pkce_challenge_id_fk | hydra_oauth2_pkce | challenge_id | 1 | hydra_oauth2_flow
hydra_oauth2_pkce_client_id_fk | hydra_oauth2_pkce | client_id | 1 | hydra_client
hydra_oauth2_pkce_client_id_fk | hydra_oauth2_pkce | nid | 2 | hydra_client
hydra_oauth2_flow_client_id_fk | hydra_oauth2_flow | client_id | 1 | hydra_client
hydra_oauth2_flow_client_id_fk | hydra_oauth2_flow | nid | 2 | hydra_client
hydra_oauth2_flow_login_session_id_fk | hydra_oauth2_flow | login_session_id | 1 | hydra_oauth2_authentication_session
fk_key_set_ref_hydra_jwk | hydra_oauth2_trusted_jwt_bearer_issuer | key_set | 1 | hydra_jwk
fk_key_set_ref_hydra_jwk | hydra_oauth2_trusted_jwt_bearer_issuer | key_id | 2 | hydra_jwk
fk_key_set_ref_hydra_jwk | hydra_oauth2_trusted_jwt_bearer_issuer | nid | 3 | hydra_jwk
Member
Author
|
Same way, |
Member
|
Nice! So I think we can drop this then |
Member
|
Do we have any indication how fast this will be on different systems? Some of our customers have rather large DBs already. |
aeneasr
previously approved these changes
Sep 19, 2023
aeneasr
approved these changes
Sep 19, 2023
Member
|
If you add the commit body to the PR description I can squash force merge (docker image scanner is failing) |
Open
6 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #2911