8000 Unique constraints should use attribute value hash instead of the val… by vramik · Pull Request #15724 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Unique constraints should use attribute value hash instead of the val… - #15724

Merged
hmlnarik merged 1 commit into
keycloak:mainfrom
vramik:15699-hash-columns-for-attribute-values
Jan 5, 2023
Merged

Unique constraints should use attribute value hash instead of the val…#15724
hmlnarik merged 1 commit into
keycloak:mainfrom
vramik:15699-hash-columns-for-attribute-values

Conversation

@vramik
@vramik vramik commented Nov 28, 2022
Copy link
Copy Markdown
Contributor

…ue itself

Closes #15699

@vramik
vramik marked this pull request as ready for review November 28, 2022 18:00
@vramik
vramik requested a review from sguilhen November 28, 2022 18:00
@vramik vramik added area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching team/storage-sig labels Nov 28, 2022
@vramik
vramik force-pushed the 15699-hash-columns-for-attribute-values branch from b0a66bf to 161c304 Compare November 30, 2022 09:56
@vramik
vramik requested a review from sguilhen November 30, 2022 09:59
sguilhen
sguilhen previously approved these changes Nov 30, 2022
View reviewed changes
@sguilhen sguilhen left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great, @vramik , specially the changesets that now look a lot better.

@sguilhen
sguilhen commented Nov 30, 2022
Copy link
Copy Markdown
Contributor

/rerun

hmlnarik
hmlnarik reviewed Dec 1, 2022
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The value should be stored in the native database type

@vramik
vramik force-pushed the 15699-hash-columns-for-attribute-values branch 3 times, most recently from 56708b8 to c08eed7 Compare December 6, 2022 11:55
@vramik
vramik requested review from hmlnarik and sguilhen December 7, 2022 10:25
sguilhen
sguilhen reviewed Dec 16, 2022
View reviewed changes
@sguilhen sguilhen left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great, thanks @vramik . Have only one question about comparing both the value_hash and the value in the criteria builders.

Comment thread ...pa/src/main/java/org/keycloak/models/map/storage/jpa/group/JpaGroupModelCriteriaBuilder.java Resolved
@sguilhen
sguilhen self-requested a review December 19, 2022 11:49
sguilhen
sguilhen previously approved these changes Dec 19, 2022
View reviewed changes
hmlnarik
hmlnarik reviewed Dec 19, 2022
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you, @vramik! Overall looks good to me, I have a few questions inline.

Comment thread model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/JpaModelCriteriaBuilder.java Resolved
Comment thread ...-jpa/src/main/java/org/keycloak/models/map/storage/jpa/user/JpaUserModelCriteriaBuilder.java Resolved
Comment thread ...pa/src/main/java/org/keycloak/models/map/storage/jpa/group/JpaGroupModelCriteriaBuilder.java Resolved
Comment thread .../src/main/java/org/keycloak/models/map/storage/jpa/client/JpaClientModelCriteriaBuilder.java Resolved
Comment thread model/map-jpa/src/main/resources/META-INF/users/jpa-users-changelog-2.xml Resolved
@vramik
vramik force-pushed the 15699-hash-columns-for-attribute-values branch 2 times, most recently from 2c528d4 to 546506e Compare January 2, 2023 21:12
@vramik
vramik force-pushed the 15699-hash-columns-for-attribute-values branch from bf6478a to 35699c0 Compare January 3, 2023 20:24
hmlnarik
hmlnarik approved these changes Jan 4, 2023
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the PR!
@sguilhen Could you please check this PR?

@hmlnarik
hmln 8000 arik merged commit f7ad002 into keycloak:main Jan 5, 2023
@vramik
vramik deleted the 15699-hash-columns-for-attribute-values branch November 5, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@sguilhen sguilhen sguilhen approved these changes
@hmlnarik hmlnarik hmlnarik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@hmlnarik hmlnarik

Labels

area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unique constraints should use attribute value hash instead of the value itself

3 participants

@vramik @sguilhen @hmlnarik
0