From c04e4992e03ac50f7398ad36b7d50b7ce8cbbd5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 11 Nov 2024 22:19:07 +0100 Subject: [PATCH] Transition from ex-GORM structs to sqlc structs (3/5) Replace old used-to-be-GORM datastructures (#104305) with sqlc-generated structs. This also makes it possible to use more specific structs that are more taylored to the specific queries, increasing efficiency. This commit deals with worker tags (on both workers and jobs). Functional changes are kept to a minimum, as the API still serves the same data. Because this work covers so much of Flamenco's code, it's been split up into different commits. Each commit brings Flamenco to a state where it compiles and unit tests pass. Only the result of the final commit has actually been tested properly. Ref: #104343 --- internal/manager/api_impl/interfaces.go | 4 +- internal/manager/api_impl/jobs_test.go | 4 +- .../api_impl/mocks/api_impl_mock.gen.go | 16 ++--- internal/manager/api_impl/worker_mgt.go | 19 ++---- internal/manager/api_impl/worker_mgt_test.go | 10 +-- .../manager/eventbus/events_workertags.go | 2 +- internal/manager/persistence/jobs.go | 4 +- internal/manager/persistence/jobs_test.go | 2 +- internal/manager/persistence/worker_tag.go | 61 ++++++------------- .../manager/persistence/worker_tag_test.go | 3 +- internal/manager/persistence/workers.go | 17 ------ internal/manager/persistence/workers_test.go | 5 -- 12 files changed, 47 insertions(+), 100 deletions(-) diff --git a/internal/manager/api_impl/interfaces.go b/internal/manager/api_impl/interfaces.go index 3404afab..18408e5b 100644 --- a/internal/manager/api_impl/interfaces.go +++ b/internal/manager/api_impl/interfaces.go @@ -72,8 +72,8 @@ type PersistenceService interface { // Worker tag management. WorkerSetTags(ctx context.Context, worker *persistence.Worker, tagUUIDs []string) error CreateWorkerTag(ctx context.Context, tag *persistence.WorkerTag) error - FetchWorkerTag(ctx context.Context, uuid string) (*persistence.WorkerTag, error) - FetchWorkerTags(ctx context.Context) ([]*persistence.WorkerTag, error) + FetchWorkerTag(ctx context.Context, uuid string) (persistence.WorkerTag, error) + FetchWorkerTags(ctx context.Context) ([]persistence.WorkerTag, error) DeleteWorkerTag(ctx context.Context, uuid string) error SaveWorkerTag(ctx context.Context, tag *persistence.WorkerTag) error FetchTagsOfWorker(ctx context.Context, workerUUID string) ([]persistence.WorkerTag, error) diff --git a/internal/manager/api_impl/jobs_test.go b/internal/manager/api_impl/jobs_test.go index b5134cab..c1344a28 100644 --- a/internal/manager/api_impl/jobs_test.go +++ b/internal/manager/api_impl/jobs_test.go @@ -330,7 +330,7 @@ func TestSubmitJobWithWorkerTag(t *testing.T) { workerTagUUID := "04435762-9dc8-4f13-80b7-643a6fa5b6fd" tag := persistence.WorkerTag{ - Model: persistence.Model{ID: 47}, + ID: 47, UUID: workerTagUUID, Name: "first tag", Description: "my first tag", @@ -383,7 +383,7 @@ func TestSubmitJobWithWorkerTag(t *testing.T) { Settings: persistence.StringInterfaceMap{}, Metadata: persistence.StringStringMap{}, - WorkerTagID: &tag.ID, + WorkerTagID: ptr(uint(tag.ID)), WorkerTag: &tag, } mf.persistence.EXPECT().FetchJob(gomock.Any(), queuedJob.JobID).Return(&dbJob, nil) diff --git a/internal/manager/api_impl/mocks/api_impl_mock.gen.go b/internal/manager/api_impl/mocks/api_impl_mock.gen.go index 6cca0d26..cb19a71e 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -144,7 +144,7 @@ func (mr *MockPersistenceServiceMockRecorder) CreateWorker(arg0, arg1 interface{ } // CreateWorkerTag mocks base method. -func (m *MockPersistenceService) CreateWorkerTag(arg0 context.Context, arg1 *persistence.WorkerTag) error { +func (m *MockPersistenceService) CreateWorkerTag(arg0 context.Context, arg1 *sqlc.WorkerTag) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "CreateWorkerTag", arg0, arg1) ret0, _ := ret[0].(error) @@ -231,10 +231,10 @@ func (mr *MockPersistenceServiceMockRecorder) FetchJobs(arg0 interface{}) *gomoc } // FetchTagsOfWorker mocks base method. -func (m *MockPersistenceService) FetchTagsOfWorker(arg0 context.Context, arg1 string) ([]persistence.WorkerTag, error) { +func (m *MockPersistenceService) FetchTagsOfWorker(arg0 context.Context, arg1 string) ([]sqlc.WorkerTag, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchTagsOfWorker", arg0, arg1) - ret0, _ := ret[0].([]persistence.WorkerTag) + ret0, _ := ret[0].([]sqlc.WorkerTag) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -306,10 +306,10 @@ func (mr *MockPersistenceServiceMockRecorder) FetchWorker(arg0, arg1 interface{} } // FetchWorkerTag mocks base method. -func (m *MockPersistenceService) FetchWorkerTag(arg0 context.Context, arg1 string) (*persistence.WorkerTag, error) { +func (m *MockPersistenceService) FetchWorkerTag(arg0 context.Context, arg1 string) (sqlc.WorkerTag, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchWorkerTag", arg0, arg1) - ret0, _ := ret[0].(*persistence.WorkerTag) + ret0, _ := ret[0].(sqlc.WorkerTag) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -321,10 +321,10 @@ func (mr *MockPersistenceServiceMockRecorder) FetchWorkerTag(arg0, arg1 interfac } // FetchWorkerTags mocks base method. -func (m *MockPersistenceService) FetchWorkerTags(arg0 context.Context) ([]*persistence.WorkerTag, error) { +func (m *MockPersistenceService) FetchWorkerTags(arg0 context.Context) ([]sqlc.WorkerTag, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchWorkerTags", arg0) - ret0, _ := ret[0].([]*persistence.WorkerTag) + ret0, _ := ret[0].([]sqlc.WorkerTag) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -466,7 +466,7 @@ func (mr *MockPersistenceServiceMockRecorder) SaveWorkerStatus(arg0, arg1 interf } // SaveWorkerTag mocks base method. -func (m *MockPersistenceService) SaveWorkerTag(arg0 context.Context, arg1 *persistence.WorkerTag) error { +func (m *MockPersistenceService) SaveWorkerTag(arg0 context.Context, arg1 *sqlc.WorkerTag) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "SaveWorkerTag", arg0, arg1) ret0, _ := ret[0].(error) diff --git a/internal/manager/api_impl/worker_mgt.go b/internal/manager/api_impl/worker_mgt.go index de185aac..3a525c60 100644 --- a/internal/manager/api_impl/worker_mgt.go +++ b/internal/manager/api_impl/worker_mgt.go @@ -336,9 +336,6 @@ func (f *Flamenco) FetchWorkerTag(e echo.Context, tagUUID string) error { logger.Error().Err(err).Msg("fetching worker tag") return sendAPIError(e, http.StatusInternalServerError, "error fetching worker tag: %v", err) } - if tag == nil { - panic("Could fetch a worker tag without error, but then the returned tag was still nil") - } return e.JSON(http.StatusOK, workerTagDBtoAPI(tag)) } @@ -388,7 +385,7 @@ func (f *Flamenco) UpdateWorkerTag(e echo.Context, tagUUID string) error { } logger = logCtx.Logger() - if err := f.persist.SaveWorkerTag(ctx, dbTag); err != nil { + if err := f.persist.SaveWorkerTag(ctx, &dbTag); err != nil { logger.Error().Err(err).Msg("saving worker tag") return sendAPIError(e, http.StatusInternalServerError, "error saving worker tag") } @@ -414,7 +411,7 @@ func (f *Flamenco) FetchWorkerTags(e echo.Context) error { apiTags := []api.WorkerTag{} for _, dbTag := range dbTags { apiTag := workerTagDBtoAPI(dbTag) - apiTags = append(apiTags, *apiTag) + apiTags = append(apiTags, apiTag) } tagList := api.WorkerTagList{ @@ -466,10 +463,10 @@ func (f *Flamenco) CreateWorkerTag(e echo.Context) error { logger.Info().Msg("created new worker tag") // SocketIO broadcast of tag creation. - sioUpdate := eventbus.NewWorkerTagUpdate(&dbTag) + sioUpdate := eventbus.NewWorkerTagUpdate(dbTag) f.broadcaster.BroadcastNewWorkerTag(sioUpdate) - return e.JSON(http.StatusOK, workerTagDBtoAPI(&dbTag)) + return e.JSON(http.StatusOK, workerTagDBtoAPI(dbTag)) } func workerSummary(w persistence.Worker) api.WorkerSummary { @@ -504,11 +501,7 @@ func workerDBtoAPI(w persistence.Worker) api.Worker { return apiWorker } -func workerTagDBtoAPI(wc *persistence.WorkerTag) *api.WorkerTag { - if wc == nil { - return nil - } - +func workerTagDBtoAPI(wc persistence.WorkerTag) api.WorkerTag { uuid := wc.UUID // Take a copy for safety. apiTag := api.WorkerTag{ @@ -518,5 +511,5 @@ func workerTagDBtoAPI(wc *persistence.WorkerTag) *api.WorkerTag { if len(wc.Description) > 0 { apiTag.Description = &wc.Description } - return &apiTag + return apiTag } diff --git a/internal/manager/api_impl/worker_mgt_test.go b/internal/manager/api_impl/worker_mgt_test.go index 366f7d9f..1384255d 100644 --- a/internal/manager/api_impl/worker_mgt_test.go +++ b/internal/manager/api_impl/worker_mgt_test.go @@ -307,7 +307,7 @@ func TestWorkerTagCRUDHappyFlow(t *testing.T) { assertResponseJSON(t, echo, http.StatusOK, &apiTag) // Fetch the tag - mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(&expectDBTag, nil) + mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(expectDBTag, nil) echo = mf.prepareMockedRequest(nil) require.NoError(t, mf.flamenco.FetchWorkerTag(echo, UUID)) assertResponseJSON(t, echo, http.StatusOK, &apiTag) @@ -330,7 +330,7 @@ func TestWorkerTagCRUDHappyFlow(t *testing.T) { Description: apiTag.Description, }, }) - mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(&expectDBTag, nil) + mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(expectDBTag, nil) mf.persistence.EXPECT().SaveWorkerTag(gomock.Any(), &expectNewDBTag) echo = mf.prepareMockedJSONRequest(newAPITag) require.NoError(t, mf.flamenco.UpdateWorkerTag(echo, UUID)) @@ -353,7 +353,7 @@ func TestWorkerTagCRUDHappyFlow(t *testing.T) { Description: newAPITag.Description, }, }) - mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(&expectDBTag, nil) + mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(expectDBTag, nil) mf.persistence.EXPECT().SaveWorkerTag(gomock.Any(), &expectNewDBTag) echo = mf.prepareMockedJSONRequest(newAPITag) require.NoError(t, mf.flamenco.UpdateWorkerTag(echo, UUID)) @@ -376,14 +376,14 @@ func TestWorkerTagCRUDHappyFlow(t *testing.T) { Description: newAPITag.Description, }, }) - mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(&expectDBTag, nil) + mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(expectDBTag, nil) mf.persistence.EXPECT().SaveWorkerTag(gomock.Any(), &expectNewDBTag) echo = mf.prepareMockedJSONRequest(newAPITag) require.NoError(t, mf.flamenco.UpdateWorkerTag(echo, UUID)) assertResponseNoContent(t, echo) // Delete. - mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(&expectDBTag, nil) + mf.persistence.EXPECT().FetchWorkerTag(gomock.Any(), UUID).Return(expectDBTag, nil) mf.persistence.EXPECT().DeleteWorkerTag(gomock.Any(), UUID) mf.broadcaster.EXPECT().BroadcastWorkerTagUpdate(api.EventWorkerTagUpdate{ Tag: api.WorkerTag{Id: &UUID}, diff --git a/internal/manager/eventbus/events_workertags.go b/internal/manager/eventbus/events_workertags.go index cc581d00..c6bc9b55 100644 --- a/internal/manager/eventbus/events_workertags.go +++ b/internal/manager/eventbus/events_workertags.go @@ -11,7 +11,7 @@ import ( // NewWorkerTagUpdate returns a partial EventWorkerTagUpdate struct for the // given worker tag. It only fills in the fields that represent the current // state of the tag. -func NewWorkerTagUpdate(tag *persistence.WorkerTag) api.EventWorkerTagUpdate { +func NewWorkerTagUpdate(tag persistence.WorkerTag) api.EventWorkerTagUpdate { tagUpdate := api.EventWorkerTagUpdate{ Tag: api.WorkerTag{ Id: &tag.UUID, diff --git a/internal/manager/persistence/jobs.go b/internal/manager/persistence/jobs.go index 262bab7c..d70dc515 100644 --- a/internal/manager/persistence/jobs.go +++ b/internal/manager/persistence/jobs.go @@ -355,7 +355,7 @@ func (db *DB) FetchJob(ctx context.Context, jobUUID string) (*Job, error) { case err != nil: return nil, workerTagError(err, "fetching worker tag of job") } - gormJob.WorkerTag = workerTag + gormJob.WorkerTag = &workerTag } return &gormJob, nil @@ -384,7 +384,7 @@ func (db *DB) FetchJobs(ctx context.Context) ([]*Job, error) { case err != nil: return nil, workerTagError(err, "fetching worker tag of job") } - gormJob.WorkerTag = workerTag + gormJob.WorkerTag = &workerTag } gormJobs[index] = &gormJob diff --git a/internal/manager/persistence/jobs_test.go b/internal/manager/persistence/jobs_test.go index 06779b95..9dde4585 100644 --- a/internal/manager/persistence/jobs_test.go +++ b/internal/manager/persistence/jobs_test.go @@ -97,7 +97,7 @@ func TestStoreAuthoredJobWithWorkerTag(t *testing.T) { require.NotNil(t, fetchedJob) require.NotNil(t, fetchedJob.WorkerTagID) - assert.Equal(t, *fetchedJob.WorkerTagID, workerTag.ID) + assert.Equal(t, int64(*fetchedJob.WorkerTagID), workerTag.ID) require.NotNil(t, fetchedJob.WorkerTag) assert.Equal(t, fetchedJob.WorkerTag.Name, workerTag.Name) diff --git a/internal/manager/persistence/worker_tag.go b/internal/manager/persistence/worker_tag.go index aacddd0c..26e0efbd 100644 --- a/internal/manager/persistence/worker_tag.go +++ b/internal/manager/persistence/worker_tag.go @@ -9,32 +9,24 @@ import ( "projects.blender.org/studio/flamenco/internal/manager/persistence/sqlc" ) -type WorkerTag struct { - Model +type WorkerTag = sqlc.WorkerTag - UUID string - Name string - Description string - - Workers []*Worker -} - -func (db *DB) CreateWorkerTag(ctx context.Context, wc *WorkerTag) error { +func (db *DB) CreateWorkerTag(ctx context.Context, tag *WorkerTag) error { queries := db.queries() now := db.now() dbID, err := queries.CreateWorkerTag(ctx, sqlc.CreateWorkerTagParams{ CreatedAt: now, - UUID: wc.UUID, - Name: wc.Name, - Description: wc.Description, + UUID: tag.UUID, + Name: tag.Name, + Description: tag.Description, }) if err != nil { return fmt.Errorf("creating new worker tag: %w", err) } - wc.ID = uint(dbID) - wc.CreatedAt = now + tag.ID = dbID + tag.CreatedAt = now return nil } @@ -51,25 +43,25 @@ func (db *DB) HasWorkerTags(ctx context.Context) (bool, error) { return count > 0, nil } -func (db *DB) FetchWorkerTag(ctx context.Context, uuid string) (*WorkerTag, error) { +func (db *DB) FetchWorkerTag(ctx context.Context, uuid string) (WorkerTag, error) { queries := db.queries() workerTag, err := queries.FetchWorkerTagByUUID(ctx, uuid) if err != nil { - return nil, workerTagError(err, "fetching worker tag") + return WorkerTag{}, workerTagError(err, "fetching worker tag") } - return convertSqlcWorkerTag(workerTag), nil + return workerTag, nil } // fetchWorkerTagByID fetches the worker tag using the given database instance. -func fetchWorkerTagByID(ctx context.Context, queries *sqlc.Queries, id int64) (*WorkerTag, error) { +func fetchWorkerTagByID(ctx context.Context, queries *sqlc.Queries, id int64) (WorkerTag, error) { workerTag, err := queries.FetchWorkerTagByID(ctx, id) if err != nil { - return nil, workerTagError(err, "fetching worker tag") + return WorkerTag{}, workerTagError(err, "fetching worker tag") } - return convertSqlcWorkerTag(workerTag), nil + return workerTag, nil } func (db *DB) SaveWorkerTag(ctx context.Context, tag *WorkerTag) error { @@ -80,7 +72,7 @@ func (db *DB) SaveWorkerTag(ctx context.Context, tag *WorkerTag) error { UUID: tag.UUID, Name: tag.Name, Description: tag.Description, - WorkerTagID: int64(tag.ID), + WorkerTagID: tag.ID, }) if err != nil { return workerTagError(err, "saving worker tag") @@ -112,36 +104,26 @@ func (db *DB) DeleteWorkerTag(ctx context.Context, uuid string) error { return nil } -func (db *DB) FetchWorkerTags(ctx context.Context) ([]*WorkerTag, error) { +func (db *DB) FetchWorkerTags(ctx context.Context) ([]WorkerTag, error) { queries := db.queries() tags, err := queries.FetchWorkerTags(ctx) if err != nil { return nil, workerTagError(err, "fetching all worker tags") } - - gormTags := make([]*WorkerTag, len(tags)) - for index, tag := range tags { - gormTags[index] = convertSqlcWorkerTag(tag) - } - return gormTags, nil + return tags, nil } func (db *DB) fetchWorkerTagsWithUUID( ctx context.Context, queries *sqlc.Queries, tagUUIDs []string, -) ([]*WorkerTag, error) { +) ([]WorkerTag, error) { tags, err := queries.FetchWorkerTagsByUUIDs(ctx, tagUUIDs) if err != nil { return nil, workerTagError(err, "fetching all worker tags") } - - gormTags := make([]*WorkerTag, len(tags)) - for index, tag := range tags { - gormTags[index] = convertSqlcWorkerTag(tag) - } - return gormTags, nil + return tags, nil } func (db *DB) WorkerSetTags(ctx context.Context, worker *Worker, tagUUIDs []string) error { @@ -180,10 +162,5 @@ func (db *DB) FetchTagsOfWorker(ctx context.Context, workerUUID string) ([]Worke if err != nil { return nil, workerTagError(err, "fetching tags of worker %s", workerUUID) } - - gormTags := make([]WorkerTag, len(tags)) - for index, tag := range tags { - gormTags[index] = *convertSqlcWorkerTag(tag) - } - return gormTags, nil + return tags, nil } diff --git a/internal/manager/persistence/worker_tag_test.go b/internal/manager/persistence/worker_tag_test.go index 57fa2604..2ca612fb 100644 --- a/internal/manager/persistence/worker_tag_test.go +++ b/internal/manager/persistence/worker_tag_test.go @@ -19,7 +19,7 @@ func TestCreateFetchTag(t *testing.T) { // Test fetching non-existent tag fetchedTag, err := f.db.FetchWorkerTag(f.ctx, "7ee21bc8-ff1a-42d2-a6b6-cc4b529b189f") assert.ErrorIs(t, err, ErrWorkerTagNotFound) - assert.Nil(t, fetchedTag) + assert.Zero(t, fetchedTag) // New tag creation is already done in the workerTestFixtures() call. assert.NotNil(t, f.tag) @@ -32,7 +32,6 @@ func TestCreateFetchTag(t *testing.T) { assert.Equal(t, f.tag.UUID, fetchedTag.UUID) assert.Equal(t, f.tag.Name, fetchedTag.Name) assert.Equal(t, f.tag.Description, fetchedTag.Description) - assert.Zero(t, fetchedTag.Workers) } func TestFetchDeleteTags(t *testing.T) { diff --git a/internal/manager/persistence/workers.go b/internal/manager/persistence/workers.go index 589c7d16..b7c54f59 100644 --- a/internal/manager/persistence/workers.go +++ b/internal/manager/persistence/workers.go @@ -248,20 +248,3 @@ func (db *DB) SummarizeWorkerStatuses(ctx context.Context) (WorkerStatusCount, e return statusCounts, nil } - -// convertSqlcWorkerTag converts a worker tag from the SQLC-generated model to -// the model expected by the rest of the code. This is mostly in place to aid in -// the GORM to SQLC migration. It is intended that eventually the rest of the -// code will use the same SQLC-generated model. -func convertSqlcWorkerTag(tag sqlc.WorkerTag) *WorkerTag { - return &WorkerTag{ - Model: Model{ - ID: uint(tag.ID), - CreatedAt: tag.CreatedAt, - UpdatedAt: tag.UpdatedAt.Time, - }, - UUID: tag.UUID, - Name: tag.Name, - Description: tag.Description, - } -} diff --git a/internal/manager/persistence/workers_test.go b/internal/manager/persistence/workers_test.go index dc8fcb58..4a3cf52d 100644 --- a/internal/manager/persistence/workers_test.go +++ b/internal/manager/persistence/workers_test.go @@ -362,11 +362,6 @@ func TestDeleteWorkerWithTagAssigned(t *testing.T) { // Delete the Worker. require.NoError(t, f.db.DeleteWorker(f.ctx, f.worker.UUID)) - - // Check the Worker has been unassigned from the tag. - tag, err := f.db.FetchWorkerTag(f.ctx, f.tag.UUID) - require.NoError(t, err) - assert.Empty(t, tag.Workers) } func TestSummarizeWorkerStatuses(t *testing.T) {