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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Replace GORM with GORMv2 #4203

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Conversation

albrechtf
Copy link

I mark this currently as a Draft, although the Go tests look quite good on my end. I will have to do a lot more tests, especially with MariaDB and using the frontend.

Notable changes with GORM v2:

  • I had to change type Map, which was (contrary to its description) not a type alias, to a type alias now, because Gorm otherwise complains about "unsupported data" in all kinds of operations (that took me only five hours to find).
  • Gorm now no longer supports the association_ tags. I am really not sure by what to replace some of them. As long as it is just about column naming, replacements are joinForeignKey etc., and that was easy to replace.
  • Same for Preload tag. This now has to be done via code. I removed them when I encountered them, but some may still be there (without effect). As I did not add equivalent code as a replacement, this could decrease performance.
  • DropTableIfExists is no longer supported by the Migrator. I replaced it with DropTable and just a log of any errors.
  • Gorm now e.g. supports soft-deletion out of the box. I did not use that, as this is already implemented manually for some object types. This is something for a later PR (by another person, probably).
  • sqlite3 is now sqlite in Gorm (sounds like a good idea to me). I only changed the strings (and directory name), but not the constant names.

Other notes:

  • I am really not familiar with Go, so I am not sure about the changes in the go.mod file. That is just what go mod tidy did for me 馃槃

Acceptance Criteria:

  • Features and enhancements must be fully implemented so that they can be released at any time without additional work
  • [? yeah think so] Automated unit and/or acceptance tests are mandatory to ensure the changes work as expected and to reduce repetitive manual work
  • Frontend components must be responsive to work and look properly on phones, tablets, and desktop computers; you must have tested them on all major browsers and different devices
  • Documentation and translation updates should be provided if needed
  • In case you submit database-related changes, they must be tested and compatible with SQLite 3 and MariaDB 10.5.12+

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2024

CLA assistant check
All committers have signed the CLA.

@albrechtf albrechtf marked this pull request as draft April 21, 2024 17:28
@lastzero
Copy link
Member

@albrechtf Thanks a lot for working on this! I really appreciate it and look forward to taking a look at your changes when I have some time, either tomorrow or when I'm back. I'll only be in the office until Wednesday to get some rest after the release.

The GORM v1 magic behind Associations had stopped me out and made me revert all my changes for v2 when I tried to do this myself because I didn't have the time to find a good solution that would work for all existing use cases. I didn't really like the Associations to begin with, but ended up using them anyway so as not to work against the framework and have working software that we can release.

We wouldn't be sad at all if the existing Associations could be replaced with something else, as long as we don't have to write lots of manual queries (via copy & paste) instead and the new solution is reliable enough to be released...

Similarly, Preload is framework magic that we would like to refactor/replace if this is possible without risking hard to find bugs that we then have to identify and fix before we can release the next stable version.

The other changes you mentioned seem straightforward and relatively low risk! 馃憤

@lastzero
Copy link
Member

lastzero commented May 18, 2024

If you are interested in more detailed feedback, I would be happy to take a look at your code/changes early next week? Anything other than the fixtures you want me to check in particular? Note that, unfortunately, I won't be able to provide you with a ready to use solution/replacement for the Associations and Preload functionality available in Gorm v1.

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.

None yet

3 participants