Repository: apigee/registry
Pull request #350 (2021-10-14 00:24:51)

Ensure the existence of tables on server creation only.

timburks: @seaneganx updated - PTAL

Patch:
diff --git a/cmd/registry-server/main.go b/cmd/registry-server/main.go
index 55c0f9e..6fd119e 100644
--- a/cmd/registry-server/main.go
+++ b/cmd/registry-server/main.go
@@ -120,7 +120,7 @@ func main() {
 	}
 	defer listener.Close()
 
-	registryServer := registry.New(registry.Config{
+	registryServer, err := registry.New(registry.Config{
 		Database:  config.Database.Driver,
 		DBConfig:  config.Database.Config,
 		LogLevel:  config.Logging.Level,
@@ -128,6 +128,9 @@ func main() {
 		Notify:    config.Pubsub.Enable,
 		ProjectID: config.Pubsub.Project,
 	})
+	if err != nil {
+		log.Fatalf("Failed to create registry server: %s", err)
+	}
 
 	var (
 		logInterceptor = grpc.UnaryInterceptor(registryServer.LoggingInterceptor)
diff --git a/server/registry/internal/storage/gorm/client.go b/server/registry/internal/storage/gorm/client.go
index c7330f5..49418ff 100644
--- a/server/registry/internal/storage/gorm/client.go
+++ b/server/registry/internal/storage/gorm/client.go
@@ -68,9 +68,7 @@ func NewClient(ctx context.Context, driver, dsn string) (*Client, error) {
 		// empirically, it does not seem safe to disable the mutex for sqlite3,
 		// which might make sense since sqlite database access is in-process.
 		disableMutex = false
-		c := &Client{db: db}
-		c.ensure()
-		return c, nil
+		return &Client{db: db}, nil
 	case "postgres", "cloudsqlpostgres":
 		db, err := gorm.Open(postgres.New(postgres.Config{
 			DriverName: driver,
@@ -88,9 +86,7 @@ func NewClient(ctx context.Context, driver, dsn string) (*Client, error) {
 		// postgres runs in a separate process and seems to have no problems
 		// with concurrent access and modifications.
 		disableMutex = true
-		c := &Client{db: db}
-		c.ensure()
-		return c, nil
+		return &Client{db: db}, nil
 	default:
 		unlock()
 		return nil, fmt.Errorf("unsupported database %s", driver)
@@ -109,24 +105,36 @@ func (c *Client) close() {
 	sqlDB.Close()
 }
 
-func (c *Client) ensureTable(v interface{}) {
+func (c *Client) ensureTable(v interface{}) error {
 	lock()
 	defer unlock()
 	if !c.db.Migrator().HasTable(v) {
-		_ = c.db.Migrator().CreateTable(v)
+		if err := c.db.Migrator().CreateTable(v); err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
-func (c *Client) ensure() {
-	c.ensureTable(&models.Project{})
-	c.ensureTable(&models.Api{})
-	c.ensureTable(&models.Version{})
-	c.ensureTable(&models.Spec{})
-	c.ensureTable(&models.SpecRevisionTag{})
-	c.ensureTable(&models.Deployment{})
-	c.ensureTable(&models.DeploymentRevisionTag{})
-	c.ensureTable(&models.Artifact{})
-	c.ensureTable(&models.Blob{})
+// EnsureTables ensures that all necessary tables exist in the database.
+func (c *Client) EnsureTables() error {
+	entities := []interface{}{
+		&models.Project{},
+		&models.Api{},
+		&models.Version{},
+		&models.Spec{},
+		&models.SpecRevisionTag{},
+		&models.Deployment{},
+		&models.DeploymentRevisionTag{},
+		&models.Artifact{},
+		&models.Blob{},
+	}
+	for _, entity := range entities {
+		if err := c.ensureTable(entity); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // IsNotFound returns true if an error is due to an entity not being found.
diff --git a/server/registry/server.go b/server/registry/server.go
index 69fcbec..42ab546 100644
--- a/server/registry/server.go
+++ b/server/registry/server.go
@@ -56,7 +56,7 @@ type RegistryServer struct {
 	rpc.UnimplementedAdminServer
 }
 
-func New(config Config) *RegistryServer {
+func New(config Config) (*RegistryServer, error) {
 	s := &RegistryServer{
 		database:      config.Database,
 		dbConfig:      config.DBConfig,
@@ -71,7 +71,15 @@ func New(config Config) *RegistryServer {
 		s.dbConfig = "/tmp/registry.db"
 	}
 
-	return s
+	db, err := storage.NewClient(context.Background(), s.database, s.dbConfig)
+	if err != nil {
+		return nil, err
+	}
+	defer db.Close()
+	if err := db.EnsureTables(); err != nil {
+		return nil, err
+	}
+	return s, nil
 }
 
 func (s *RegistryServer) getStorageClient(ctx context.Context) (*storage.Client, error) {


Test patch:
diff --git a/server/registry/internal/storage/gorm/gorm_test.go b/server/registry/internal/storage/gorm/gorm_test.go
index d699fcf..6080e78 100644
--- a/server/registry/internal/storage/gorm/gorm_test.go
+++ b/server/registry/internal/storage/gorm/gorm_test.go
@@ -33,6 +33,9 @@ func TestFieldClearing(t *testing.T) {
 		t.Fatalf("NewClient returned error: %s", err)
 	}
 	defer c.Close()
+	if err := c.EnsureTables(); err != nil {
+		t.Fatalf("EnsureTables returned error: %s", err)
+	}
 
 	original := &models.Project{
 		ProjectID:   "my-project",
@@ -66,8 +69,14 @@ func TestFieldClearing(t *testing.T) {
 func TestCRUD(t *testing.T) {
 	ctx := context.Background()
 
-	c, _ := NewClient(ctx, "sqlite3", t.TempDir()+"/testing.db")
+	c, err := NewClient(ctx, "sqlite3", t.TempDir()+"/testing.db")
+	if err != nil {
+		t.Fatalf("NewClient returned error: %s", err)
+	}
 	defer c.Close()
+	if err := c.EnsureTables(); err != nil {
+		t.Fatalf("EnsureTables returned error: %s", err)
+	}
 
 	now := time.Now()
 	project := &models.Project{
@@ -80,8 +89,7 @@ func TestCRUD(t *testing.T) {
 	k := c.NewKey("Project", "projects/demo")
 
 	// Create a project.
-	_, err := c.Put(ctx, k, project)
-	if err != nil {
+	if _, err := c.Put(ctx, k, project); err != nil {
 		t.Errorf(err.Error())
 	}
 
@@ -127,8 +135,14 @@ func TestLoad(t *testing.T) {
 	ctx := context.Background()
 
 	db := t.TempDir() + "/testing.db"
-	c, _ := NewClient(ctx, "sqlite3", db)
-	c.Close()
+	c, err := NewClient(ctx, "sqlite3", db)
+	if err != nil {
+		t.Fatalf("NewClient returned error: %s", err)
+	}
+	defer c.Close()
+	if err := c.EnsureTables(); err != nil {
+		t.Fatalf("EnsureTables returned error: %s", err)
+	}
 
 	for i := 0; i < 99; i++ {
 		c, err := NewClient(ctx, "sqlite3", db)
diff --git a/server/registry/server_test.go b/server/registry/server_test.go
index 597048b..d2cc5dd 100644
--- a/server/registry/server_test.go
+++ b/server/registry/server_test.go
@@ -43,20 +43,28 @@ func defaultTestServer(t *testing.T) *RegistryServer {
 	t.Helper()
 
 	if !usePostgres {
-		return serverWithSQLite(t)
+		if server, err := serverWithSQLite(t); err != nil {
+			t.Fatalf("Setup: failed to get server with SQLite: %s", err)
+		} else {
+			return server
+		}
 	}
 
 	server, err := serverWithPostgres(t)
 	if err != nil {
 		t.Errorf("Setup: failed to get server with postgres: %s", err)
 		t.Log("Falling back to server with SQLite storage")
-		return serverWithSQLite(t)
+		if server, err := serverWithSQLite(t); err != nil {
+			t.Fatalf("Setup: failed to get server with SQLite: %s", err)
+		} else {
+			return server
+		}
 	}
 
 	return server
 }
 
-func serverWithSQLite(t *testing.T) *RegistryServer {
+func serverWithSQLite(t *testing.T) (*RegistryServer, error) {
 	return New(Config{
 		Database: "sqlite3",
 		DBConfig: fmt.Sprintf("%s/registry.db", t.TempDir()),
@@ -74,7 +82,7 @@ func serverWithPostgres(t *testing.T) (*RegistryServer, error) {
 	return New(Config{
 		Database: postgresDriver,
 		DBConfig: postgresDBConfig,
-	}), nil
+	})
 }
 
 func resetPostgres() error {