reminders: migrate away from gorm to sqlboiler #1652

jo3-l · 2024-05-22T05:28:24Z

This is 1/n PRs switching the codebase away from gorm to sqlboiler.

Best reviewed commit-by-commit. The database schema code (first commit) should be reviewed especially carefully, since the relevant queries will be run automatically on startup.

On `NOT NULL` constraints

By default, gorm does not add NOT NULL constraints to columns not explicitly marked as such. However, a number of columns in our gorm models across the codebase are effectively non-nullable -- for instance, the reminders.user_id column is never empty in valid data, but is nonetheless nullable in the table definition. This complicates migration to sqlboiler, since if the current table definition is kept, sqlboiler will faithfully reflect the state of the database and generate models with nullable field types. But as our current codebase implicitly assumes many fields to be non-null, working with sqlboiler models thus becomes quite convoluted.

To address this issue, we have decided to retroactively add NOT NULL constraints when applicable to existing tables. This will be done automatically at startup once these changes are applied using the existing database migration machinery. Self-hosters upgrading to a release containing the changes here should therefore be ready for a delayed first restart, as adding new NOT NULL constraints require Postgres to acquire an exclusive lock on and fully scan tables to check that no entries contain null columns.

While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps.

jo3-l · 2024-05-22T05:32:07Z

reminders/plugin_bot.go

-			delMsg := fmt.Sprintf("Deleted reminder **#%d**: '%s'", reminder.ID, limitString(reminder.Message))
-
-			// If there is another reminder with the same timestamp, do not remove the scheduled event
-			for _, v := range currentReminders {
-				if v.When == reminder.When {
-					return delMsg, nil
-				}
-			}
-
-			return delMsg, nil


The comment here implies that the scheduled event to send the reminder will be removed if there exists no reminder with the same timestamp, but this is not the case. The current code simplifies fully to return delMsg, nil, so there is no functional change despite appearances.

Per the new comment added above, it is perfectly fine to not delete the scheduled event and just remove the database entry.

jo3-l · 2024-05-22T05:32:34Z

reminders/plugin_bot.go

 		},
 	},
 }

-func stringReminders(reminders []*Reminder, displayUsernames bool) string {


This was moved to reminders.go and renamed to DisplayReminders.

jo3-l · 2024-05-22T05:33:38Z

reminders/plugin_bot.go

-	for _, v := range reminders {
-		if v.When <= nowUnix {
-			err := v.Trigger()
+	// TODO: can we move this filtering step into the database query?


I think we can, but it is probably preferable to minimize non-database refactors (that are not obviously correct) to prevent introducing bugs. Perhaps we can try this refactor in a future version, once the gorm -> sqlboiler transition is complete.

jo3-l · 2024-05-22T05:35:07Z

reminders/reminders.go

 )

+//go:generate sqlboiler --no-hooks --add-soft-deletes psql


I was surprised to see that reminders currently soft-delete instead of hard-delete. That is, once triggered, they remain in the database with the deleted_at field set. I am unsure whether this is an oversight -- gorm does not make it particularly obvious that soft-delete is the default behavior -- or intended.

This PR currently continues to soft-delete, but we could easily switch to hard deletions.

reminders/schema.go

jo3-l · 2024-05-22T05:37:07Z

lib/discordgo/discord.go

@@ -153,3 +154,7 @@ func CheckRetry(_ context.Context, resp *http.Response, err error) (bool, error)
 func StrID(id int64) string {
 	return strconv.FormatInt(id, 10)
 }
+
+func ParseID(s string) (int64, error) {


See description of third commit for why this alias was added.

Largely uncontroversial other than the choice to introduce the Config struct (in addition to the autogenerated models.ModerationConfig.) See long comment in config.go for details. Unlike botlabs-gg#1652, I generally avoided any unrelated refactors since the moderation plugin is considerably more complicated and changes are high-stakes.

jo3-l · 2024-05-24T19:29:26Z

I think this PR is ready to review and merge, when appropriate.

Largely uncontroversial other than the choice to introduce the Config struct (in addition to the autogenerated models.ModerationConfig.) See long comment in config.go for details. Unlike botlabs-gg#1652, I generally avoided any unrelated refactors since the moderation plugin is considerably more complicated and changes are high-stakes.

* reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary

* all: migrate executed command logging away from gorm to sqlboiler (#1654) * common: add schemas for executed command logs * common: configure and run sqlboiler for executed command log model * all: migrate executed command logging to sqlboiler * reminders: migrate away from gorm to sqlboiler (#1652) * reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary * soundboard: remove stray references to gorm (#1651) The soundboard module used to use gorm, but was migrated to sqlboiler some years ago in commit 628ea9a. There are still two (erroneous) references to gorm remaining which were not caught since this section of code ignores errors. This commit changes these to use sqlboiler as well. * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible (#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans. * youtube: migrate away from gorm to sqlboiler (#1660) * youtube: add database schemas * youtube: set up and run sqlboiler * youtube: migrate to sqlboiler * notifications: migrate away from gorm to sqlboiler (#1659) * notifications: decouple from configstore * notifications: add database schemas * notifications: set up and run sqlboiler * notifications: migrate to sqlboiler * moderation: migrate away from gorm to sqlboiler (#1658) * moderation: decouple from configstore * moderation: add database schemas * moderation: set up and run sqlboiler * moderation: migrate to sqlboiler * web: support null.Int64 in forms and validator We currently use gorilla/schema to parse HTML form values into structs -- this allows us to specify, e,g., `<input ... name=SomeField>` and have `payload.SomeField` set correctly in Go handlers. This approach mostly works nicely, with the major exception of null.Int64 (formerly sql.NullInt64 with gorm) fields, which show up in the moderation plugin, e.g., Config.DefaultMuteDuration. Particularly, since gorilla/schema has no native support for null.Int64, we have instead instructed gorilla/schema to directly set the Int64 value using <input ... name=DefaultMuteDuration.Int64> and then manually set DefaultMuteDuration.Valid=true in the web handler, without which the changes are not saved to database. This approach is, for obvious reasons, rather brittle. In this commit we therefore register a custom gorilla/schema decoder for null.Int64, obviating the need for both pointing to the Int64 field and manually setting Valid=true. While at it, we also fix the validator so that it checks null.Int64 fields properly. * web: support validating types.Int64Array gorm uses pq.Int64Array but sqlboiler uses types.Int64Array, so we need also support the latter now. * common/configstore: remove package (#1668) * all: remove last traces of gorm (#1670) * common: connect to postgres via database/sql ...instead of through gorm. * common: remove gorm log hook * common: remove gorm SmallModel * deps: run go mod tidy --------- Co-authored-by: Joe L <[email protected]>

…g#1655) * all: migrate executed command logging away from gorm to sqlboiler (botlabs-gg#1654) * common: add schemas for executed command logs * common: configure and run sqlboiler for executed command log model * all: migrate executed command logging to sqlboiler * reminders: migrate away from gorm to sqlboiler (botlabs-gg#1652) * reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary * soundboard: remove stray references to gorm (botlabs-gg#1651) The soundboard module used to use gorm, but was migrated to sqlboiler some years ago in commit 628ea9a. There are still two (erroneous) references to gorm remaining which were not caught since this section of code ignores errors. This commit changes these to use sqlboiler as well. * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible (botlabs-gg#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans. * youtube: migrate away from gorm to sqlboiler (botlabs-gg#1660) * youtube: add database schemas * youtube: set up and run sqlboiler * youtube: migrate to sqlboiler * notifications: migrate away from gorm to sqlboiler (botlabs-gg#1659) * notifications: decouple from configstore * notifications: add database schemas * notifications: set up and run sqlboiler * notifications: migrate to sqlboiler * moderation: migrate away from gorm to sqlboiler (botlabs-gg#1658) * moderation: decouple from configstore * moderation: add database schemas * moderation: set up and run sqlboiler * moderation: migrate to sqlboiler * web: support null.Int64 in forms and validator We currently use gorilla/schema to parse HTML form values into structs -- this allows us to specify, e,g., `<input ... name=SomeField>` and have `payload.SomeField` set correctly in Go handlers. This approach mostly works nicely, with the major exception of null.Int64 (formerly sql.NullInt64 with gorm) fields, which show up in the moderation plugin, e.g., Config.DefaultMuteDuration. Particularly, since gorilla/schema has no native support for null.Int64, we have instead instructed gorilla/schema to directly set the Int64 value using <input ... name=DefaultMuteDuration.Int64> and then manually set DefaultMuteDuration.Valid=true in the web handler, without which the changes are not saved to database. This approach is, for obvious reasons, rather brittle. In this commit we therefore register a custom gorilla/schema decoder for null.Int64, obviating the need for both pointing to the Int64 field and manually setting Valid=true. While at it, we also fix the validator so that it checks null.Int64 fields properly. * web: support validating types.Int64Array gorm uses pq.Int64Array but sqlboiler uses types.Int64Array, so we need also support the latter now. * common/configstore: remove package (botlabs-gg#1668) * all: remove last traces of gorm (botlabs-gg#1670) * common: connect to postgres via database/sql ...instead of through gorm. * common: remove gorm log hook * common: remove gorm SmallModel * deps: run go mod tidy --------- Co-authored-by: Joe L <[email protected]>

jo3-l added 3 commits May 21, 2024 18:28

reminders: add database schemas

cf61f4a

reminders: set up and run sqlboiler

6753d8d

jo3-l commented May 22, 2024

View reviewed changes

jo3-l mentioned this pull request May 22, 2024

schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible #1650

Merged

reminders: auto-delete old entries with null guild_id as necessary

7411253

jo3-l changed the title ~~reminders: switch away from gorm to sqlboiler~~ wip: reminders: switch away from gorm to sqlboiler May 24, 2024

jo3-l changed the title ~~wip: reminders: switch away from gorm to sqlboiler~~ wip: reminders: migrate away from gorm to sqlboiler May 24, 2024

jo3-l marked this pull request as ready for review May 24, 2024 19:29

jo3-l changed the title ~~wip: reminders: migrate away from gorm to sqlboiler~~ reminders: migrate away from gorm to sqlboiler May 24, 2024

ashishjh-bst merged commit 5813655 into botlabs-gg:gorm-to-sqlboiler May 28, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reminders: migrate away from gorm to sqlboiler #1652

reminders: migrate away from gorm to sqlboiler #1652

jo3-l commented May 22, 2024 •

edited

Loading

jo3-l May 22, 2024

jo3-l May 22, 2024

jo3-l May 22, 2024

jo3-l May 22, 2024

jo3-l May 22, 2024

jo3-l commented May 24, 2024

reminders: migrate away from gorm to sqlboiler #1652

reminders: migrate away from gorm to sqlboiler #1652

Conversation

jo3-l commented May 22, 2024 • edited Loading

On NOT NULL constraints

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l May 22, 2024

Choose a reason for hiding this comment

jo3-l commented May 24, 2024

jo3-l commented May 22, 2024 •

edited

Loading

On `NOT NULL` constraints