fix: move experiment SQL error (#9042)

* fix: move experiement SQL error * test: add intg test cases * fix: typo
determined-ai · Mar 25, 2024 · 6c88e8d · 6c88e8d
1 parent 3fa0df1
commit 6c88e8d
Show file tree

Hide file tree

Showing 2 changed files with 139 additions and 6 deletions.
diff --git a/master/internal/api_experiment_intg_test.go b/master/internal/api_experiment_intg_test.go
@@ -292,6 +292,139 @@ func TestMoveExperimentRunID(t *testing.T) {
 	})
 }
 
+func TestMoveExperiments(t *testing.T) {
+	api, curUser, ctx := setupAPITest(t, nil)
+	_, projectID := createProjectAndWorkspace(ctx, t, api)
+
+	t.Run("Move an experiement without filters", func(t *testing.T) {
+		exp := createTestExp(t, api, curUser)
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{int32(exp.ID)},
+			DestinationProjectId: int32(projectID),
+			Filters:              nil,
+		})
+		for _, v := range result.Results {
+			require.Equal(t, v.Error, "")
+		}
+		require.Equal(t, len(result.Results), 1)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move multiple experiments without filters", func(t *testing.T) {
+		exp1 := createTestExp(t, api, curUser)
+		exp2 := createTestExp(t, api, curUser)
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{int32(exp1.ID), int32(exp2.ID)},
+			DestinationProjectId: int32(projectID),
+			Filters:              nil,
+		})
+		for _, v := range result.Results {
+			require.Equal(t, v.Error, "")
+		}
+		require.Equal(t, len(result.Results), 2)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move experiments with filters", func(t *testing.T) {
+		const numNewExp = 13
+		label := uuid.New().String()
+		for i := 0; i < numNewExp; i++ {
+			createTestExp(t, api, curUser, label)
+		}
+
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{},
+			DestinationProjectId: int32(projectID),
+			Filters: &apiv1.BulkExperimentFilters{
+				Labels: []string{label},
+			},
+		})
+		for _, v := range result.Results {
+			require.Equal(t, v.Error, "")
+		}
+		require.Equal(t, len(result.Results), numNewExp)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move no experiments with filters (no filter match)", func(t *testing.T) {
+		notExsistentLable := uuid.New().String()
+		createTestExp(t, api, curUser)
+
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{},
+			DestinationProjectId: int32(projectID),
+			Filters: &apiv1.BulkExperimentFilters{
+				Labels:   []string{notExsistentLable},
+				Archived: &wrappers.BoolValue{Value: true},
+			},
+		})
+		require.Equal(t, len(result.Results), 0)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move multiple experiments to non-existent project", func(t *testing.T) {
+		exp1 := createTestExp(t, api, curUser)
+		exp2 := createTestExp(t, api, curUser)
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{int32(exp1.ID), int32(exp2.ID)},
+			DestinationProjectId: 0,
+			Filters:              nil,
+		})
+		require.Nil(t, result)
+		require.Error(t, err)
+	})
+
+	t.Run("Move 0 experiments", func(t *testing.T) {
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{},
+			DestinationProjectId: int32(projectID),
+			Filters:              nil,
+		})
+		require.Equal(t, len(result.Results), 0)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move a non-existent experiement", func(t *testing.T) {
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        []int32{-1, 0},
+			DestinationProjectId: int32(projectID),
+			Filters:              nil,
+		})
+		require.Equal(t, len(result.Results), 2)
+		require.NoError(t, err)
+	})
+
+	t.Run("Move mixed of existent and non-existent experiments", func(t *testing.T) {
+		exp := createTestExp(t, api, curUser)
+		expIds := []int32{-1, 0, int32(exp.ID)}
+		result, err := api.MoveExperiments(ctx, &apiv1.MoveExperimentsRequest{
+			ExperimentIds:        expIds,
+			DestinationProjectId: int32(projectID),
+			Filters:              nil,
+		})
+		successIDList := make([]int32, 0)
+		errorIDList := make([]int32, 0)
+		for _, v := range result.Results {
+			if v.Error == "" {
+				require.Equal(t, v.Error, "")
+				successIDList = append(successIDList, v.Id)
+			} else {
+				require.Equal(
+					t,
+					v.Error,
+					fmt.Sprintf("rpc error: code = NotFound desc = experiment '%d' not found", v.Id),
+				)
+				errorIDList = append(errorIDList, v.Id)
+			}
+		}
+		require.Equal(t, len(successIDList), 1)
+		require.Equal(t, len(errorIDList), 2)
+		require.Equal(t, successIDList[0], int32(exp.ID))
+		require.Equal(t, len(result.Results), len(expIds))
+		require.NoError(t, err)
+	})
+}
+
 func TestDeleteExperimentWithoutCheckpoints(t *testing.T) {
 	api, curUser, ctx := setupAPITest(t, nil)
 	exp := createTestExp(t, api, curUser)

diff --git a/master/internal/experiment/bulk_action.go b/master/internal/experiment/bulk_action.go
@@ -632,19 +632,19 @@ func MoveExperiments(ctx context.Context,
 
 	var expChecks []archiveExperimentOKResult
 	getQ := db.Bun().NewSelect().
-		ModelTableExpr("experiments AS exp").
+		ModelTableExpr("experiments AS e").
 		Model(&expChecks).
-		Column("exp.id").
-		ColumnExpr("(exp.archived OR p.archived OR w.archived) AS archived").
+		Column("e.id").
+		ColumnExpr("(e.archived OR p.archived OR w.archived) AS archived").
 		ColumnExpr("TRUE AS state").
-		Join("JOIN projects p ON exp.project_id = p.id").
+		Join("JOIN projects p ON e.project_id = p.id").
 		Join("JOIN workspaces w ON p.workspace_id = w.id")
 
 	if filters == nil {
-		getQ = getQ.Where("exp.id IN (?)", bun.In(experimentIds))
+		getQ = getQ.Where("e.id IN (?)", bun.In(experimentIds))
 	} else {
 		getQ = queryBulkExperiments(getQ, filters).
-			Where("NOT (exp.archived OR p.archived OR w.archived)")
+			Where("NOT (e.archived OR p.archived OR w.archived)")
 	}
 
 	if getQ, err = AuthZProvider.Get().FilterExperimentsQuery(ctx, *curUser, nil, getQ,