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.

Sign up for GitHub

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

Jump to bottom

Migrate database to GORM and new schema #8

Merged
merged 12 commits into from
Feb 3, 2025
Merged

Migrate database to GORM and new schema #8

merged 12 commits into from
Feb 3, 2025

Conversation

jessegeens
Copy link
Contributor

Following ADR-Reva-003, this PR refactors the sharing SQL logic to now use GORM, an ORM for Go. This change also comes with newly written unit tests for all the database logic.

@jessegeens jessegeens marked this pull request as ready for review January 8, 2025 08:35
@jessegeens jessegeens requested a review from glpatcern January 8, 2025 08:35
@jessegeens jessegeens mentioned this pull request Jan 8, 2025
Open
diocas
diocas reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

Some questions and comments. Haven't had the time to review the tests yet.

share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
@jessegeens jessegeens force-pushed the fix/tx-sharing branch 3 times, most recently from 6b8c45a to 7843bfb Compare January 10, 2025 16:03
@jessegeens jessegeens force-pushed the fix/tx-sharing branch 2 times, most recently from f2d54aa to 48683cf Compare January 13, 2025 12:16
diocas
diocas reviewed Jan 23, 2025
View reviewed changes
Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

Another round of comments... Can we also add the annotations that set the indexes and constraints to the gorm models?

share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
share/sql/sql.go Outdated Show resolved Hide resolved
@diocas
Copy link
Contributor

diocas commented Jan 23, 2025

As discussed via MM, I would also like to confirm that null values (via manually inserted rows) do not break anything. (or, we just set annotations with the proper sql configs).

@jessegeens jessegeens force-pushed the fix/tx-sharing branch 2 times, most recently from cca2168 to 9337420 Compare February 3, 2025 09:58
glpatcern
glpatcern approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

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

This is all good now, NULL values were also checked and the generated schema was reviewed so to have varchar with reasonable sizes as opposed to longtext.
The migration went through in test and we can go for QA.

@glpatcern glpatcern merged commit aa40edc into master Feb 3, 2025
@jessegeens jessegeens mentioned this pull request Feb 4, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diocas diocas diocas left review comments

@glpatcern glpatcern glpatcern 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.
3 participants
@jessegeens @diocas @glpatcern