Remove controller soft delete (#1229)

* Remove controller soft delete

* PR comments

* Test fixes

* Further test fixes

.github/workflows/ci.yaml

@@ -25,6 +25,7 @@ jobs:
   build_test:
     name: Build and Test
     runs-on: ubuntu-22.04
+    timeout-minutes: 45
     steps:
       - uses: actions/checkout@v4
         with:

internal/db/controller_test.go

@@ -4,14 +4,11 @@ package db_test
 
 import (
 	"context"
-	"database/sql"
 	"testing"
-	"time"
 
 	qt "github.com/frankban/quicktest"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	jujuparams "github.com/juju/juju/rpc/params"
-	"github.com/juju/juju/state"
 
 	"github.com/canonical/jimm/internal/db"
 	"github.com/canonical/jimm/internal/dbmodel"
@@ -72,7 +69,6 @@ func (s *dbSuite) TestGetController(c *qt.C) {
 	controller := dbmodel.Controller{
 		Name:      "test-controller",
 		UUID:      "00000000-0000-0000-0000-0000-0000000000001",
-		Models:    []dbmodel.Model{},
 		CloudName: "test-cloud",
 	}
 	err = s.Database.AddController(context.Background(), &controller)
@@ -102,105 +98,6 @@ func (s *dbSuite) TestGetController(c *qt.C) {
 	c.Assert(eError.Code, qt.Equals, errors.CodeNotFound)
 }
 
-func (s *dbSuite) TestGetControllerWithModels(c *qt.C) {
-	err := s.Database.Migrate(context.Background(), true)
-	c.Assert(err, qt.Equals, nil)
-
-	cloud := dbmodel.Cloud{
-		Name: "test-cloud",
-		Type: "test-provider",
-		Regions: []dbmodel.CloudRegion{{
-			Name: "test-region",
-		}},
-	}
-	c.Assert(s.Database.DB.Create(&cloud).Error, qt.IsNil)
-
-	controller := dbmodel.Controller{
-		Name:        "test-controller",
-		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
-		Models:      []dbmodel.Model{},
-		CloudName:   "test-cloud",
-		CloudRegion: "test-region",
-	}
-	u, err := dbmodel.NewIdentity("[email protected]")
-	c.Assert(err, qt.IsNil)
-	c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil)
-
-	cred := dbmodel.CloudCredential{
-		Name:     "test-cred",
-		Cloud:    cloud,
-		Owner:    *u,
-		AuthType: "empty",
-	}
-	c.Assert(s.Database.DB.Create(&cred).Error, qt.IsNil)
-
-	err = s.Database.AddController(context.Background(), &controller)
-	c.Assert(err, qt.Equals, nil)
-
-	models := []dbmodel.Model{{
-		Name: "test-model-1",
-		UUID: sql.NullString{
-			String: "00000001-0000-0000-0000-0000-000000000001",
-			Valid:  true,
-		},
-		Owner:           *u,
-		Controller:      controller,
-		CloudRegion:     cloud.Regions[0],
-		CloudCredential: cred,
-		Type:            "iaas",
-		IsController:    true,
-		DefaultSeries:   "warty",
-		Life:            state.Alive.String(),
-		Status: dbmodel.Status{
-			Status: "available",
-			Since: sql.NullTime{
-				Time:  time.Now(),
-				Valid: true,
-			},
-		},
-		SLA: dbmodel.SLA{
-			Level: "unsupported",
-		},
-	}, {
-		Name: "test-model-2",
-		UUID: sql.NullString{
-			String: "00000001-0000-0000-0000-0000-000000000002",
-			Valid:  true,
-		},
-		Owner:           *u,
-		Controller:      controller,
-		CloudRegion:     cloud.Regions[0],
-		CloudCredential: cred,
-		Type:            "iaas",
-		IsController:    false,
-		DefaultSeries:   "warty",
-		Life:            state.Alive.String(),
-		Status: dbmodel.Status{
-			Status: "available",
-			Since: sql.NullTime{
-				Time:  time.Now(),
-				Valid: true,
-			},
-		},
-		SLA: dbmodel.SLA{
-			Level: "unsupported",
-		},
-	}}
-	for _, m := range models {
-		c.Assert(s.Database.DB.Create(&m).Error, qt.IsNil)
-	}
-
-	dbController := dbmodel.Controller{
-		UUID: controller.UUID,
-	}
-	err = s.Database.GetController(context.Background(), &dbController)
-	c.Assert(err, qt.Equals, nil)
-	controller.Models = []dbmodel.Model{
-		models[0],
-	}
-	c.Assert(dbController, qt.CmpEquals(cmpopts.IgnoreFields(dbmodel.Controller{}, "Models"), cmpopts.EquateEmpty()), controller)
-}
-
 func TestForEachControllerUnconfiguredDatabase(t *testing.T) {
 	c := qt.New(t)
 
@@ -289,7 +186,6 @@ func (s *dbSuite) TestUpdateController(c *qt.C) {
 		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
 		CloudName:   "test-cloud",
 		CloudRegion: "test-region",
-		Models:      []dbmodel.Model{},
 	}
 	err = s.Database.AddController(context.Background(), &controller)
 	c.Assert(err, qt.Equals, nil)
@@ -336,7 +232,6 @@ func (s *dbSuite) TestDeleteController(c *qt.C) {
 		Name:      "test-controller",
 		UUID:      "00000000-0000-0000-0000-0000-0000000000001",
 		CloudName: "test-cloud",
-		Models:    []dbmodel.Model{},
 	}
 	err = s.Database.AddController(context.Background(), &controller)
 	c.Assert(err, qt.Equals, nil)

internal/db/model.go

@@ -181,3 +181,19 @@ func preloadModel(prefix string, db *gorm.DB) *gorm.DB {
 
 	return db
 }
+
+// GetModelsByController retrieves a list of models hosted on the specified controller.
+// Note that because we do not preload here, foreign key references will be empty.
+func (d *Database) GetModelsByController(ctx context.Context, ctl dbmodel.Controller) ([]dbmodel.Model, error) {
+	const op = errors.Op("db.GetModelsByController")
+
+	if err := d.ready(); err != nil {
+		return nil, errors.E(op, err)
+	}
+	var models []dbmodel.Model
+	db := d.DB.WithContext(ctx)
+	if err := db.Model(ctl).Association("Models").Find(&models); err != nil {
+		return nil, errors.E(op, dbError(err))
+	}
+	return models, nil
+}

internal/db/model_test.go

@@ -7,6 +7,7 @@ import (
 	"database/sql"
 	"sort"
 	"testing"
+	"time"
 
 	qt "github.com/frankban/quicktest"
 	"github.com/juju/juju/state"
@@ -57,7 +58,6 @@ func (s *dbSuite) TestAddModel(c *qt.C) {
 		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
 		CloudName:   "test-cloud",
 		CloudRegion: "test-region",
-		Models:      []dbmodel.Model{},
 	}
 	err = s.Database.AddController(context.Background(), &controller)
 	c.Assert(err, qt.Equals, nil)
@@ -127,7 +127,6 @@ func (s *dbSuite) TestGetModel(c *qt.C) {
 	controller := dbmodel.Controller{
 		Name:        "test-controller",
 		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
-		Models:      []dbmodel.Model{},
 		CloudName:   "test-cloud",
 		CloudRegion: "test-region",
 	}
@@ -231,7 +230,6 @@ func (s *dbSuite) TestUpdateModel(c *qt.C) {
 		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
 		CloudName:   "test-cloud",
 		CloudRegion: "test-region",
-		Models:      []dbmodel.Model{},
 	}
 	err = s.Database.AddController(context.Background(), &controller)
 	c.Assert(err, qt.Equals, nil)
@@ -308,7 +306,6 @@ func (s *dbSuite) TestDeleteModel(c *qt.C) {
 		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
 		CloudName:   "test-cloud",
 		CloudRegion: "test-region",
-		Models:      []dbmodel.Model{},
 	}
 	err = s.Database.AddController(context.Background(), &controller)
 	c.Assert(err, qt.Equals, nil)
@@ -641,3 +638,102 @@ func (s *dbSuite) TestGetModelsByUUID(c *qt.C) {
 	c.Check(models[2].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000003")
 	c.Check(models[2].Controller.Name, qt.Not(qt.Equals), "")
 }
+
+func (s *dbSuite) TestGetModelsByController(c *qt.C) {
+	err := s.Database.Migrate(context.Background(), true)
+	c.Assert(err, qt.Equals, nil)
+
+	cloud := dbmodel.Cloud{
+		Name: "test-cloud",
+		Type: "test-provider",
+		Regions: []dbmodel.CloudRegion{{
+			Name: "test-region",
+		}},
+	}
+	c.Assert(s.Database.DB.Create(&cloud).Error, qt.IsNil)
+
+	controller := dbmodel.Controller{
+		Name:        "test-controller",
+		UUID:        "00000000-0000-0000-0000-0000-0000000000001",
+		CloudName:   "test-cloud",
+		CloudRegion: "test-region",
+	}
+	u, err := dbmodel.NewIdentity("[email protected]")
+	c.Assert(err, qt.IsNil)
+	c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil)
+
+	cred := dbmodel.CloudCredential{
+		Name:     "test-cred",
+		Cloud:    cloud,
+		Owner:    *u,
+		AuthType: "empty",
+	}
+	c.Assert(s.Database.DB.Create(&cred).Error, qt.IsNil)
+
+	err = s.Database.AddController(context.Background(), &controller)
+	c.Assert(err, qt.Equals, nil)
+
+	models := []dbmodel.Model{{
+		Name: "test-model-1",
+		UUID: sql.NullString{
+			String: "00000001-0000-0000-0000-0000-000000000001",
+			Valid:  true,
+		},
+		Owner:           *u,
+		Controller:      controller,
+		CloudRegion:     cloud.Regions[0],
+		CloudCredential: cred,
+		Type:            "iaas",
+		IsController:    true,
+		DefaultSeries:   "focal",
+		Life:            state.Alive.String(),
+		Status: dbmodel.Status{
+			Status: "available",
+			Since: sql.NullTime{
+				Time:  time.Now(),
+				Valid: true,
+			},
+		},
+		SLA: dbmodel.SLA{
+			Level: "unsupported",
+		},
+	}, {
+		Name: "test-model-2",
+		UUID: sql.NullString{
+			String: "00000001-0000-0000-0000-0000-000000000002",
+			Valid:  true,
+		},
+		Owner:           *u,
+		Controller:      controller,
+		CloudRegion:     cloud.Regions[0],
+		CloudCredential: cred,
+		Type:            "iaas",
+		IsController:    false,
+		DefaultSeries:   "focal",
+		Life:            state.Alive.String(),
+		Status: dbmodel.Status{
+			Status: "available",
+			Since: sql.NullTime{
+				Time:  time.Now(),
+				Valid: true,
+			},
+		},
+		SLA: dbmodel.SLA{
+			Level: "unsupported",
+		},
+	}}
+	for _, m := range models {
+		c.Assert(s.Database.DB.Create(&m).Error, qt.IsNil)
+	}
+	foundModels, err := s.Database.GetModelsByController(context.Background(), controller)
+	foundModelNames := []string{}
+	for _, m := range foundModels {
+		foundModelNames = append(foundModelNames, m.Name)
+	}
+	modelNames := []string{}
+	for _, m := range models {
+		modelNames = append(modelNames, m.Name)
+	}
+	c.Assert(err, qt.IsNil)
+	c.Assert(foundModelNames, qt.DeepEquals, modelNames)
+}

internal/dbmodel/cloud_test.go

@@ -353,13 +353,17 @@ func TestCloudRegionControllers(t *testing.T) {
 	c.Assert(err, qt.IsNil)
 	c.Check(crcps, qt.HasLen, 2)
 	c.Check(crcps[0].Controller, qt.DeepEquals, dbmodel.Controller{
-		Model:       ctl2.Model,
+		ID:          ctl2.ID,
+		CreatedAt:   ctl2.CreatedAt,
+		UpdatedAt:   ctl2.UpdatedAt,
 		Name:        ctl2.Name,
 		CloudName:   ctl2.CloudName,
 		CloudRegion: ctl2.CloudRegion,
 	})
 	c.Check(crcps[1].Controller, qt.DeepEquals, dbmodel.Controller{
-		Model:       ctl1.Model,
+		ID:          ctl1.ID,
+		CreatedAt:   ctl1.CreatedAt,
+		UpdatedAt:   ctl1.UpdatedAt,
 		Name:        ctl1.Name,
 		CloudName:   ctl1.CloudName,
 		CloudRegion: ctl1.CloudRegion,
@@ -370,13 +374,17 @@ func TestCloudRegionControllers(t *testing.T) {
 	c.Assert(err, qt.IsNil)
 	c.Check(crcps, qt.HasLen, 2)
 	c.Check(crcps[0].Controller, qt.DeepEquals, dbmodel.Controller{
-		Model:       ctl1.Model,
+		ID:          ctl1.ID,
+		CreatedAt:   ctl1.CreatedAt,
+		UpdatedAt:   ctl1.UpdatedAt,
 		Name:        ctl1.Name,
 		CloudName:   ctl1.CloudName,
 		CloudRegion: ctl1.CloudRegion,
 	})
 	c.Check(crcps[1].Controller, qt.DeepEquals, dbmodel.Controller{
-		Model:       ctl2.Model,
+		ID:          ctl2.ID,
+		CreatedAt:   ctl2.CreatedAt,
+		UpdatedAt:   ctl2.UpdatedAt,
 		Name:        ctl2.Name,
 		CloudName:   ctl2.CloudName,
 		CloudRegion: ctl2.CloudRegion,

internal/dbmodel/controller.go

@@ -6,6 +6,7 @@ import (
 	"database/sql"
 	"net"
 	"strconv"
+	"time"
 
 	jujuparams "github.com/juju/juju/rpc/params"
 	"github.com/juju/names/v5"
@@ -17,7 +18,10 @@ import (
 // A controller represents a juju controller which is hosting models
 // within the JAAS system.
 type Controller struct {
-	gorm.Model
+	// Note that we do not use gorm.Model to avoid the use of soft-deletes.
+	ID        uint `gorm:"primarykey"`
+	CreatedAt time.Time
+	UpdatedAt time.Time
 
 	// Name is the name given to this controller.
 	Name string `gorm:"not null;uniqueIndex"`

internal/dbmodel/sql/postgres/1_9.sql

@@ -0,0 +1,6 @@
+-- 1_9.sql is a migration that deletes soft-deleted controllers and
+-- drops the deleted_at column from the controllers table.
+DELETE FROM controllers WHERE deleted_at IS NOT null;
+ALTER TABLE controllers DROP COLUMN deleted_at;
+
+UPDATE versions SET major=1, minor=9 WHERE component='jimmdb';

internal/dbmodel/version.go

@@ -20,7 +20,7 @@ const (
 	// Minor is the minor version of the model described in the dbmodel
 	// package. It should be incremented for any change made to the
 	// database model from database model in a released JIMM.
-	Minor = 8
+	Minor = 9
 )
 
 type Version struct {