Make provider class mandatory in DB (#3132)

Relates to #2845 Undo migration #35 after discussion with Ozz and Jakub this morning. We want the provider class to store the concrete type of the provider, this will be used by the ProviderFactory. After studying the code, there does not appear to be any situation in which this field will be null in the staging/prod DB.
mindersec · Apr 18, 2024 · 11da628 · 11da628
1 parent 9139c73
commit 11da628
Show file tree

Hide file tree

Showing 16 changed files with 49 additions and 26 deletions.
diff --git a/database/migrations/000053_provider_class_non_nullable.down.sql b/database/migrations/000053_provider_class_non_nullable.down.sql
@@ -0,0 +1,15 @@
+-- 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.
+
+ALTER TABLE providers ALTER COLUMN class DROP NOT NULL
diff --git a/database/migrations/000053_provider_class_non_nullable.up.sql b/database/migrations/000053_provider_class_non_nullable.up.sql
@@ -0,0 +1,16 @@
+-- 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.
+
+-- undo migration #35
+ALTER TABLE providers ALTER COLUMN class SET NOT NULL
diff --git a/database/mock/store.go b/database/mock/store.go
diff --git a/internal/controlplane/handlers_oauth.go b/internal/controlplane/handlers_oauth.go
@@ -534,7 +534,7 @@ func (s *Server) VerifyProviderTokenFrom(ctx context.Context,
 		}
 		for _, p := range prov {
 			// Recently-created, let's use it.
-			if p.Class.ProviderClass == db.ProviderClassGithubApp && p.CreatedAt.After(in.GetTimestamp().AsTime()) {
+			if p.Class == db.ProviderClassGithubApp && p.CreatedAt.After(in.GetTimestamp().AsTime()) {
 				return &pb.VerifyProviderTokenFromResponse{Status: "OK"}, nil
 			}
 		}

diff --git a/internal/controlplane/handlers_providers.go b/internal/controlplane/handlers_providers.go
@@ -66,7 +66,7 @@ func (s *Server) GetProvider(ctx context.Context, req *minderv1.GetProviderReque
 			AuthFlows:        protobufProviderAuthFlowFromDB(ctx, *prov),
 			Config:           cfg,
 			CredentialsState: providers.GetCredentialStateForProvider(ctx, *prov, s.store, s.cryptoEngine, &s.cfg.Provider),
-			Class:            providers.GetProviderClassString(*prov),
+			Class:            string(prov.Class),
 		},
 	}, nil
 }
@@ -129,7 +129,7 @@ func (s *Server) ListProviders(ctx context.Context, req *minderv1.ListProvidersR
 			AuthFlows:        protobufProviderAuthFlowFromDB(ctx, p),
 			CredentialsState: providers.GetCredentialStateForProvider(ctx, p, s.store, s.cryptoEngine, &s.cfg.Provider),
 			Config:           cfg,
-			Class:            providers.GetProviderClassString(p),
+			Class:            string(p.Class),
 		})
 	}
 

diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go
@@ -475,10 +475,10 @@ func (s *Server) inferProviderByOwner(ctx context.Context, owner string, project
 
 	slices.SortFunc(opts, func(a, b db.Provider) int {
 		// Sort GitHub OAuth provider after all GitHub App providers
-		if a.Class.ProviderClass == db.ProviderClassGithub && b.Class.ProviderClass == db.ProviderClassGithubApp {
+		if a.Class == db.ProviderClassGithub && b.Class == db.ProviderClassGithubApp {
 			return 1
 		}
-		if a.Class.ProviderClass == db.ProviderClassGithubApp && b.Class.ProviderClass == db.ProviderClassGithub {
+		if a.Class == db.ProviderClassGithubApp && b.Class == db.ProviderClassGithub {
 			return -1
 		}
 		return 0

diff --git a/internal/db/models.go b/internal/db/models.go
diff --git a/internal/db/providers.sql.go b/internal/db/providers.sql.go
diff --git a/internal/db/providers_test.go b/internal/db/providers_test.go
@@ -36,7 +36,7 @@ func createRandomProvider(t *testing.T, projectID uuid.UUID) Provider {
 	prov, err := testQueries.CreateProvider(context.Background(), CreateProviderParams{
 		Name:       rand.RandomName(seed),
 		ProjectID:  projectID,
-		Class:      NullProviderClass{ProviderClass: ProviderClassGithub, Valid: true},
+		Class:      ProviderClassGithub,
 		Implements: []ProviderType{ProviderTypeGithub, ProviderTypeGit},
 		AuthFlows:  []AuthorizationFlow{AuthorizationFlowUserInput},
 		Definition: json.RawMessage("{}"),

diff --git a/internal/db/querier.go b/internal/db/querier.go
diff --git a/internal/eea/eea_test.go b/internal/eea/eea_test.go
@@ -160,7 +160,7 @@ func createNeededEntities(ctx context.Context, t *testing.T, testQueries db.Stor
 	prov, err := testQueries.CreateProvider(ctx, db.CreateProviderParams{
 		Name:       providerName,
 		ProjectID:  proj.ID,
-		Class:      db.NullProviderClass{ProviderClass: db.ProviderClassGithub, Valid: true},
+		Class:      db.ProviderClassGithub,
 		Implements: []db.ProviderType{db.ProviderTypeRest},
 		AuthFlows:  []db.AuthorizationFlow{db.AuthorizationFlowUserInput},
 		Definition: json.RawMessage(`{}`),

diff --git a/internal/projects/create.go b/internal/projects/create.go
@@ -60,7 +60,7 @@ func ProvisionSelfEnrolledOAuthProject(
 	_, err = qtx.CreateProvider(ctx, db.CreateProviderParams{
 		Name:       github.Github,
 		ProjectID:  project.ID,
-		Class:      db.NullProviderClass{ProviderClass: db.ProviderClassGithub, Valid: true},
+		Class:      db.ProviderClassGithub,
 		Implements: github.Implements,
 		Definition: json.RawMessage(`{"github": {}}`),
 		AuthFlows:  github.AuthorizationFlows,

diff --git a/internal/providers/classes.go b/internal/providers/classes.go
@@ -16,14 +16,6 @@ package providers
 
 import "github.com/stacklok/minder/internal/db"
 
-// GetProviderClassString returns the string representation of the provider class.
-func GetProviderClassString(prov db.Provider) string {
-	if prov.Class.Valid {
-		return string(prov.Class.ProviderClass)
-	}
-	return ""
-}
-
 // ListProviderClasses returns a list of provider classes.
 func ListProviderClasses() []string {
 	return []string{

diff --git a/internal/providers/github/common.go b/internal/providers/github/common.go
@@ -931,7 +931,7 @@ func CanHandleOwner(_ context.Context, prov db.Provider, owner string) bool {
 	if prov.Name == fmt.Sprintf("%s-%s", db.ProviderClassGithubApp, owner) {
 		return true
 	}
-	if prov.Class.ProviderClass == db.ProviderClassGithub {
+	if prov.Class == db.ProviderClassGithub {
 		return true
 	}
 	return false

diff --git a/internal/providers/github/service/service.go b/internal/providers/github/service/service.go
@@ -151,7 +151,7 @@ func (p *ghProviderService) CreateGitHubOAuthProvider(
 		createdProvider, err := qtx.CreateProvider(ctx, db.CreateProviderParams{
 			Name:       providerName,
 			ProjectID:  stateData.ProjectID,
-			Class:      db.NullProviderClass{ProviderClass: providerClass, Valid: true},
+			Class:      providerClass,
 			Implements: providerDef.Traits,
 			Definition: json.RawMessage(`{"github": {}}`),
 			AuthFlows:  providerDef.AuthorizationFlows,
@@ -327,7 +327,7 @@ func createGitHubApp(
 	savedProvider, err := qtx.CreateProvider(ctx, db.CreateProviderParams{
 		Name:       fmt.Sprintf("%s-%s", db.ProviderClassGithubApp, installationOwner.GetLogin()),
 		ProjectID:  projectId,
-		Class:      db.NullProviderClass{ProviderClass: db.ProviderClassGithubApp, Valid: true},
+		Class:      db.ProviderClassGithubApp,
 		Implements: app.Implements,
 		Definition: json.RawMessage(`{"github-app": {}}`),
 		AuthFlows:  app.AuthorizationFlows,

diff --git a/internal/providers/github/service/service_test.go b/internal/providers/github/service/service_test.go
@@ -272,7 +272,7 @@ func TestProviderService_CreateGitHubAppProvider(t *testing.T) {
 	require.Equal(t, dbProv.ProjectID, dbproj.ID)
 	require.Equal(t, dbProv.AuthFlows, app.AuthorizationFlows)
 	require.Equal(t, dbProv.Implements, app.Implements)
-	require.Equal(t, dbProv.Class, db.NullProviderClass{ProviderClass: db.ProviderClassGithubApp, Valid: true})
+	require.Equal(t, dbProv.Class, db.ProviderClassGithubApp)
 	require.Contains(t, dbProv.Name, db.ProviderClassGithubApp)
 	require.Contains(t, dbProv.Name, accountLogin)
 
@@ -506,7 +506,7 @@ func TestProviderService_DeleteProvider(t *testing.T) {
 		db.CreateProviderParams{
 			Name:       rand.RandomName(seed),
 			ProjectID:  dbproj.ID,
-			Class:      db.NullProviderClass{ProviderClass: db.ProviderClassGithubApp, Valid: true},
+			Class:      db.ProviderClassGithubApp,
 			Implements: []db.ProviderType{db.ProviderTypeGithub, db.ProviderTypeGit},
 			AuthFlows:  []db.AuthorizationFlow{db.AuthorizationFlowUserInput},
 			Definition: json.RawMessage("{}"),