Repository: G-Research/fasttrackml
Pull request #778 (2023-12-28 16:37:18)

Prevent deletion of default namespace and default experiment
PR 1/2. What's inside the box:
- prevent deletion for `default` namespace.
- prevent deletion for `default` experiment.
- fix typos.
- updated `unit` and `integration` tests according to the new logic.


Patch:
diff --git a/pkg/api/admin/service/namespace/service.go b/pkg/api/admin/service/namespace/service.go
index b7962b1..ebb9960 100644
--- a/pkg/api/admin/service/namespace/service.go
+++ b/pkg/api/admin/service/namespace/service.go
@@ -99,6 +99,9 @@ func (s Service) DeleteNamespace(ctx context.Context, id uint) error {
 	if namespace == nil {
 		return eris.Errorf("namespace not found by id: %d", id)
 	}
+	if namespace.IsDefault() {
+		return eris.Errorf("unable to delete default namespace")
+	}
 	if err := s.namespaceRepository.Delete(ctx, namespace); err != nil {
 		return eris.Wrap(err, "error deleting namespace")
 	}
diff --git a/pkg/api/aim/experiments.go b/pkg/api/aim/experiments.go
index 7d0c615..0efdbf5 100644
--- a/pkg/api/aim/experiments.go
+++ b/pkg/api/aim/experiments.go
@@ -307,20 +307,25 @@ func DeleteExperiment(c *fiber.Ctx) error {
 		)
 	}
 
-	// validate that requested experiment exists.
+	// validate that requested experiment exists and is not a default experiment.
+	experiment := database.Experiment{}
 	if err := database.DB.Select(
-		"ID",
+		"ID", "Name",
+	).Where(
+		"experiments.experiment_id = ?", id,
 	).Where(
 		"experiments.namespace_id = ?", ns.ID,
-	).First(&database.Experiment{
-		ID: common.GetPointer(int32(id)),
-	}).Error; err != nil {
+	).First(&experiment).Error; err != nil {
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return fiber.ErrNotFound
 		}
 		return fmt.Errorf("unable to find experiment %q: %w", params.ID, err)
 	}
 
+	if experiment.IsDefault() {
+		return fiber.NewError(fiber.StatusBadRequest, "unable to delete default experiment")
+	}
+
 	// TODO this code should move to service with injected repository
 	experimentRepo := repositories.NewExperimentRepository(database.DB)
 	if err = experimentRepo.Delete(c.Context(), &models.Experiment{
diff --git a/pkg/api/mlflow/dao/models/experiment.go b/pkg/api/mlflow/dao/models/experiment.go
index 2d5830e..4b444e9 100644
--- a/pkg/api/mlflow/dao/models/experiment.go
+++ b/pkg/api/mlflow/dao/models/experiment.go
@@ -4,6 +4,12 @@ import (
 	"database/sql"
 )
 
+// Default Experiment properties.
+const (
+	DefaultExperimentID   = int32(0)
+	DefaultExperimentName = "Default"
+)
+
 // Experiment represents model to work with `experiments` table.
 type Experiment struct {
 	ID               *int32         `gorm:"column:experiment_id;not null;primaryKey"`
@@ -18,6 +24,11 @@ type Experiment struct {
 	Runs             []Run           `gorm:"constraint:OnDelete:CASCADE"`
 }
 
+// IsDefault makes check that Experiment is default.
+func (e Experiment) IsDefault() bool {
+	return e.ID != nil && *e.ID == DefaultExperimentID && e.Name == DefaultExperimentName
+}
+
 // ExperimentTag represents model to work with `experiment_tags` table.
 type ExperimentTag struct {
 	Key          string `gorm:"type:varchar(250);not null;primaryKey"`
diff --git a/pkg/api/mlflow/dao/models/namespace.go b/pkg/api/mlflow/dao/models/namespace.go
index 1642f2d..1755a23 100644
--- a/pkg/api/mlflow/dao/models/namespace.go
+++ b/pkg/api/mlflow/dao/models/namespace.go
@@ -7,6 +7,9 @@ import (
 	"gorm.io/gorm"
 )
 
+// DefaultNamespaceCode represents default Namespace code.
+const DefaultNamespaceCode = "default"
+
 // Namespace represents model to work with `namespaces` table.
 type Namespace struct {
 	ID                  uint           `gorm:"primaryKey;autoIncrement" json:"id"`
@@ -19,9 +22,15 @@ type Namespace struct {
 	Experiments         []Experiment   `gorm:"constraint:OnDelete:CASCADE" json:"experiments"`
 }
 
+// DisplayName returns Namespace display name.
 func (ns Namespace) DisplayName() string {
 	if ns.Description != "" {
 		return fmt.Sprintf("%s (%s)", ns.Code, ns.Description)
 	}
 	return ns.Code
 }
+
+// IsDefault makes check that Namespace is default.
+func (ns Namespace) IsDefault() bool {
+	return ns.Code == DefaultNamespaceCode
+}
diff --git a/pkg/api/mlflow/service/experiment/service.go b/pkg/api/mlflow/service/experiment/service.go
index 7e01155..dae252f 100644
--- a/pkg/api/mlflow/service/experiment/service.go
+++ b/pkg/api/mlflow/service/experiment/service.go
@@ -190,7 +190,11 @@ func (s Service) DeleteExperiment(
 
 	experiment, err := s.experimentRepository.GetByNamespaceIDAndExperimentID(ctx, ns.ID, int32(parsedID))
 	if err != nil {
-		return api.NewResourceDoesNotExistError(`unable to find experiment '%d': %s`, parsedID, err)
+		return api.NewResourceDoesNotExistError("unable to find experiment '%d': %s", parsedID, err)
+	}
+
+	if experiment.IsDefault() {
+		return api.NewBadRequestError("unable to delete default experiment")
 	}
 
 	experiment.LifecycleStage = models.LifecycleStageDeleted
diff --git a/pkg/common/middleware/namespace/namespace.go b/pkg/common/middleware/namespace/namespace.go
index af6a58b..0e68e35 100644
--- a/pkg/common/middleware/namespace/namespace.go
+++ b/pkg/common/middleware/namespace/namespace.go
@@ -17,8 +17,7 @@ import (
 )
 
 const (
-	namespaceContextKey  = "namespace"
-	defaultNamespaceCode = "default"
+	namespaceContextKey = "namespace"
 )
 
 var namespaceRegexp = regexp.MustCompile(`^/ns/([^/]+)/`)
@@ -28,7 +27,7 @@ func New(namespaceRepository repositories.NamespaceRepositoryProvider) fiber.Han
 	return func(c *fiber.Ctx) (err error) {
 		log.Debugf("checking namespace for path: %s", c.Path())
 		// if namespace exists in the request then try to process it, otherwise fallback to default namespace.
-		namespaceCode := defaultNamespaceCode
+		namespaceCode := models.DefaultNamespaceCode
 		if matches := namespaceRegexp.FindStringSubmatch(c.Path()); matches != nil {
 			namespaceCode = strings.Clone(matches[1])
 			c.Path(strings.TrimPrefix(c.Path(), fmt.Sprintf("/ns/%s", namespaceCode)))
diff --git a/pkg/database/model.go b/pkg/database/model.go
index 7d3b2d1..a7faca1 100644
--- a/pkg/database/model.go
+++ b/pkg/database/model.go
@@ -31,6 +31,12 @@ const (
 	LifecycleStageDeleted LifecycleStage = "deleted"
 )
 
+// Default Experiment properties.
+const (
+	DefaultExperimentID   = int32(0)
+	DefaultExperimentName = "Default"
+)
+
 type Namespace struct {
 	ID                  uint           `gorm:"primaryKey;autoIncrement" json:"id"`
 	Apps                []App          `gorm:"constraint:OnDelete:CASCADE" json:"apps"`
@@ -56,6 +62,11 @@ type Experiment struct {
 	Runs             []Run           `gorm:"constraint:OnDelete:CASCADE"`
 }
 
+// IsDefault makes check that Experiment is default.
+func (e Experiment) IsDefault() bool {
+	return e.ID != nil && *e.ID == DefaultExperimentID && e.Name == DefaultExperimentName
+}
+
 type ExperimentTag struct {
 	Key          string `gorm:"type:varchar(250);not null;primaryKey"`
 	Value        string `gorm:"type:varchar(5000)"`
diff --git a/pkg/ui/common/helpers.go b/pkg/ui/common/helpers.go
index e44cbc9..755e4aa 100644
--- a/pkg/ui/common/helpers.go
+++ b/pkg/ui/common/helpers.go
@@ -56,6 +56,6 @@ func ErrorMessageForUI(field, errMsg string) string {
 	case validationError.Match(msg):
 		return fmt.Sprintf("The %s is invalid.", field)
 	default:
-		return fmt.Sprintf("An unexepected error was encountered: %s", msg)
+		return fmt.Sprintf("An unexpected error was encountered: %s", msg)
 	}
 }


Test patch:
diff --git a/pkg/api/admin/service/namespace/service_test.go b/pkg/api/admin/service/namespace/service_test.go
index 955d668..da3f646 100644
--- a/pkg/api/admin/service/namespace/service_test.go
+++ b/pkg/api/admin/service/namespace/service_test.go
@@ -192,6 +192,27 @@ func TestService_DeleteNamespace_Error(t *testing.T) {
 	assert.Equal(t, "namespace not found by id: 0", err.Error())
 }
 
+func TestService_DeleteDefaultNamespace_Error(t *testing.T) {
+	// init repository mocks.
+	namespaceRepository := repositories.MockNamespaceRepositoryProvider{}
+	namespaceRepository.On(
+		"GetByID", context.TODO(), uint(0),
+	).Return(&models.Namespace{
+		ID:   1,
+		Code: models.DefaultNamespaceCode,
+	}, nil)
+
+	// call service under testing.
+	service := NewService(
+		&namespaceRepository,
+	)
+	err := service.DeleteNamespace(context.TODO(), uint(0))
+
+	// compare results.
+	assert.NotNil(t, err)
+	assert.Equal(t, "unable to delete default namespace", err.Error())
+}
+
 func TestService_UpdateNamespace_Ok(t *testing.T) {
 	// init repository mocks.
 	namespaceRepository := repositories.MockNamespaceRepositoryProvider{}
diff --git a/pkg/api/mlflow/service/experiment/service_test.go b/pkg/api/mlflow/service/experiment/service_test.go
index fe66a82..29deb0b 100644
--- a/pkg/api/mlflow/service/experiment/service_test.go
+++ b/pkg/api/mlflow/service/experiment/service_test.go
@@ -307,6 +307,27 @@ func TestService_DeleteExperiment_Error(t *testing.T) {
 				)
 			},
 		},
+		{
+			name:  "DefaultExperiment",
+			error: api.NewBadRequestError("unable to delete default experiment"),
+			request: &request.DeleteExperimentRequest{
+				ID: "0",
+			},
+			service: func() *Service {
+				experimentRepository := repositories.MockExperimentRepositoryProvider{}
+				experimentRepository.On(
+					"GetByNamespaceIDAndExperimentID", context.TODO(), ns.ID, int32(0),
+				).Return(&models.Experiment{
+					ID:   common.GetPointer(models.DefaultExperimentID),
+					Name: models.DefaultExperimentName,
+				}, nil)
+				return NewService(
+					&config.ServiceConfig{},
+					&repositories.MockTagRepositoryProvider{},
+					&experimentRepository,
+				)
+			},
+		},
 		{
 			name:  "UpdateExperimentDatabaseError",
 			error: api.NewInternalError(`unable to delete experiment '1': database error`),
diff --git a/pkg/ui/common/helpers_test.go b/pkg/ui/common/helpers_test.go
index f12fca7..94d4f93 100644
--- a/pkg/ui/common/helpers_test.go
+++ b/pkg/ui/common/helpers_test.go
@@ -29,7 +29,7 @@ func TestErrorMessageForUI(t *testing.T) {
 			name:     "UnknownError",
 			field:    "username",
 			errMsg:   "An unknown error occurred",
-			expected: "An unexepected error was encountered: An unknown error occurred",
+			expected: "An unexpected error was encountered: An unknown error occurred",
 		},
 	}
 
diff --git a/tests/integration/golang/admin/namespace/delete_test.go b/tests/integration/golang/admin/namespace/delete_test.go
index 3cda2d3..9ce8b7f 100644
--- a/tests/integration/golang/admin/namespace/delete_test.go
+++ b/tests/integration/golang/admin/namespace/delete_test.go
@@ -81,7 +81,16 @@ func (s *DeleteNamespaceTestSuite) Test_Error() {
 			ID:                      "10",
 			expectedNamespacesCount: 2,
 			response: map[string]any{
-				"message": "An unexepected error was encountered: namespace not found by id: 10",
+				"message": "An unexpected error was encountered: namespace not found by id: 10",
+				"status":  "error",
+			},
+		},
+		{
+			name:                    "DeleteDefaultNamespace",
+			ID:                      "1",
+			expectedNamespacesCount: 2,
+			response: map[string]any{
+				"message": "An unexpected error was encountered: unable to delete default namespace",
 				"status":  "error",
 			},
 		},
diff --git a/tests/integration/golang/admin/namespace/update_test.go b/tests/integration/golang/admin/namespace/update_test.go
index 8652d68..943f6c9 100644
--- a/tests/integration/golang/admin/namespace/update_test.go
+++ b/tests/integration/golang/admin/namespace/update_test.go
@@ -74,7 +74,7 @@ func (s *UpdateNamespaceTestSuite) Test_Error() {
 				Description: "test namespace updated",
 			},
 			response: map[string]any{
-				"message": "An unexepected error was encountered: namespace not found by id: 10",
+				"message": "An unexpected error was encountered: namespace not found by id: 10",
 				"status":  "error",
 			},
 		},
diff --git a/tests/integration/golang/aim/experiment/delete_test.go b/tests/integration/golang/aim/experiment/delete_test.go
index 220c48a..2984774 100644
--- a/tests/integration/golang/aim/experiment/delete_test.go
+++ b/tests/integration/golang/aim/experiment/delete_test.go
@@ -18,11 +18,7 @@ type DeleteExperimentTestSuite struct {
 }
 
 func TestDeleteExperimentTestSuite(t *testing.T) {
-	suite.Run(t, &DeleteExperimentTestSuite{
-		helpers.BaseTestSuite{
-			SkipCreateDefaultExperiment: true,
-		},
-	})
+	suite.Run(t, &DeleteExperimentTestSuite{})
 }
 
 func (s *DeleteExperimentTestSuite) Test_Ok() {
@@ -65,10 +61,15 @@ func (s *DeleteExperimentTestSuite) Test_Error() {
 			error: "Not Found",
 		},
 		{
+			ID:   "incorrect_experiment_id",
 			name: "DeleteIncorrectExperimentID",
 			error: `: unable to parse experiment id "incorrect_experiment_id": strconv.ParseInt:` +
 				` parsing "incorrect_experiment_id": invalid syntax`,
-			ID: "incorrect_experiment_id",
+		},
+		{
+			ID:    "0",
+			name:  "DeleteDefaultExperiment",
+			error: `unable to delete default experiment`,
 		},
 	}
 	for _, tt := range tests {
diff --git a/tests/integration/golang/mlflow/experiment/delete_test.go b/tests/integration/golang/mlflow/experiment/delete_test.go
index 13d0533..374d71f 100644
--- a/tests/integration/golang/mlflow/experiment/delete_test.go
+++ b/tests/integration/golang/mlflow/experiment/delete_test.go
@@ -21,11 +21,7 @@ type DeleteExperimentTestSuite struct {
 }
 
 func TestDeleteExperimentTestSuite(t *testing.T) {
-	suite.Run(t, &DeleteExperimentTestSuite{
-		helpers.BaseTestSuite{
-			SkipCreateDefaultExperiment: true,
-		},
-	})
+	suite.Run(t, &DeleteExperimentTestSuite{})
 }
 
 func (s *DeleteExperimentTestSuite) Test_Ok() {
@@ -96,6 +92,13 @@ func (s *DeleteExperimentTestSuite) Test_Error() {
 				ID: "123",
 			},
 		},
+		{
+			name:  "DeleteDefaultExperiment",
+			error: api.NewBadRequestError("unable to delete default experiment"),
+			request: &request.DeleteExperimentRequest{
+				ID: "0",
+			},
+		},
 	}
 
 	for _, tt := range testData {