From 22600c4c1efc2b214cefe6efad0808265e26f78d Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 29 Aug 2024 13:37:06 +0300 Subject: [PATCH] Finally remove per-entity columns from EEA (#4305) Now that the queries aren't using it and the main logic actually uses the central entity instances table, we can drop these. Closes: https://github.com/stacklok/minder/issues/4169 Signed-off-by: Juan Antonio Osorio --- ...00103_eea_rm_per_entities_columns.down.sql | 29 +++++ .../000103_eea_rm_per_entities_columns.up.sql | 29 +++++ internal/db/entity_execution_lock.sql.go | 20 +--- internal/db/models.go | 28 ++--- internal/eea/eea.go | 61 +++------- internal/eea/eea_test.go | 113 ++++-------------- internal/eea/entities.go | 25 ++-- internal/engine/entities/entity_event.go | 13 +- 8 files changed, 133 insertions(+), 185 deletions(-) create mode 100644 database/migrations/000103_eea_rm_per_entities_columns.down.sql create mode 100644 database/migrations/000103_eea_rm_per_entities_columns.up.sql diff --git a/database/migrations/000103_eea_rm_per_entities_columns.down.sql b/database/migrations/000103_eea_rm_per_entities_columns.down.sql new file mode 100644 index 0000000000..f457a334d3 --- /dev/null +++ b/database/migrations/000103_eea_rm_per_entities_columns.down.sql @@ -0,0 +1,29 @@ +-- Copyright 2024 Stacklok, Inc +-- +-- Licensed under the Apache License, Version 2.0 (the "License"); +-- you may not use this file except in compliance with the License. +-- You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. + +BEGIN; + +ALTER TABLE entity_execution_lock ADD COLUMN IF NOT EXISTS repository_id UUID NOT NULL REFERENCES repositories(id) ON DELETE CASCADE; +ALTER TABLE entity_execution_lock ADD COLUMN IF NOT EXISTS artifact_id UUID REFERENCES artifacts(id) ON DELETE CASCADE; +ALTER TABLE entity_execution_lock ADD COLUMN IF NOT EXISTS pull_request_id UUID REFERENCES pull_requests(id) ON DELETE CASCADE; + +ALTER TABLE flush_cache ADD COLUMN IF NOT EXISTS repository_id UUID NOT NULL REFERENCES repositories(id) ON DELETE CASCADE; +ALTER TABLE flush_cache ADD COLUMN IF NOT EXISTS artifact_id UUID REFERENCES artifacts(id) ON DELETE CASCADE; +ALTER TABLE flush_cache ADD COLUMN IF NOT EXISTS pull_request_id UUID REFERENCES pull_requests(id) ON DELETE CASCADE; + +-- make project_id nullable +ALTER TABLE entity_execution_lock ALTER COLUMN project_id DROP NOT NULL; +ALTER TABLE flush_cache ALTER COLUMN project_id DROP NOT NULL; + +COMMIT; diff --git a/database/migrations/000103_eea_rm_per_entities_columns.up.sql b/database/migrations/000103_eea_rm_per_entities_columns.up.sql new file mode 100644 index 0000000000..b9eda420bf --- /dev/null +++ b/database/migrations/000103_eea_rm_per_entities_columns.up.sql @@ -0,0 +1,29 @@ +-- Copyright 2024 Stacklok, Inc +-- +-- Licensed under the Apache License, Version 2.0 (the "License"); +-- you may not use this file except in compliance with the License. +-- You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. + +BEGIN; + +ALTER TABLE entity_execution_lock DROP COLUMN IF EXISTS repository_id; +ALTER TABLE entity_execution_lock DROP COLUMN IF EXISTS artifact_id; +ALTER TABLE entity_execution_lock DROP COLUMN IF EXISTS pull_request_id; + +ALTER TABLE flush_cache DROP COLUMN IF EXISTS repository_id; +ALTER TABLE flush_cache DROP COLUMN IF EXISTS artifact_id; +ALTER TABLE flush_cache DROP COLUMN IF EXISTS pull_request_id; + +-- make project_id not nullable +ALTER TABLE entity_execution_lock ALTER COLUMN project_id SET NOT NULL; +ALTER TABLE flush_cache ALTER COLUMN project_id SET NOT NULL; + +COMMIT; diff --git a/internal/db/entity_execution_lock.sql.go b/internal/db/entity_execution_lock.sql.go index b83c1c4b0c..b2f21ebc78 100644 --- a/internal/db/entity_execution_lock.sql.go +++ b/internal/db/entity_execution_lock.sql.go @@ -22,7 +22,7 @@ INSERT INTO flush_cache( $3::UUID ) ON CONFLICT(entity_instance_id) DO NOTHING -RETURNING id, entity, repository_id, artifact_id, pull_request_id, queued_at, project_id, entity_instance_id +RETURNING id, entity, queued_at, project_id, entity_instance_id ` type EnqueueFlushParams struct { @@ -37,9 +37,6 @@ func (q *Queries) EnqueueFlush(ctx context.Context, arg EnqueueFlushParams) (Flu err := row.Scan( &i.ID, &i.Entity, - &i.RepositoryID, - &i.ArtifactID, - &i.PullRequestID, &i.QueuedAt, &i.ProjectID, &i.EntityInstanceID, @@ -50,7 +47,7 @@ func (q *Queries) EnqueueFlush(ctx context.Context, arg EnqueueFlushParams) (Flu const flushCache = `-- name: FlushCache :one DELETE FROM flush_cache WHERE entity_instance_id= $1 -RETURNING id, entity, repository_id, artifact_id, pull_request_id, queued_at, project_id, entity_instance_id +RETURNING id, entity, queued_at, project_id, entity_instance_id ` func (q *Queries) FlushCache(ctx context.Context, entityInstanceID uuid.UUID) (FlushCache, error) { @@ -59,9 +56,6 @@ func (q *Queries) FlushCache(ctx context.Context, entityInstanceID uuid.UUID) (F err := row.Scan( &i.ID, &i.Entity, - &i.RepositoryID, - &i.ArtifactID, - &i.PullRequestID, &i.QueuedAt, &i.ProjectID, &i.EntityInstanceID, @@ -70,7 +64,7 @@ func (q *Queries) FlushCache(ctx context.Context, entityInstanceID uuid.UUID) (F } const listFlushCache = `-- name: ListFlushCache :many -SELECT id, entity, repository_id, artifact_id, pull_request_id, queued_at, project_id, entity_instance_id FROM flush_cache +SELECT id, entity, queued_at, project_id, entity_instance_id FROM flush_cache ` func (q *Queries) ListFlushCache(ctx context.Context) ([]FlushCache, error) { @@ -85,9 +79,6 @@ func (q *Queries) ListFlushCache(ctx context.Context) ([]FlushCache, error) { if err := rows.Scan( &i.ID, &i.Entity, - &i.RepositoryID, - &i.ArtifactID, - &i.PullRequestID, &i.QueuedAt, &i.ProjectID, &i.EntityInstanceID, @@ -124,7 +115,7 @@ DO UPDATE SET locked_by = gen_random_uuid(), last_lock_time = NOW() WHERE entity_execution_lock.last_lock_time < (NOW() - ($4::TEXT || ' seconds')::interval) -RETURNING id, entity, locked_by, last_lock_time, repository_id, artifact_id, pull_request_id, project_id, entity_instance_id +RETURNING id, entity, locked_by, last_lock_time, project_id, entity_instance_id ` type LockIfThresholdNotExceededParams struct { @@ -152,9 +143,6 @@ func (q *Queries) LockIfThresholdNotExceeded(ctx context.Context, arg LockIfThre &i.Entity, &i.LockedBy, &i.LastLockTime, - &i.RepositoryID, - &i.ArtifactID, - &i.PullRequestID, &i.ProjectID, &i.EntityInstanceID, ) diff --git a/internal/db/models.go b/internal/db/models.go index e40f2822c1..36d3f83848 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -503,15 +503,12 @@ type Entitlement struct { } type EntityExecutionLock struct { - ID uuid.UUID `json:"id"` - Entity Entities `json:"entity"` - LockedBy uuid.UUID `json:"locked_by"` - LastLockTime time.Time `json:"last_lock_time"` - RepositoryID uuid.NullUUID `json:"repository_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - ProjectID uuid.NullUUID `json:"project_id"` - EntityInstanceID uuid.UUID `json:"entity_instance_id"` + ID uuid.UUID `json:"id"` + Entity Entities `json:"entity"` + LockedBy uuid.UUID `json:"locked_by"` + LastLockTime time.Time `json:"last_lock_time"` + ProjectID uuid.UUID `json:"project_id"` + EntityInstanceID uuid.UUID `json:"entity_instance_id"` } type EntityInstance struct { @@ -561,14 +558,11 @@ type Feature struct { } type FlushCache struct { - ID uuid.UUID `json:"id"` - Entity Entities `json:"entity"` - RepositoryID uuid.NullUUID `json:"repository_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - QueuedAt time.Time `json:"queued_at"` - ProjectID uuid.NullUUID `json:"project_id"` - EntityInstanceID uuid.UUID `json:"entity_instance_id"` + ID uuid.UUID `json:"id"` + Entity Entities `json:"entity"` + QueuedAt time.Time `json:"queued_at"` + ProjectID uuid.UUID `json:"project_id"` + EntityInstanceID uuid.UUID `json:"entity_instance_id"` } type LatestEvaluationStatus struct { diff --git a/internal/eea/eea.go b/internal/eea/eea.go index 525273f74c..10f235aac2 100644 --- a/internal/eea/eea.go +++ b/internal/eea/eea.go @@ -239,33 +239,8 @@ func (e *EEA) FlushAll(ctx context.Context) error { for _, cache := range caches { cache := cache - projectID := cache.ProjectID - - // ensure that the eiw has a project ID. - // If there is no projectID (older minder). Get it from a repo. - if !projectID.Valid { - if !cache.RepositoryID.Valid { - zerolog.Ctx(ctx).Info().Msg("No project nor repo ID provided in entry, skipping") - continue - } - r, err := e.querier.GetRepositoryByID(ctx, cache.RepositoryID.UUID) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - zerolog.Ctx(ctx).Info().Msg("No project found for repository, skipping") - continue - } - return fmt.Errorf("unable to look up project for repository %s: %w", - cache.RepositoryID.UUID, err) - } - - projectID = uuid.NullUUID{ - UUID: r.ProjectID, - Valid: true, - } - } - eiw, err := e.buildEntityWrapper(ctx, cache.Entity, - cache.RepositoryID, projectID.UUID, cache.ArtifactID, cache.PullRequestID) + cache.ProjectID, cache.EntityInstanceID) if err != nil && errors.Is(err, sql.ErrNoRows) { continue } else if err != nil { @@ -290,17 +265,16 @@ func (e *EEA) FlushAll(ctx context.Context) error { func (e *EEA) buildEntityWrapper( ctx context.Context, entity db.Entities, - repoID uuid.NullUUID, projID uuid.UUID, - artID, prID uuid.NullUUID, + entityID uuid.UUID, ) (*entities.EntityInfoWrapper, error) { switch entity { case db.EntitiesRepository: - return e.buildRepositoryInfoWrapper(ctx, repoID, projID) + return e.buildRepositoryInfoWrapper(ctx, entityID, projID) case db.EntitiesArtifact: - return e.buildArtifactInfoWrapper(ctx, repoID, projID, artID) + return e.buildArtifactInfoWrapper(ctx, entityID, projID) case db.EntitiesPullRequest: - return e.buildPullRequestInfoWrapper(ctx, repoID, projID, prID) + return e.buildPullRequestInfoWrapper(ctx, entityID, projID) case db.EntitiesBuildEnvironment, db.EntitiesRelease, db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild: return nil, fmt.Errorf("entity type %q not yet supported", entity) @@ -311,28 +285,27 @@ func (e *EEA) buildEntityWrapper( func (e *EEA) buildRepositoryInfoWrapper( ctx context.Context, - repoID uuid.NullUUID, + repoID uuid.UUID, projID uuid.UUID, ) (*entities.EntityInfoWrapper, error) { - providerID, r, err := getRepository(ctx, e.querier, projID, repoID.UUID) + providerID, r, err := getRepository(ctx, e.querier, projID, repoID) if err != nil { return nil, fmt.Errorf("error getting repository: %w", err) } return entities.NewEntityInfoWrapper(). WithRepository(r). - WithRepositoryID(repoID.UUID). + WithRepositoryID(repoID). WithProjectID(projID). WithProviderID(providerID), nil } func (e *EEA) buildArtifactInfoWrapper( ctx context.Context, - repoID uuid.NullUUID, + artID uuid.UUID, projID uuid.UUID, - artID uuid.NullUUID, ) (*entities.EntityInfoWrapper, error) { - providerID, a, err := artifacts.GetArtifact(ctx, e.querier, projID, artID.UUID) + providerID, a, err := artifacts.GetArtifact(ctx, e.querier, projID, artID) if err != nil { return nil, fmt.Errorf("error getting artifact with versions: %w", err) } @@ -340,29 +313,25 @@ func (e *EEA) buildArtifactInfoWrapper( eiw := entities.NewEntityInfoWrapper(). WithProjectID(projID). WithArtifact(a). - WithArtifactID(artID.UUID). + WithArtifactID(artID). WithProviderID(providerID) - if repoID.Valid { - eiw.WithRepositoryID(repoID.UUID) - } return eiw, nil } func (e *EEA) buildPullRequestInfoWrapper( ctx context.Context, - repoID uuid.NullUUID, + prID uuid.UUID, projID uuid.UUID, - prID uuid.NullUUID, ) (*entities.EntityInfoWrapper, error) { - providerID, pr, err := getPullRequest(ctx, e.querier, projID, repoID.UUID, prID.UUID) + providerID, repoID, pr, err := getPullRequest(ctx, e.querier, projID, prID) if err != nil { return nil, fmt.Errorf("error getting pull request: %w", err) } return entities.NewEntityInfoWrapper(). - WithRepositoryID(repoID.UUID). + WithRepositoryID(repoID). WithProjectID(projID). WithPullRequest(pr). - WithPullRequestID(prID.UUID). + WithPullRequestID(prID). WithProviderID(providerID), nil } diff --git a/internal/eea/eea_test.go b/internal/eea/eea_test.go index 3f9db48b29..20eb902d9e 100644 --- a/internal/eea/eea_test.go +++ b/internal/eea/eea_test.go @@ -212,21 +212,12 @@ func TestFlushAll(t *testing.T) { { ID: uuid.New(), Entity: db.EntitiesRepository, - RepositoryID: uuid.NullUUID{UUID: repoID, Valid: true}, QueuedAt: time.Now(), + ProjectID: projectID, EntityInstanceID: repoID, }, }, nil) - // 1 - fetch repo info for repo - // base repo info - mockStore.EXPECT().GetRepositoryByID(ctx, repoID). - Return(db.Repository{ - ID: repoID, - ProjectID: projectID, - Provider: providerName, - ProviderID: providerID, - }, nil) // subsequent repo fetch for protobuf conversion mockStore.EXPECT().GetRepositoryByIDAndProject(ctx, gomock.Any()). Return(db.Repository{ @@ -250,30 +241,14 @@ func TestFlushAll(t *testing.T) { mockStore.EXPECT().ListFlushCache(ctx). Return([]db.FlushCache{ { - ID: uuid.New(), - Entity: db.EntitiesArtifact, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - ArtifactID: uuid.NullUUID{ - UUID: artID, - Valid: true, - }, + ID: uuid.New(), + Entity: db.EntitiesArtifact, EntityInstanceID: artID, + ProjectID: projectID, QueuedAt: time.Now(), }, }, nil) - // 1 - fetch repo info for repo - // base repo info - mockStore.EXPECT().GetRepositoryByID(ctx, repoID). - Return(db.Repository{ - ID: repoID, - ProjectID: projectID, - Provider: providerName, - ProviderID: providerID, - }, nil) // subsequent artifact fetch for protobuf conversion mockStore.EXPECT().GetRepositoryByIDAndProject(ctx, gomock.Any()). Return(db.Repository{ @@ -305,21 +280,10 @@ func TestFlushAll(t *testing.T) { mockStore.EXPECT().ListFlushCache(ctx). Return([]db.FlushCache{ { - ID: uuid.New(), - Entity: db.EntitiesArtifact, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - ArtifactID: uuid.NullUUID{ - UUID: artID, - Valid: true, - }, - QueuedAt: time.Now(), - ProjectID: uuid.NullUUID{ - UUID: projectID, - Valid: true, - }, + ID: uuid.New(), + Entity: db.EntitiesArtifact, + QueuedAt: time.Now(), + ProjectID: projectID, EntityInstanceID: artID, }, }, nil) @@ -348,28 +312,16 @@ func TestFlushAll(t *testing.T) { { name: "flushes one artifact with no repo and a set project ID in the flush", mockDBSetup: func(ctx context.Context, mockStore *mockdb.MockStore) { - repoID := uuid.New() artID := uuid.New() projectID := uuid.New() mockStore.EXPECT().ListFlushCache(ctx). Return([]db.FlushCache{ { - ID: uuid.New(), - Entity: db.EntitiesArtifact, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - ArtifactID: uuid.NullUUID{ - UUID: artID, - Valid: true, - }, - QueuedAt: time.Now(), - ProjectID: uuid.NullUUID{ - UUID: projectID, - Valid: true, - }, + ID: uuid.New(), + Entity: db.EntitiesArtifact, + QueuedAt: time.Now(), + ProjectID: projectID, EntityInstanceID: artID, }, }, nil) @@ -396,29 +348,14 @@ func TestFlushAll(t *testing.T) { mockStore.EXPECT().ListFlushCache(ctx). Return([]db.FlushCache{ { - ID: uuid.New(), - Entity: db.EntitiesPullRequest, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - PullRequestID: uuid.NullUUID{ - UUID: prID, - Valid: true, - }, - QueuedAt: time.Now(), + ID: uuid.New(), + Entity: db.EntitiesPullRequest, + ProjectID: projectID, + EntityInstanceID: prID, + QueuedAt: time.Now(), }, }, nil) - // 1 - fetch repo info for repo - // base repo info - mockStore.EXPECT().GetRepositoryByID(ctx, repoID). - Return(db.Repository{ - ID: repoID, - ProjectID: projectID, - Provider: providerName, - ProviderID: providerID, - }, nil) // subsequent artifact fetch for protobuf conversion mockStore.EXPECT().GetRepositoryByIDAndProject(ctx, gomock.Any()). Return(db.Repository{ @@ -605,23 +542,21 @@ func TestFlushAllListFlushListsARepoThatGetsDeletedLater(t *testing.T) { }) repoID := uuid.New() + projID := uuid.New() // initial list flush mockStore.EXPECT().ListFlushCache(ctx). Return([]db.FlushCache{ { - ID: uuid.New(), - Entity: db.EntitiesRepository, - RepositoryID: uuid.NullUUID{ - UUID: repoID, - Valid: true, - }, - QueuedAt: time.Now(), + ID: uuid.New(), + Entity: db.EntitiesRepository, + ProjectID: projID, + EntityInstanceID: repoID, + QueuedAt: time.Now(), }, }, nil) - // repo does not exist - mockStore.EXPECT().GetRepositoryByID(ctx, repoID). + mockStore.EXPECT().GetRepositoryByIDAndProject(ctx, gomock.Any()). Return(db.Repository{}, sql.ErrNoRows) t.Log("Flushing all") diff --git a/internal/eea/entities.go b/internal/eea/entities.go index 89b56d3abb..7d47ef177c 100644 --- a/internal/eea/entities.go +++ b/internal/eea/entities.go @@ -57,29 +57,28 @@ func getPullRequest( ctx context.Context, store db.Querier, projectID, - repoID, pullRequestID uuid.UUID, -) (uuid.UUID, *minderv1.PullRequest, error) { - // Get repository data - we need the owner and name - dbrepo, err := store.GetRepositoryByIDAndProject(ctx, db.GetRepositoryByIDAndProjectParams{ - ID: repoID, - ProjectID: projectID, - }) +) (uuid.UUID, uuid.UUID, *minderv1.PullRequest, error) { + dbpr, err := store.GetPullRequestByID(ctx, pullRequestID) if errors.Is(err, sql.ErrNoRows) { - return uuid.Nil, nil, fmt.Errorf("repository not found") + return uuid.Nil, uuid.Nil, nil, fmt.Errorf("pull request not found") } else if err != nil { - return uuid.Nil, nil, fmt.Errorf("cannot read repository: %v", err) + return uuid.Nil, uuid.Nil, nil, fmt.Errorf("cannot read pull request: %v", err) } - dbpr, err := store.GetPullRequestByID(ctx, pullRequestID) + // Get repository data - we need the owner and name + dbrepo, err := store.GetRepositoryByIDAndProject(ctx, db.GetRepositoryByIDAndProjectParams{ + ID: dbpr.RepositoryID, + ProjectID: projectID, + }) if errors.Is(err, sql.ErrNoRows) { - return uuid.Nil, nil, fmt.Errorf("pull request not found") + return uuid.Nil, uuid.Nil, nil, fmt.Errorf("repository not found") } else if err != nil { - return uuid.Nil, nil, fmt.Errorf("cannot read pull request: %v", err) + return uuid.Nil, uuid.Nil, nil, fmt.Errorf("cannot read repository: %v", err) } // TODO: Do we need extra columns in the pull request table? - return dbrepo.ProviderID, &minderv1.PullRequest{ + return dbrepo.ProviderID, dbrepo.ID, &minderv1.PullRequest{ Context: &minderv1.Context{ Project: ptr.Ptr(dbrepo.ProjectID.String()), Provider: ptr.Ptr(dbrepo.Provider), diff --git a/internal/engine/entities/entity_event.go b/internal/engine/entities/entity_event.go index b70179bc3c..3670800a77 100644 --- a/internal/engine/entities/entity_event.go +++ b/internal/engine/entities/entity_event.go @@ -528,10 +528,7 @@ func ParseEntityEvent(msg *message.Message) (*EntityInfoWrapper, error) { return nil, err } - // We always have the repository ID. - if err := out.withRepositoryIDFromMessage(msg); err != nil { - return nil, err - } + // We don't always have repo ID (e.g. for artifacts) out.withActionEventFromMessage(msg) @@ -539,16 +536,24 @@ func ParseEntityEvent(msg *message.Message) (*EntityInfoWrapper, error) { switch typ { case RepositoryEventEntityType: out.AsRepository() + if err := out.withRepositoryIDFromMessage(msg); err != nil { + return nil, err + } case VersionedArtifactEventEntityType: out.AsArtifact() if err := out.withArtifactIDFromMessage(msg); err != nil { return nil, err } + //nolint:gosec // The repo is not always present + out.withRepositoryIDFromMessage(msg) case PullRequestEventEntityType: out.AsPullRequest() if err := out.withPullRequestIDFromMessage(msg); err != nil { return nil, err } + if err := out.withRepositoryIDFromMessage(msg); err != nil { + return nil, err + } default: return nil, fmt.Errorf("unknown entity type: %s", typ) }