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

feat: deprecate UUID users #86

Merged
merged 9 commits into from
Nov 26, 2024
Merged

feat: deprecate UUID users #86

merged 9 commits into from
Nov 26, 2024

Conversation

luthfifahlevi
Copy link

@luthfifahlevi luthfifahlevi commented Nov 14, 2024

Currently we have 3 identifiers for compass users, that are: UUID, ID (with UUID type too), and email. The UUID identifier has been used for only validating users for every access Compass API, while the relationship between tables is using ID, even for updated_by column. For validating users flow, it will try to get a user by UUID first. If it is not found, then it will using UpsertByEmail func that run query:

INSERT INTO users (uuid, email, provider) VALUES ($1, $2, $3) ON CONFLICT (email)
DO UPDATE SET uuid = $1, email = $2 WHERE users.uuid IS NULL
RETURNING id

As we can see, the “actual” identifier is an email with ON CONFLICT query but WHERE condition using UUID instead.

Therefore, to avoid unnecessary maintaining more column, we can focus validating on email and relationship key on ID, so only keep these two identifier. We don't need UUID column anymore.

Reference why it has UUID before: raystack#90

@coveralls
Copy link

coveralls commented Nov 14, 2024

Pull Request Test Coverage Report for Build 11908532132

Details

  • 45 of 47 (95.74%) changed or added relevant lines in 9 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 84.029%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/store/postgres/user_repository.go 27 29 93.1%
Files with Coverage Reduction New Missed Lines %
internal/server/v1beta1/asset.go 2 91.25%
Totals Coverage Status
Change from base Build 11100290469: -0.09%
Covered Lines: 6866
Relevant Lines: 8171

💛 - Coveralls

Copy link

@batrov batrov left a comment

Choose a reason for hiding this comment

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

what should we do with existing data that has unit id, but doesn't have email?

@luthfifahlevi
Copy link
Author

what should we do with existing data that has unit id, but doesn't have email?

what you mean is uuid, right? as our ADR discussion before, we'll migrate it first to email as 3rd step https://docs.google.com/document/d/1FZYnbD51m4ZpAoS-ZxQslAgiPaoMOdlbFu4ontx_Jck/edit?tab=t.0

@batrov
Copy link

batrov commented Nov 15, 2024

what should we do with existing data that has unit id, but doesn't have email?

what you mean is uuid, right? as our ADR discussion before, we'll migrate it first to email as 3rd step https://docs.google.com/document/d/1FZYnbD51m4ZpAoS-ZxQslAgiPaoMOdlbFu4ontx_Jck/edit?tab=t.0

have we checked how our clients interact with compass? what happens if they did not send the email key in the header?

@luthfifahlevi
Copy link
Author

luthfifahlevi commented Nov 15, 2024

what should we do with existing data that has unit id, but doesn't have email?

what you mean is uuid, right? as our ADR discussion before, we'll migrate it first to email as 3rd step https://docs.google.com/document/d/1FZYnbD51m4ZpAoS-ZxQslAgiPaoMOdlbFu4ontx_Jck/edit?tab=t.0

have we checked how our clients interact with compass? what happens if they did not send the email key in the header?

I already checked the clients that use UUID only in headers, and for ODS, it found only 2 users, that are journey team and dex, so will inform them to use email headers instead.
For GTF, could you help to check it @anjali9791 @Gadigeppa-J ?
Confirmed that GTF does NOT have another client that use their Compass, so should be safe

@luthfifahlevi luthfifahlevi marked this pull request as ready for review November 15, 2024 07:20
@luthfifahlevi luthfifahlevi merged commit 6119ad8 into main Nov 26, 2024
3 checks passed
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.

4 participants