From 21cf3c47f9235279d136367112116ee41927e08b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 26 Sep 2024 23:03:43 +0200 Subject: [PATCH] Manager: remove GORM annotations and last dependencies Remove GORM struct annotations/tags and references to GORM types. Ref: #104305 --- internal/manager/persistence/db.go | 4 +- internal/manager/persistence/errors.go | 23 ++---- internal/manager/persistence/errors_test.go | 10 +-- internal/manager/persistence/integrity.go | 5 -- internal/manager/persistence/jobs.go | 70 ++++++++----------- .../manager/persistence/jobs_blocklist.go | 10 +-- internal/manager/persistence/last_rendered.go | 4 +- internal/manager/persistence/sqlite_busy.go | 8 --- .../persistence/worker_sleep_schedule.go | 12 ++-- internal/manager/persistence/worker_tag.go | 8 +-- internal/manager/persistence/workers.go | 31 ++++---- 11 files changed, 71 insertions(+), 114 deletions(-) diff --git a/internal/manager/persistence/db.go b/internal/manager/persistence/db.go index 709c8543..094236cd 100644 --- a/internal/manager/persistence/db.go +++ b/internal/manager/persistence/db.go @@ -29,7 +29,7 @@ type DB struct { // Soft deletion is not used by Flamenco. If it ever becomes necessary to // support soft-deletion, see https://gorm.io/docs/delete.html#Soft-Delete type Model struct { - ID uint `gorm:"primarykey"` + ID uint CreatedAt time.Time UpdatedAt time.Time } @@ -167,8 +167,6 @@ type queriesTX struct { } // queries returns the SQLC Queries struct, connected to this database. -// It is intended that all GORM queries will be migrated to use this interface -// instead. // // After calling this function, all queries should use this transaction until it // is closed (either committed or rolled back). Otherwise SQLite will deadlock, diff --git a/internal/manager/persistence/errors.go b/internal/manager/persistence/errors.go index 18360799..ff94a5c4 100644 --- a/internal/manager/persistence/errors.go +++ b/internal/manager/persistence/errors.go @@ -5,16 +5,13 @@ import ( "database/sql" "errors" "fmt" - - "gorm.io/gorm" ) var ( - // TODO: let these errors wrap database/sql.ErrNoRows. - ErrJobNotFound = PersistenceError{Message: "job not found", Err: gorm.ErrRecordNotFound} - ErrTaskNotFound = PersistenceError{Message: "task not found", Err: gorm.ErrRecordNotFound} - ErrWorkerNotFound = PersistenceError{Message: "worker not found", Err: gorm.ErrRecordNotFound} - ErrWorkerTagNotFound = PersistenceError{Message: "worker tag not found", Err: gorm.ErrRecordNotFound} + ErrJobNotFound = PersistenceError{Message: "job not found", Err: sql.ErrNoRows} + ErrTaskNotFound = PersistenceError{Message: "task not found", Err: sql.ErrNoRows} + ErrWorkerNotFound = PersistenceError{Message: "worker not found", Err: sql.ErrNoRows} + ErrWorkerTagNotFound = PersistenceError{Message: "worker tag not found", Err: sql.ErrNoRows} ErrDeletingWithoutFK = errors.New("refusing to delete a job when foreign keys are not enabled on the database") @@ -80,9 +77,6 @@ func wrapError(errorToWrap error, message string, format ...interface{}) error { // This helps to keep Gorm as "implementation detail" of the persistence layer. func translateGormJobError(err error) error { if errors.Is(err, sql.ErrNoRows) { - return ErrTaskNotFound - } - if errors.Is(err, gorm.ErrRecordNotFound) { return ErrJobNotFound } return err @@ -94,9 +88,6 @@ func translateGormTaskError(err error) error { if errors.Is(err, sql.ErrNoRows) { return ErrTaskNotFound } - if errors.Is(err, gorm.ErrRecordNotFound) { - return ErrTaskNotFound - } return err } @@ -106,9 +97,6 @@ func translateGormWorkerError(err error) error { if errors.Is(err, sql.ErrNoRows) { return ErrWorkerNotFound } - if errors.Is(err, gorm.ErrRecordNotFound) { - return ErrWorkerNotFound - } return err } @@ -118,8 +106,5 @@ func translateGormWorkerTagError(err error) error { if errors.Is(err, sql.ErrNoRows) { return ErrWorkerTagNotFound } - if errors.Is(err, gorm.ErrRecordNotFound) { - return ErrWorkerTagNotFound - } return err } diff --git a/internal/manager/persistence/errors_test.go b/internal/manager/persistence/errors_test.go index 4a3f7d70..3a6c3c69 100644 --- a/internal/manager/persistence/errors_test.go +++ b/internal/manager/persistence/errors_test.go @@ -2,16 +2,16 @@ package persistence import ( + "database/sql" "errors" "testing" "github.com/stretchr/testify/assert" - "gorm.io/gorm" ) func TestNotFoundErrors(t *testing.T) { - assert.ErrorIs(t, ErrJobNotFound, gorm.ErrRecordNotFound) - assert.ErrorIs(t, ErrTaskNotFound, gorm.ErrRecordNotFound) + assert.ErrorIs(t, ErrJobNotFound, sql.ErrNoRows) + assert.ErrorIs(t, ErrTaskNotFound, sql.ErrNoRows) assert.Contains(t, ErrJobNotFound.Error(), "job") assert.Contains(t, ErrTaskNotFound.Error(), "task") @@ -19,7 +19,7 @@ func TestNotFoundErrors(t *testing.T) { func TestTranslateGormJobError(t *testing.T) { assert.Nil(t, translateGormJobError(nil)) - assert.Equal(t, ErrJobNotFound, translateGormJobError(gorm.ErrRecordNotFound)) + assert.Equal(t, ErrJobNotFound, translateGormJobError(sql.ErrNoRows)) otherError := errors.New("this error is not special for this function") assert.Equal(t, otherError, translateGormJobError(otherError)) @@ -27,7 +27,7 @@ func TestTranslateGormJobError(t *testing.T) { func TestTranslateGormTaskError(t *testing.T) { assert.Nil(t, translateGormTaskError(nil)) - assert.Equal(t, ErrTaskNotFound, translateGormTaskError(gorm.ErrRecordNotFound)) + assert.Equal(t, ErrTaskNotFound, translateGormTaskError(sql.ErrNoRows)) otherError := errors.New("this error is not special for this function") assert.Equal(t, otherError, translateGormTaskError(otherError)) diff --git a/internal/manager/persistence/integrity.go b/internal/manager/persistence/integrity.go index e16ec45d..946797bc 100644 --- a/internal/manager/persistence/integrity.go +++ b/internal/manager/persistence/integrity.go @@ -148,11 +148,6 @@ func (db *DB) pragmaForeignKeyCheck(ctx context.Context) (ok bool) { // ensureForeignKeysEnabled checks whether foreign keys are enabled, and if not, // tries to enable them. -// -// This is likely caused by either GORM or its embedded SQLite creating a new -// connection to the low-level SQLite driver. Unfortunately the GORM-embedded -// SQLite doesn't have an 'on-connect' callback function to always enable -// foreign keys. func (db *DB) ensureForeignKeysEnabled(ctx context.Context) { fkEnabled, err := db.areForeignKeysEnabled(ctx) diff --git a/internal/manager/persistence/jobs.go b/internal/manager/persistence/jobs.go index a6851c41..bff86e6c 100644 --- a/internal/manager/persistence/jobs.go +++ b/internal/manager/persistence/jobs.go @@ -13,7 +13,6 @@ import ( "time" "github.com/rs/zerolog/log" - "gorm.io/gorm" "projects.blender.org/studio/flamenco/internal/manager/job_compilers" "projects.blender.org/studio/flamenco/internal/manager/persistence/sqlc" @@ -22,23 +21,23 @@ import ( type Job struct { Model - UUID string `gorm:"type:char(36);default:'';unique;index"` + UUID string - Name string `gorm:"type:varchar(64);default:''"` - JobType string `gorm:"type:varchar(32);default:''"` - Priority int `gorm:"type:smallint;default:0"` - Status api.JobStatus `gorm:"type:varchar(32);default:''"` - Activity string `gorm:"type:varchar(255);default:''"` + Name string + JobType string + Priority int + Status api.JobStatus + Activity string - Settings StringInterfaceMap `gorm:"type:jsonb"` - Metadata StringStringMap `gorm:"type:jsonb"` + Settings StringInterfaceMap + Metadata StringStringMap DeleteRequestedAt sql.NullTime - Storage JobStorageInfo `gorm:"embedded;embeddedPrefix:storage_"` + Storage JobStorageInfo WorkerTagID *uint - WorkerTag *WorkerTag `gorm:"foreignkey:WorkerTagID;references:ID;constraint:OnDelete:SET NULL"` + WorkerTag *WorkerTag } type StringInterfaceMap map[string]interface{} @@ -54,43 +53,32 @@ func (j *Job) DeleteRequested() bool { // files. type JobStorageInfo struct { // ShamanCheckoutID is only set when the job was actually using Shaman storage. - ShamanCheckoutID string `gorm:"type:varchar(255);default:''"` + ShamanCheckoutID string } type Task struct { Model - UUID string `gorm:"type:char(36);default:'';unique;index"` + UUID string - Name string `gorm:"type:varchar(64);default:''"` - Type string `gorm:"type:varchar(32);default:''"` - JobID uint `gorm:"default:0"` - Job *Job `gorm:"foreignkey:JobID;references:ID;constraint:OnDelete:CASCADE"` - JobUUID string `gorm:"-"` // Fetched by SQLC, handled by GORM in Task.AfterFind() - Priority int `gorm:"type:smallint;default:50"` - Status api.TaskStatus `gorm:"type:varchar(16);default:''"` + Name string + Type string + JobID uint + Job *Job + JobUUID string // Fetched by SQLC, handled by GORM in Task.AfterFind() + Priority int + Status api.TaskStatus // Which worker is/was working on this. WorkerID *uint - Worker *Worker `gorm:"foreignkey:WorkerID;references:ID;constraint:OnDelete:SET NULL"` - WorkerUUID string `gorm:"-"` // Fetched by SQLC, handled by GORM in Task.AfterFind() - LastTouchedAt time.Time `gorm:"index"` // Should contain UTC timestamps. + Worker *Worker + WorkerUUID string // Fetched by SQLC, handled by GORM in Task.AfterFind() + LastTouchedAt time.Time // Should contain UTC timestamps. // Dependencies are tasks that need to be completed before this one can run. - Dependencies []*Task `gorm:"many2many:task_dependencies;constraint:OnDelete:CASCADE"` + Dependencies []*Task - Commands Commands `gorm:"type:jsonb"` - Activity string `gorm:"type:varchar(255);default:''"` -} - -// AfterFind updates the task JobUUID and WorkerUUID fields from its job/worker, if known. -func (t *Task) AfterFind(tx *gorm.DB) error { - if t.JobUUID == "" && t.Job != nil { - t.JobUUID = t.Job.UUID - } - if t.WorkerUUID == "" && t.Worker != nil { - t.WorkerUUID = t.Worker.UUID - } - return nil + Commands Commands + Activity string } type Commands []Command @@ -138,10 +126,10 @@ type TaskFailure struct { // Don't include the standard Gorm ID, UpdatedAt, or DeletedAt fields, as they're useless here. // Entries will never be updated, and should never be soft-deleted but just purged from existence. CreatedAt time.Time - TaskID uint `gorm:"primaryKey;autoIncrement:false"` - Task *Task `gorm:"foreignkey:TaskID;references:ID;constraint:OnDelete:CASCADE"` - WorkerID uint `gorm:"primaryKey;autoIncrement:false"` - Worker *Worker `gorm:"foreignkey:WorkerID;references:ID;constraint:OnDelete:CASCADE"` + TaskID uint + Task *Task + WorkerID uint + Worker *Worker } // StoreJob stores an AuthoredJob and its tasks, and saves it to the database. diff --git a/internal/manager/persistence/jobs_blocklist.go b/internal/manager/persistence/jobs_blocklist.go index 925e6c02..0ff531f3 100644 --- a/internal/manager/persistence/jobs_blocklist.go +++ b/internal/manager/persistence/jobs_blocklist.go @@ -17,13 +17,13 @@ type JobBlock struct { ID uint CreatedAt time.Time - JobID uint `gorm:"default:0;uniqueIndex:job_worker_tasktype"` - Job *Job `gorm:"foreignkey:JobID;references:ID;constraint:OnDelete:CASCADE"` + JobID uint + Job *Job - WorkerID uint `gorm:"default:0;uniqueIndex:job_worker_tasktype"` - Worker *Worker `gorm:"foreignkey:WorkerID;references:ID;constraint:OnDelete:CASCADE"` + WorkerID uint + Worker *Worker - TaskType string `gorm:"uniqueIndex:job_worker_tasktype"` + TaskType string } // AddWorkerToJobBlocklist prevents this Worker of getting any task, of this type, on this job, from the task scheduler. diff --git a/internal/manager/persistence/last_rendered.go b/internal/manager/persistence/last_rendered.go index 37238340..d545fde3 100644 --- a/internal/manager/persistence/last_rendered.go +++ b/internal/manager/persistence/last_rendered.go @@ -15,8 +15,8 @@ import ( // This is used to show the global last-rendered image in the web interface. type LastRendered struct { Model - JobID uint `gorm:"default:0"` - Job *Job `gorm:"foreignkey:JobID;references:ID;constraint:OnDelete:CASCADE"` + JobID uint + Job *Job } // SetLastRendered sets this job as the one with the most recent rendered image. diff --git a/internal/manager/persistence/sqlite_busy.go b/internal/manager/persistence/sqlite_busy.go index 1c202000..78301eea 100644 --- a/internal/manager/persistence/sqlite_busy.go +++ b/internal/manager/persistence/sqlite_busy.go @@ -26,14 +26,6 @@ func isDatabaseBusyError(err error) bool { if err == nil { return false } - - // The exact error type is dependent on deep dependencies of GORM. The code - // below used to work, until an upgrade of one of those dependencies. This is - // why I feel it's more future-proof to just check for SQLITE_BUSY in the - // error text. - // - // sqlErr, ok := err.(*sqlite.Error) - // return ok && sqlErr.Code() == sqlite_lib.SQLITE_BUSY return strings.Contains(err.Error(), "SQLITE_BUSY") } diff --git a/internal/manager/persistence/worker_sleep_schedule.go b/internal/manager/persistence/worker_sleep_schedule.go index 34ebe0a5..133fc470 100644 --- a/internal/manager/persistence/worker_sleep_schedule.go +++ b/internal/manager/persistence/worker_sleep_schedule.go @@ -18,16 +18,16 @@ import ( type SleepSchedule struct { Model - WorkerID uint `gorm:"default:0;unique;index"` - Worker *Worker `gorm:"foreignkey:WorkerID;references:ID;constraint:OnDelete:CASCADE"` + WorkerID uint + Worker *Worker - IsActive bool `gorm:"default:false;index"` + IsActive bool // Space-separated two-letter strings indicating days of week the schedule is // active ("mo", "tu", etc.). Empty means "every day". - DaysOfWeek string `gorm:"default:''"` - StartTime TimeOfDay `gorm:"default:''"` - EndTime TimeOfDay `gorm:"default:''"` + DaysOfWeek string + StartTime TimeOfDay + EndTime TimeOfDay NextCheck time.Time } diff --git a/internal/manager/persistence/worker_tag.go b/internal/manager/persistence/worker_tag.go index ba8220d7..10dbacb4 100644 --- a/internal/manager/persistence/worker_tag.go +++ b/internal/manager/persistence/worker_tag.go @@ -12,11 +12,11 @@ import ( type WorkerTag struct { Model - UUID string `gorm:"type:char(36);default:'';unique;index"` - Name string `gorm:"type:varchar(64);default:'';unique"` - Description string `gorm:"type:varchar(255);default:''"` + UUID string + Name string + Description string - Workers []*Worker `gorm:"many2many:worker_tag_membership;constraint:OnDelete:CASCADE"` + Workers []*Worker } func (db *DB) CreateWorkerTag(ctx context.Context, wc *WorkerTag) error { diff --git a/internal/manager/persistence/workers.go b/internal/manager/persistence/workers.go index 2fe37abd..93b76ec0 100644 --- a/internal/manager/persistence/workers.go +++ b/internal/manager/persistence/workers.go @@ -11,32 +11,31 @@ import ( "time" "github.com/rs/zerolog/log" - "gorm.io/gorm" "projects.blender.org/studio/flamenco/internal/manager/persistence/sqlc" "projects.blender.org/studio/flamenco/pkg/api" ) type Worker struct { Model - DeletedAt gorm.DeletedAt `gorm:"index"` + DeletedAt sql.NullTime - UUID string `gorm:"type:char(36);default:'';unique;index"` - Secret string `gorm:"type:varchar(255);default:''"` - Name string `gorm:"type:varchar(64);default:''"` + UUID string + Secret string + Name string - Address string `gorm:"type:varchar(39);default:'';index"` // 39 = max length of IPv6 address. - Platform string `gorm:"type:varchar(16);default:''"` - Software string `gorm:"type:varchar(32);default:''"` - Status api.WorkerStatus `gorm:"type:varchar(16);default:''"` - LastSeenAt time.Time `gorm:"index"` // Should contain UTC timestamps. - CanRestart bool `gorm:"type:smallint;default:false"` + Address string // 39 = max length of IPv6 address. + Platform string + Software string + Status api.WorkerStatus + LastSeenAt time.Time // Should contain UTC timestamps. + CanRestart bool - StatusRequested api.WorkerStatus `gorm:"type:varchar(16);default:''"` - LazyStatusRequest bool `gorm:"type:smallint;default:false"` + StatusRequested api.WorkerStatus + LazyStatusRequest bool - SupportedTaskTypes string `gorm:"type:varchar(255);default:''"` // comma-separated list of task types. + SupportedTaskTypes string // comma-separated list of task types. - Tags []*WorkerTag `gorm:"many2many:worker_tag_membership;constraint:OnDelete:CASCADE"` + Tags []*WorkerTag } func (w *Worker) Identifier() string { @@ -315,7 +314,7 @@ func convertSqlcWorker(worker sqlc.Worker) *Worker { CreatedAt: worker.CreatedAt, UpdatedAt: worker.UpdatedAt.Time, }, - DeletedAt: gorm.DeletedAt(worker.DeletedAt), + DeletedAt: worker.DeletedAt, UUID: worker.UUID, Secret: worker.Secret,