diff --git a/api/http/handler/handler.go b/api/http/handler/handler.go index 33d341773..8c5a8667c 100644 --- a/api/http/handler/handler.go +++ b/api/http/handler/handler.go @@ -79,7 +79,7 @@ type Handler struct { } // @title PortainerCE API -// @version 2.43.0 +// @version 2.44.0 // @description.markdown // @x-tagGroups [{"name":"Access Control","tags":["auth","roles","team_memberships","teams","users"]},{"name":"Administration","tags":["backup","ldap","motd","settings","status","system","ssl","upload"]},{"name":"Docker","tags":["templates","custom_templates","docker","registries","resource_controls","stacks","webhooks","websocket"]},{"name":"Edge Compute","tags":["edge_agent","edge_groups","edge_jobs","edge","edge_stacks"]},{"name":"Environment Management","tags":["endpoint_groups","endpoints","tags"]},{"name":"GitOps","tags":["gitops"]},{"name":"Kubernetes","tags":["helm","kubernetes"]}] // @termsOfService diff --git a/api/http/handler/stacks/stack_delete.go b/api/http/handler/stacks/stack_delete.go index b9e14fbc5..d639f8378 100644 --- a/api/http/handler/stacks/stack_delete.go +++ b/api/http/handler/stacks/stack_delete.go @@ -139,13 +139,25 @@ func (handler *Handler) stackDelete(w http.ResponseWriter, r *http.Request) *htt } } - if err := handler.FileService.RemoveDirectory(stack.ProjectPath); err != nil { + if err := handler.removeStackProjectDir(stack); err != nil { log.Warn().Err(err).Msg("Unable to remove stack files from disk") } return response.Empty(w) } +// removeStackProjectDir deletes a stack's files from disk. For file-based (non-git) stacks the +// whole stack root (compose/{id}) is removed so every versioned subfolder (v1..vN) is cleaned up, +// because stack.ProjectPath only points at the current version directory (compose/{id}/v{N}). +// Git stacks keep a ProjectPath of compose/{id}, so removing it directly preserves their behavior. +func (handler *Handler) removeStackProjectDir(stack *portainer.Stack) error { + if stack.WorkflowID == 0 { + return handler.FileService.RemoveDirectory(handler.FileService.GetStackProjectPath(strconv.Itoa(int(stack.ID)))) + } + + return handler.FileService.RemoveDirectory(stack.ProjectPath) +} + func (handler *Handler) deleteExternalStack(r *http.Request, w http.ResponseWriter, stackName string, securityContext *security.RestrictedRequestContext) *httperror.HandlerError { endpointID, err := request.RetrieveNumericQueryParameter(r, "endpointId", false) if err != nil { @@ -355,7 +367,7 @@ func (handler *Handler) stackDeleteKubernetesByName(w http.ResponseWriter, r *ht continue } - if err := handler.FileService.RemoveDirectory(stack.ProjectPath); err != nil { + if err := handler.removeStackProjectDir(&stack); err != nil { errs = errors.Join(errs, err) log.Warn().Err(err).Msg("Unable to remove stack files from disk") } diff --git a/api/http/handler/stacks/stack_file.go b/api/http/handler/stacks/stack_file.go index 8302713d3..2cc5bb903 100644 --- a/api/http/handler/stacks/stack_file.go +++ b/api/http/handler/stacks/stack_file.go @@ -30,6 +30,7 @@ type stackFileResponse struct { // @security jwt // @produce json // @param id path int true "Stack identifier" +// @param version query int false "return this file version (file-based stacks)" // @success 200 {object} stackFileResponse "Success" // @failure 400 "Invalid request" // @failure 403 "Permission denied" diff --git a/api/http/handler/stacks/stack_file_test.go b/api/http/handler/stacks/stack_file_test.go index fe1afed45..a2489dcd2 100644 --- a/api/http/handler/stacks/stack_file_test.go +++ b/api/http/handler/stacks/stack_file_test.go @@ -76,6 +76,130 @@ func TestStackFile_GitPendingRedeploy_Returns409(t *testing.T) { require.Equal(t, http.StatusConflict, rr.Code) } +// setupFileVersionStackTest wires a handler over a real datastore + filesystem and creates a +// file-based (non-git) stack together with the given on-disk file versions. It returns the handler +// and the created stack so version-selection cases can be exercised through the HTTP handler. +func setupFileVersionStackTest(t *testing.T, stack *portainer.Stack, versionContent map[int]string) *Handler { + t.Helper() + + _, store := datastore.MustNewTestStore(t, false, true) + + _, err := mockCreateUser(store) + require.NoError(t, err) + + endpoint, err := mockCreateEndpoint(store) + require.NoError(t, err) + + fileService, err := filesystem.NewService(t.TempDir(), "") + require.NoError(t, err) + + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.FileService = fileService + handler.DataStore = store + + stackFolder := strconv.Itoa(int(stack.ID)) + for v, content := range versionContent { + _, err := fileService.StoreStackFileFromBytesByVersion(stackFolder, stack.EntryPoint, v, []byte(content)) + require.NoError(t, err) + } + + stack.EndpointID = endpoint.ID + require.NoError(t, store.Stack().Create(stack)) + + return handler +} + +// requestStackFile performs a GET /stacks/{id}/file request (optionally with a raw query string). +func requestStackFile(t *testing.T, handler *Handler, stackID portainer.StackID, rawQuery string) *httptest.ResponseRecorder { + t.Helper() + + target := "/stacks/" + strconv.Itoa(int(stackID)) + "/file" + if rawQuery != "" { + target += "?" + rawQuery + } + + req := mockCreateStackRequestWithSecurityContext(http.MethodGet, target, nil) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + return rr +} + +// TestStackFile_VersionParam exercises the ?version= selector on a file-based stack: rejects a +// negative version, rejects an out-of-range version, and returns the selected version's content. +func TestStackFile_VersionParam(t *testing.T) { + t.Parallel() + + newHandlerAndStack := func(t *testing.T) (*Handler, portainer.StackID) { + stack := &portainer.Stack{ + ID: 10, + Type: portainer.DockerComposeStack, + EntryPoint: "docker-compose.yml", + StackFileVersion: 3, + Versions: []portainer.StackFileVersionInfo{ + {Version: 1}, {Version: 2}, {Version: 3}, + }, + } + handler := setupFileVersionStackTest(t, stack, map[int]string{ + 1: "V1-CONTENT", 2: "V2-CONTENT", 3: "V3-CONTENT", + }) + // Point ProjectPath at the current version directory (as the versioning code does). + stack.ProjectPath = handler.FileService.GetStackProjectPathByVersion("10", 3, "") + require.NoError(t, handler.DataStore.Stack().Update(stack.ID, stack)) + + return handler, stack.ID + } + + t.Run("negative version returns 400", func(t *testing.T) { + handler, id := newHandlerAndStack(t) + rr := requestStackFile(t, handler, id, "version=-1") + require.Equal(t, http.StatusBadRequest, rr.Code) + }) + + t.Run("out-of-range version returns 400", func(t *testing.T) { + handler, id := newHandlerAndStack(t) + rr := requestStackFile(t, handler, id, "version=99") + require.Equal(t, http.StatusBadRequest, rr.Code) + }) + + t.Run("valid version returns that version content", func(t *testing.T) { + handler, id := newHandlerAndStack(t) + rr := requestStackFile(t, handler, id, "version=2") + require.Equal(t, http.StatusOK, rr.Code) + + var resp stackFileResponse + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &resp)) + require.Equal(t, "V2-CONTENT", resp.StackFileContent) + }) +} + +// TestStackFile_VersionParam_LegacyFallback covers the fallback branch for stacks predating the +// version history seed: when Versions[] is empty, an in-range version (1..StackFileVersion) is +// accepted and served from its v{N} directory. +func TestStackFile_VersionParam_LegacyFallback(t *testing.T) { + t.Parallel() + + stack := &portainer.Stack{ + ID: 11, + Type: portainer.DockerComposeStack, + EntryPoint: "docker-compose.yml", + StackFileVersion: 2, + // Versions intentionally empty: legacy stack without a recorded history. + } + handler := setupFileVersionStackTest(t, stack, map[int]string{ + 1: "LEGACY-V1", 2: "LEGACY-V2", + }) + stack.ProjectPath = handler.FileService.GetStackProjectPathByVersion("11", 2, "") + require.NoError(t, handler.DataStore.Stack().Update(stack.ID, stack)) + + rr := requestStackFile(t, handler, stack.ID, "version=1") + require.Equal(t, http.StatusOK, rr.Code) + + var resp stackFileResponse + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &resp)) + require.Equal(t, "LEGACY-V1", resp.StackFileContent) +} + func TestStackFile_MatchingGitSettings_ReturnsFileContent(t *testing.T) { t.Parallel() _, store := datastore.MustNewTestStore(t, false, true) diff --git a/api/http/handler/stacks/stack_versioning_test.go b/api/http/handler/stacks/stack_versioning_test.go index d87f62bc0..948ebe8aa 100644 --- a/api/http/handler/stacks/stack_versioning_test.go +++ b/api/http/handler/stacks/stack_versioning_test.go @@ -1,11 +1,16 @@ package stacks import ( + "errors" "strconv" "testing" portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/dataservices" + "github.com/portainer/portainer/api/datastore" "github.com/portainer/portainer/api/filesystem" + "github.com/portainer/portainer/api/internal/testhelpers" + httperror "github.com/portainer/portainer/pkg/libhttp/error" "github.com/stretchr/testify/require" ) @@ -92,6 +97,250 @@ func versionNumbers(versions []portainer.StackFileVersionInfo) []int { return out } +// TestValidateRollbackTarget checks the rollback-version guard: it rejects non-positive versions, +// versions above the current file version, and versions that are not present in the append-only +// history (a "hole"), while accepting an in-range version that exists in Versions[]. +func TestValidateRollbackTarget(t *testing.T) { + t.Parallel() + + // StackFileVersion is 5, but v4 was never recorded (a hole in the history). + stack := &portainer.Stack{ + StackFileVersion: 5, + Versions: []portainer.StackFileVersionInfo{ + {Version: 1}, {Version: 2}, {Version: 3}, {Version: 5}, + }, + } + + require.Error(t, validateRollbackTarget(stack, 0), "zero is not a valid version") + require.Error(t, validateRollbackTarget(stack, -1), "negative is not a valid version") + require.Error(t, validateRollbackTarget(stack, 6), "version above StackFileVersion is out of range") + require.Error(t, validateRollbackTarget(stack, 4), "a version missing from history (hole) is rejected") + + require.NoError(t, validateRollbackTarget(stack, 3), "in-range version present in history is accepted") + require.NoError(t, validateRollbackTarget(stack, 5), "current version present in history is accepted") +} + +// newVersioningTestHandler builds a Handler wired with a real filesystem service and datastore, +// plus a test user, for exercising the version snapshot/prune helpers. +func newVersioningTestHandler(t *testing.T) (*Handler, *datastore.Store, portainer.FileService, *portainer.User) { + t.Helper() + + _, store := datastore.MustNewTestStore(t, false, true) + + fs, err := filesystem.NewService(t.TempDir(), "") + require.NoError(t, err) + + user, err := mockCreateUser(store) + require.NoError(t, err) + + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.DataStore = store + handler.FileService = fs + + return handler, store, fs, user +} + +// TestSnapshotFileBasedStackVersion_Rollback verifies the rollback branch: it reads the TARGET +// version's content from disk (ignoring the client-supplied content), writes a NEW monotonic +// version whose note is "rollback from v{N}", and repoints ProjectPath to the new version dir. +func TestSnapshotFileBasedStackVersion_Rollback(t *testing.T) { + t.Parallel() + + handler, store, fs, user := newVersioningTestHandler(t) + + stackFolder := "1" + entryPoint := "docker-compose.yml" + + // Seed three on-disk versions with distinct content. + for v, content := range map[int]string{1: "V1-CONTENT", 2: "V2-CONTENT", 3: "V3-CONTENT"} { + _, err := fs.StoreStackFileFromBytesByVersion(stackFolder, entryPoint, v, []byte(content)) + require.NoError(t, err) + } + + stack := &portainer.Stack{ + ID: 1, + EntryPoint: entryPoint, + StackFileVersion: 3, + ProjectPath: fs.GetStackProjectPathByVersion(stackFolder, 3, ""), + Versions: []portainer.StackFileVersionInfo{ + {Version: 1}, {Version: 2}, {Version: 3}, + }, + } + + target := 1 + var ( + pruned []int + httpErr *httperror.HandlerError + ) + err := store.UpdateTx(func(tx dataservices.DataStoreTx) error { + // Client content is deliberately non-empty to prove it is IGNORED for a rollback. + pruned, httpErr = handler.snapshotFileBasedStackVersion(tx, stack, []byte("CLIENT-CONTENT-IGNORED"), &target, user.ID) + return nil + }) + require.NoError(t, err) + require.Nil(t, httpErr) + require.Empty(t, pruned, "no retention pruning expected below the cap") + + // A new monotonic version (v4) was created. + require.Equal(t, 4, stack.StackFileVersion) + require.Equal(t, fs.GetStackProjectPathByVersion(stackFolder, 4, ""), stack.ProjectPath) + + last := stack.Versions[len(stack.Versions)-1] + require.Equal(t, 4, last.Version) + require.Equal(t, "rollback from v1", last.Note) + + // The new version's content is the TARGET (v1) content read from disk, not the client payload. + got, err := fs.GetFileContent(stack.ProjectPath, entryPoint) + require.NoError(t, err) + require.Equal(t, "V1-CONTENT", string(got)) +} + +// TestSnapshotFileBasedStackVersion_MonotonicVersion guards against a len-based next-version bug: +// the new version must be StackFileVersion+1, strictly greater than any previously trimmed version, +// even when the history slice is shorter than StackFileVersion (older entries already pruned). +func TestSnapshotFileBasedStackVersion_MonotonicVersion(t *testing.T) { + t.Parallel() + + handler, store, fs, user := newVersioningTestHandler(t) + + stackFolder := "1" + entryPoint := "docker-compose.yml" + + // StackFileVersion is 24 but only two history entries remain (older ones were pruned): + // a len-based scheme would wrongly compute the next version as len+1 = 3. + stack := &portainer.Stack{ + ID: 1, + EntryPoint: entryPoint, + StackFileVersion: 24, + ProjectPath: fs.GetStackProjectPathByVersion(stackFolder, 24, ""), + Versions: []portainer.StackFileVersionInfo{ + {Version: 23}, {Version: 24}, + }, + } + + var httpErr *httperror.HandlerError + err := store.UpdateTx(func(tx dataservices.DataStoreTx) error { + _, httpErr = handler.snapshotFileBasedStackVersion(tx, stack, []byte("NEW-CONTENT"), nil, user.ID) + return nil + }) + require.NoError(t, err) + require.Nil(t, httpErr) + + require.Equal(t, 25, stack.StackFileVersion, "next version must be StackFileVersion+1, not len-based") + last := stack.Versions[len(stack.Versions)-1] + require.Equal(t, 25, last.Version) + require.Greater(t, last.Version, 24, "new version must be strictly greater than any prior version") + + got, err := fs.GetFileContent(stack.ProjectPath, entryPoint) + require.NoError(t, err) + require.Equal(t, "NEW-CONTENT", string(got)) +} + +// countingFileService wraps a FileService to count and optionally fail RemoveDirectory calls, +// so tests can assert prune ordering and error-swallowing without touching real disk. +type countingFileService struct { + portainer.FileService + removeDirErr error + removeDirCalls int + removedDirs []string +} + +func (s *countingFileService) GetStackProjectPathByVersion(stackID string, version int, commitHash string) string { + return "compose/" + stackID + "/v" + strconv.Itoa(version) +} + +func (s *countingFileService) RemoveDirectory(dir string) error { + s.removeDirCalls++ + s.removedDirs = append(s.removedDirs, dir) + return s.removeDirErr +} + +// TestPruneStackFileVersionDirs_RemovesGivenDirs verifies the prune helper physically deletes +// exactly the requested version directories and leaves the others untouched. +func TestPruneStackFileVersionDirs_RemovesGivenDirs(t *testing.T) { + t.Parallel() + + fs, err := filesystem.NewService(t.TempDir(), "") + require.NoError(t, err) + + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.FileService = fs + + const stackID = portainer.StackID(9) + stackFolder := strconv.Itoa(int(stackID)) + + for v := 1; v <= 3; v++ { + _, err := fs.StoreStackFileFromBytesByVersion(stackFolder, "docker-compose.yml", v, []byte("v"+strconv.Itoa(v))) + require.NoError(t, err) + } + + handler.pruneStackFileVersionDirs(stackID, []int{1, 3}) + + for v, wantExists := range map[int]bool{1: false, 2: true, 3: false} { + exists, err := fs.FileExists(fs.GetStackProjectPathByVersion(stackFolder, v, "")) + require.NoError(t, err) + require.Equal(t, wantExists, exists, "version %d directory existence mismatch", v) + } +} + +// TestPruneStackFileVersionDirs_SwallowsRemoveError verifies a RemoveDirectory failure is +// best-effort: it is logged and swallowed (no panic), and every requested version is attempted. +func TestPruneStackFileVersionDirs_SwallowsRemoveError(t *testing.T) { + t.Parallel() + + fs := &countingFileService{removeDirErr: errors.New("disk error")} + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.FileService = fs + + require.NotPanics(t, func() { + handler.pruneStackFileVersionDirs(7, []int{1, 2}) + }) + require.Equal(t, 2, fs.removeDirCalls, "every requested version directory must be attempted") +} + +// TestPruneGateContract_Illustrative documents the post-commit gate shape used by stackUpdate: +// the pruned version directories are physically removed only when the transaction committed +// (err == nil); on a failed transaction the trimmed Versions[] was never persisted, so the +// directories must be kept on disk to stay consistent with the database. +// +// NOTE: this reproduces the gate condition inline — it illustrates the intended contract rather +// than exercising the real handler wiring (forcing a mid-commit UpdateTx failure while +// pruneVersions is already populated is not injectable in a unit test). The actual gate lives in +// stackUpdate (`if err == nil && len(pruneVersions) > 0`); pruneStackFileVersionDirs's real +// behaviour (deletes the given dirs, swallows RemoveDirectory errors) is covered non-vacuously by +// TestPruneStackFileVersionDirs_RemovesGivenDirs and _SwallowsRemoveError. +func TestPruneGateContract_Illustrative(t *testing.T) { + t.Parallel() + + pruneVersions := []int{1, 2} + + t.Run("transaction failed - prune skipped", func(t *testing.T) { + fs := &countingFileService{} + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.FileService = fs + + var err error = errors.New("commit failed") + if err == nil && len(pruneVersions) > 0 { + handler.pruneStackFileVersionDirs(7, pruneVersions) + } + + require.Zero(t, fs.removeDirCalls, "no directory may be deleted when the transaction failed") + }) + + t.Run("transaction committed - prune runs", func(t *testing.T) { + fs := &countingFileService{} + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.FileService = fs + + var err error + if err == nil && len(pruneVersions) > 0 { + handler.pruneStackFileVersionDirs(7, pruneVersions) + } + + require.Equal(t, len(pruneVersions), fs.removeDirCalls, "all pruned directories deleted after commit") + }) +} + // TestSnapshotStackFilesToVersion verifies a multi-file snapshot writes every file into the // v{N} folder and returns the version project path (not the base path). func TestSnapshotStackFilesToVersion(t *testing.T) { diff --git a/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.test.tsx b/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.test.tsx index 09871a0b7..b33d95628 100644 --- a/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.test.tsx +++ b/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, waitFor } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { Formik } from 'formik'; import { vi } from 'vitest'; @@ -17,7 +17,10 @@ import { server } from '@/setup-tests/server'; import { usePreventExit } from '@@/WebEditorForm'; -import { StackEditorTabInner } from './StackEditorTabInner'; +import { + StackEditorTabInner, + resolveRollbackTarget, +} from './StackEditorTabInner'; import { StackEditorFormValues } from './StackEditorTab.types'; import { useVersionedStackFile } from './useVersionedStackFile'; @@ -383,6 +386,178 @@ describe('version rollback', () => { }); }); +// F6: the version selector must not silently discard manual edits. The backend +// ignores the client buffer whenever rollbackTo is set, so rollbackTo must only +// be set for a genuinely older version and must be cleared once the user either +// re-selects the current version or edits the buffer by hand. +describe('resolveRollbackTarget (F6 decision)', () => { + it('returns undefined when the current/top version is picked', () => { + // versions[0] is the latest -> picking it is not a rollback + expect(resolveRollbackTarget(3, [3, 2, 1])).toBeUndefined(); + }); + + it('returns the version when a genuinely older version is picked', () => { + expect(resolveRollbackTarget(2, [3, 2, 1])).toBe(2); + expect(resolveRollbackTarget(1, [3, 2, 1])).toBe(1); + }); + + it('returns undefined when only a single version exists', () => { + expect(resolveRollbackTarget(1, [1])).toBeUndefined(); + }); + + it('returns undefined when versions are unavailable', () => { + expect(resolveRollbackTarget(1, undefined)).toBeUndefined(); + expect(resolveRollbackTarget(1, [])).toBeUndefined(); + }); +}); + +describe('version selector edit trap (F6)', () => { + // The `version` passed to useVersionedStackFile mirrors values.rollbackTo, so + // we assert on the latest call to observe how rollbackTo evolves. + function lastRollbackTo() { + const { calls } = vi.mocked(useVersionedStackFile).mock; + return calls[calls.length - 1][0].version; + } + + beforeEach(() => { + vi.mocked(useVersionedStackFile).mockReturnValue({ + content: '', + isLoading: false, + }); + }); + + it('should set rollbackTo when an older version is selected', async () => { + renderComponent({ versions: [3, 2, 1] }); + const user = userEvent.setup(); + + const versionSelect = screen.getByRole('combobox', { name: /version/i }); + await user.selectOptions(versionSelect, '2'); + + await waitFor(() => { + expect(lastRollbackTo()).toBe(2); + }); + }); + + it('should clear rollbackTo when the current/top version is re-selected', async () => { + renderComponent({ versions: [3, 2, 1] }); + const user = userEvent.setup(); + + const versionSelect = screen.getByRole('combobox', { name: /version/i }); + + // First roll back to an older version... + await user.selectOptions(versionSelect, '2'); + await waitFor(() => { + expect(lastRollbackTo()).toBe(2); + }); + + // ...then return to the current version: this is not a rollback, so + // rollbackTo must be cleared to restore normal edit-and-deploy. + await user.selectOptions(versionSelect, '3'); + await waitFor(() => { + expect(lastRollbackTo()).toBeUndefined(); + }); + }); + + it('should clear rollbackTo when the user edits the buffer', async () => { + renderComponent( + { versions: [3, 2, 1] }, + { initialValues: { ...defaultInitialValues, rollbackTo: 2 } } + ); + const user = userEvent.setup(); + + // rollbackTo starts at 2 (an older version pre-selected) + expect(lastRollbackTo()).toBe(2); + + const editor = screen.getByTestId('stack-editor'); + await waitFor(() => { + expect(editor).not.toHaveAttribute('readonly'); + }); + + // A genuine user edit should reset rollbackTo so the edits are honored. + await user.type(editor, ' # manual edit'); + + await waitFor(() => { + expect(lastRollbackTo()).toBeUndefined(); + }); + }); + + it('should NOT clear rollbackTo when a version is loaded programmatically', async () => { + let capturedOnLoad: ((content: string) => void) | undefined; + vi.mocked(useVersionedStackFile).mockImplementation(({ onLoad }) => { + capturedOnLoad = onLoad; + return { content: '', isLoading: false }; + }); + + renderComponent( + { versions: [3, 2, 1] }, + { initialValues: { ...defaultInitialValues, rollbackTo: 2 } } + ); + + expect(capturedOnLoad).toBeDefined(); + + // The programmatic version-load goes through handleLoadFile (setFieldValue + // on stackFileContent), NOT the CodeEditor onChange, so rollbackTo must + // stay set — otherwise the rollback would immediately cancel itself. + act(() => { + capturedOnLoad?.('version: "2"\nservices:\n db:\n image: postgres'); + }); + + // Wait past the CodeEditor's 300ms onChange debounce: if the programmatic + // load ever leaked into handleContentChange it would clear rollbackTo only + // after the debounce fires, so asserting at t≈0 would pass vacuously. By + // waiting beyond the window we ensure the assertion fails if a load ever + // clears rollbackTo. + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + expect(lastRollbackTo()).toBe(2); + }); + + it('should restore the current content when returning to the current/top version', async () => { + let capturedOnLoad: ((content: string) => void) | undefined; + vi.mocked(useVersionedStackFile).mockImplementation(({ onLoad }) => { + capturedOnLoad = onLoad; + return { content: '', isLoading: false }; + }); + + renderComponent({ versions: [3, 2, 1] }); + const user = userEvent.setup(); + + const editor = screen.getByTestId('stack-editor'); + await waitFor(() => { + expect(editor).not.toHaveAttribute('readonly'); + }); + + const versionSelect = screen.getByRole('combobox', { name: /version/i }); + + // Roll back to an older version: rollbackTo is set and its content is loaded + // into the buffer (simulating useVersionedStackFile's fetch via onLoad). + const olderContent = 'version: "2"\nservices:\n db:\n image: postgres'; + await user.selectOptions(versionSelect, '2'); + await waitFor(() => { + expect(lastRollbackTo()).toBe(2); + }); + act(() => { + capturedOnLoad?.(olderContent); + }); + await waitFor(() => { + expect(editor).toHaveValue(olderContent); + }); + + // Return to the current/top version: rollbackTo must clear AND the editor + // must show the current content again, not the older version's leftover + // content (which would otherwise be deployed as a brand-new version). + await user.selectOptions(versionSelect, '3'); + await waitFor(() => { + expect(lastRollbackTo()).toBeUndefined(); + }); + expect(editor).toHaveValue(defaultInitialValues.stackFileContent); + }); +}); + describe('form submission', () => { it('should enable submit button when form is valid', async () => { renderComponent(); diff --git a/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.tsx b/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.tsx index d406b3a28..bd5a12846 100644 --- a/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.tsx +++ b/app/react/docker/stacks/ItemView/StackEditorTab/StackEditorTabInner.tsx @@ -22,6 +22,24 @@ import { WebhookFieldset } from '../../common/WebhookFieldset'; import { StackEditorFormValues } from './StackEditorTab.types'; import { useVersionedStackFile } from './useVersionedStackFile'; +/** + * Decide the `rollbackTo` value for a version chosen in the selector. + * + * `versions[0]` is the latest/current version (the list is sorted descending). + * Picking the current version is NOT a rollback, so this returns `undefined` + * to keep the normal edit-and-deploy flow; only a genuinely older version sets + * a rollback target (which the backend reads from disk, ignoring the buffer). + */ +export function resolveRollbackTarget( + newVersion: number, + versions?: Array +): number | undefined { + if (!versions || versions.length <= 1) { + return undefined; + } + return newVersion < versions[0] ? newVersion : undefined; +} + interface StackEditorTabInnerProps { stackType: StackType | undefined; composeSyntaxMaxVersion: number; @@ -69,6 +87,21 @@ export function StackEditorTabInner({ [setFieldValue] ); + const handleContentChange = useCallback( + (value: string) => { + setFieldValue('stackFileContent', value); + // A manual edit means the user wants a normal edit-and-deploy, not a + // rollback, so clear rollbackTo (otherwise the backend would ignore the + // edited buffer and redeploy the selected version from disk). This runs + // only on genuine user input: the programmatic version-load goes through + // handleLoadFile -> setFieldValue, which updates the editor's `value` + // prop and is skipped by CodeMirror's onChange (marked as an external + // change), so it does not clear rollbackTo. + setFieldValue('rollbackTo', undefined); + }, + [setFieldValue] + ); + useVersionedStackFile({ stackId, version: values.rollbackTo, @@ -119,7 +152,7 @@ export function StackEditorTabInner({ id="stack-editor" textTip="Define or paste the content of your docker compose file here" type="yaml" - onChange={(value) => setFieldValue('stackFileContent', value)} + onChange={handleContentChange} value={values.stackFileContent} readonly={isOrphaned || !isAuthorizedToUpdate} schema={schema} @@ -169,10 +202,21 @@ export function StackEditorTabInner({ async function handleVersionChange(newVersion: number) { if (versions && versions.length > 1) { - setFieldValue( - 'rollbackTo', - newVersion < versions[0] ? newVersion : versions[0] - ); + // Picking the current/top version clears rollbackTo (undefined); only a + // genuinely older version sets it. See resolveRollbackTarget. + const rollbackTarget = resolveRollbackTarget(newVersion, versions); + if (rollbackTarget === undefined) { + // Returning to the current version: restore the current file content so + // the editor matches the selector. Otherwise the previously-viewed older + // version's content would remain in the buffer (useVersionedStackFile + // does not fetch when rollbackTo is undefined) and get deployed as a new + // version. initialValues.stackFileContent is the current stack file + // loaded at page init. This flows through the CodeEditor `value` prop as + // an external change, so it does not re-trigger handleContentChange and + // rollbackTo stays undefined. + setFieldValue('stackFileContent', initialValues.stackFileContent); + } + setFieldValue('rollbackTo', rollbackTarget); } } } diff --git a/package.json b/package.json index 11ecea3f2..e1f33f49e 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "author": "Portainer.io", "name": "@portainer/ce", "homepage": "http://portainer.io", - "version": "2.43.0", + "version": "2.44.0", "repository": { "type": "git", "url": "git@github.com:portainer/portainer.git"