Skip to content

Switch UGID from type INT to TEXT#1144

Merged
adombeck merged 5 commits intomainfrom
fix-ugid-type
Dec 1, 2025
Merged

Switch UGID from type INT to TEXT#1144
adombeck merged 5 commits intomainfrom
fix-ugid-type

Conversation

@jibel
Copy link
Contributor

@jibel jibel commented Nov 27, 2025

ugid are ids provided by the broker that we do not control. They are rightfully treated as strings in the brokers and authd, but declared as INT in the database schema.

This PR uses the right type for UGID and adds 2 tests for the migration from v1 and old v2.

STRICT is not enforced at the moment, because it would require to change BOOLEAN of the locked column as well.

The casing of the table name has been fixed as well.

UDENG-7762

@jibel jibel requested a review from a team as a code owner November 27, 2025 14:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.78%. Comparing base (c5c1afd) to head (b1c027b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
- Coverage   87.82%   87.78%   -0.04%     
==========================================
  Files          89       89              
  Lines        6150     6150              
  Branches      111      111              
==========================================
- Hits         5401     5399       -2     
- Misses        693      695       +2     
  Partials       56       56              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@adombeck adombeck left a comment

Choose a reason for hiding this comment

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

Nice work! Just a few small comments. I'm glad we don't need a schema migration :)

@adombeck
Copy link
Contributor

STRICT is not enforced at the moment, because it would require to change BOOLEAN of the locked column as well.

Oh, I must have missed that in the review of #782. Do you know what the locked column is treated as? Is it also TEXT? 😵

@3v1n0
Copy link
Contributor

3v1n0 commented Nov 28, 2025

Nice, migrations check on a more complete setup are already covered by the integration tests we already have, right?

@adombeck
Copy link
Contributor

migrations check on a more complete setup are already covered by the integration tests we already have, right?

The change works without any schema migration which the tests added here prove, so I don't see the need for testing more complex setups - or am I missing something?

@jibel
Copy link
Contributor Author

jibel commented Nov 28, 2025

STRICT is not enforced at the moment, because it would require to change BOOLEAN of the locked column as well.

Oh, I must have missed that in the review of #782. Do you know what the locked column is treated as? Is it also TEXT? 😵

It is treated as INT

TestLoadOldSchemaV2Database ensures databases created with the old
schema (v2, ugid INT) can be opened and read correctly by the current
code without additional migrations.
TestMigrateOldSchemaV1 ensures databases created with schema v1 (no
'locked' column) are transparently migrated on open and readable via
current code.
@adombeck adombeck merged commit 1c7cdb1 into main Dec 1, 2025
23 of 25 checks passed
@adombeck adombeck deleted the fix-ugid-type branch December 1, 2025 12:17
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