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

Migrate database to GORM and new schema #8

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jessegeens
Copy link

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
Copy link

@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
Copy link

@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 Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go Outdated Show resolved Hide resolved
share/model.go 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
if m.isProjectAdminFromCtx(ctx, user) {
return "", []interface{}{}, nil
// Now we do the actual update to the db model
switch rs.State {
Copy link

Choose a reason for hiding this comment

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

Why do we do this in 2 steps instead of doing it in the first switch? It's also because it's making me a bit confused that you can change both state and hidden fileds of a share, but then, the state overwrites the hidden value? Not sure I'm making sense...

Copy link
Author

Choose a reason for hiding this comment

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

The switch rs.State should indeed be moved up to case "state" or it will override hidden.

However, I think we have to review some time how we handle the "ShareStates", because now we kind of have double information (we use ShareState_SHARE_STATE_REJECTED to mean hidden, but even the CS3 API ShareState has a field for Hidden, so this seems to be redundant)

Copy link
Member

@glpatcern glpatcern Jan 24, 2025

Choose a reason for hiding this comment

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

I remember arguing with the web people at the time about this... the reasoning was that they wanted IIRC:

  • accepted (= sync-able) + visible
  • accepted + hidden (but still sync-able)
  • rejected (and not sync-able)

Now we're adding the sync as well and there's probably too much redundancy. Let's simplify? -> let's have a quick chat?

@diocas
Copy link

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).

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.

3 participants