From 99b9e44e8cb6465aecdabd6ede6b9672538146a3 Mon Sep 17 00:00:00 2001 From: Anagh Hegde Date: Fri, 7 Jun 2024 13:02:20 +0530 Subject: [PATCH 01/12] chore: refactor analytics event for partial import (#34066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description The block drag and drop flow uses the partial import flow under the hood. This is causing wrong numbers being shown for the partial import flow usage. Hence refactoring the flow to handle the analytics events. Fixes #33868 ## Automation /ok-to-test tags="@tag.Templates" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 1cf3c837ba83aed905bad06ed5b97f3816de5e3a > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **New Features** - Enhanced import functionality to include user information and send analytics events after the import process. --- .../partial/PartialImportServiceCEImpl.java | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java index c3dc7551c5..db80d3ac15 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java @@ -94,13 +94,15 @@ public class PartialImportServiceCEImpl implements PartialImportServiceCE { @Override public Mono importResourceInPage( String workspaceId, String applicationId, String pageId, String branchName, Part file) { + Mono currUserMono = sessionUserService.getCurrentUser(); return importService .extractArtifactExchangeJson(file) .flatMap(artifactExchangeJson -> { if (artifactExchangeJson instanceof ApplicationJson && isImportableResource((ApplicationJson) artifactExchangeJson)) { - return importResourceInPage( - workspaceId, applicationId, pageId, branchName, (ApplicationJson) artifactExchangeJson); + return importResourceInPage(workspaceId, applicationId, pageId, branchName, (ApplicationJson) + artifactExchangeJson) + .zipWith(currUserMono); } else { return Mono.error( new AppsmithException( @@ -108,7 +110,21 @@ public class PartialImportServiceCEImpl implements PartialImportServiceCE { "The file is not compatible with the current partial import operation. Please check the file and try again.")); } }) - .map(BuildingBlockImportDTO::getApplication); + .flatMap(tuple -> { + final BuildingBlockImportDTO buildingBlockImportDTO = tuple.getT1(); + final User user = tuple.getT2(); + final Map eventData = + Map.of(FieldName.APPLICATION, buildingBlockImportDTO.getApplication()); + final Map data = Map.of( + FieldName.APPLICATION_ID, applicationId, + FieldName.WORKSPACE_ID, + buildingBlockImportDTO.getApplication().getWorkspaceId(), + FieldName.EVENT_DATA, eventData); + + return analyticsService + .sendEvent(AnalyticsEvents.PARTIAL_IMPORT.getEventName(), user.getUsername(), data) + .thenReturn(buildingBlockImportDTO.getApplication()); + }); } private boolean isImportableResource(ApplicationJson artifactExchangeJson) { @@ -128,8 +144,6 @@ public class PartialImportServiceCEImpl implements PartialImportServiceCE { Mono branchedPageIdMono = newPageService.findBranchedPageId(branchName, pageId, AclPermission.MANAGE_PAGES); - Mono currUserMono = sessionUserService.getCurrentUser(); - // Extract file and get App Json Mono partiallyImportedAppMono = getImportApplicationPermissions() .flatMap(permissionProvider -> { @@ -273,25 +287,13 @@ public class PartialImportServiceCEImpl implements PartialImportServiceCE { }) .as(transactionalOperator::transactional); - // Send Analytics event - return partiallyImportedAppMono.zipWith(currUserMono).flatMap(tuple -> { - Application application = tuple.getT1(); - User user = tuple.getT2(); - final Map eventData = Map.of(FieldName.APPLICATION, application); - - final Map data = Map.of( - FieldName.APPLICATION_ID, application.getId(), - FieldName.WORKSPACE_ID, application.getWorkspaceId(), - FieldName.EVENT_DATA, eventData); + return partiallyImportedAppMono.map(application -> { BuildingBlockImportDTO buildingBlockImportDTO = new BuildingBlockImportDTO(); buildingBlockImportDTO.setApplication(application); buildingBlockImportDTO.setWidgetDsl(applicationJson.getWidgets()); buildingBlockImportDTO.setRefactoredEntityNameMap( mappedImportableResourcesDTO.getRefactoringNameReference()); - - return analyticsService - .sendEvent(AnalyticsEvents.PARTIAL_IMPORT.getEventName(), user.getUsername(), data) - .thenReturn(buildingBlockImportDTO); + return buildingBlockImportDTO; }); } From 7b32f1aa501c0acba4abc367108e65816a1ed3dd Mon Sep 17 00:00:00 2001 From: Valera Melnikov Date: Fri, 7 Jun 2024 11:06:43 +0300 Subject: [PATCH 02/12] fix: improve link behaviour (#34043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Add hover and focus for WDS link component. https://github.com/appsmithorg/appsmith/assets/11555074/51e50841-b347-4585-aa5a-2fd54e13cf06 ## Automation /ok-to-test tags="@tag.Anvil" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 1a10e18ab16d30be828eeb931a2d5c22448fa096 > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Style** - Improved the appearance of links with new CSS properties, enhancing hover and focus states for better user experience. - **Refactor** - Reordered JSX elements within the Link component for cleaner and more maintainable code structure. --- .../widgets/src/components/Link/src/Link.tsx | 24 +++++++++---------- .../src/components/Link/src/styles.module.css | 16 +++++++++---- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/app/client/packages/design-system/widgets/src/components/Link/src/Link.tsx b/app/client/packages/design-system/widgets/src/components/Link/src/Link.tsx index a7f1aceedf..d11abd039d 100644 --- a/app/client/packages/design-system/widgets/src/components/Link/src/Link.tsx +++ b/app/client/packages/design-system/widgets/src/components/Link/src/Link.tsx @@ -18,18 +18,18 @@ export function Link(props: LinkProps) { } = props; return ( - - + + {children} - - + + ); } diff --git a/app/client/packages/design-system/widgets/src/components/Link/src/styles.module.css b/app/client/packages/design-system/widgets/src/components/Link/src/styles.module.css index f816fa47d0..802a5aeaed 100644 --- a/app/client/packages/design-system/widgets/src/components/Link/src/styles.module.css +++ b/app/client/packages/design-system/widgets/src/components/Link/src/styles.module.css @@ -1,11 +1,19 @@ .link { + position: relative; text-decoration: underline; - text-decoration-color: currentColor; text-underline-offset: 2px; color: currentColor; + text-decoration-color: var(--color-bd-accent); + text-decoration-thickness: 0.5px; - &:hover { - color: currentColor; - text-decoration-color: currentColor; + &[data-hovered] { + text-decoration: none; + } + + &[data-focus-visible] { + border-radius: var(--border-radius-elevation-3); + outline: 2px solid var(--color-bd-focus); + outline-offset: 4px; + text-decoration: none; } } From 5ef50350f31da6f9338172adcd1bd5e7e6c4196e Mon Sep 17 00:00:00 2001 From: Aman Agarwal Date: Fri, 7 Jun 2024 14:57:44 +0530 Subject: [PATCH 03/12] fix: hide skip button if the application object is not present in the onboarding screen (#34048) --- .../Applications/CreateNewAppsOption.test.tsx | 116 ++++++++++++++++++ .../Applications/CreateNewAppsOption.tsx | 56 ++++----- 2 files changed, 144 insertions(+), 28 deletions(-) create mode 100644 app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx diff --git a/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx b/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx new file mode 100644 index 0000000000..d299b7ef94 --- /dev/null +++ b/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx @@ -0,0 +1,116 @@ +import "@testing-library/jest-dom/extend-expect"; +import React from "react"; +import { render, screen } from "@testing-library/react"; +import { Provider } from "react-redux"; +import configureStore from "redux-mock-store"; +import { lightTheme } from "selectors/themeSelectors"; +import { ThemeProvider } from "styled-components"; +import CreateNewAppsOption from "./CreateNewAppsOption"; +import { BrowserRouter as Router } from "react-router-dom"; +import { unitTestBaseMockStore } from "layoutSystems/common/dropTarget/unitTestUtils"; + +const defaultStoreState = { + ...unitTestBaseMockStore, + entities: { + ...unitTestBaseMockStore.entities, + plugins: { + list: [], + }, + datasources: { + list: [], + mockDatasourceList: [], + }, + }, + ui: { + ...unitTestBaseMockStore.ui, + selectedWorkspace: { + ...unitTestBaseMockStore.ui.selectedWorkspace, + applications: [unitTestBaseMockStore.ui.applications.currentApplication], + }, + debugger: { + isOpen: false, + }, + editor: { + loadingStates: {}, + isProtectedMode: true, + zoomLevel: 1, + }, + gitSync: { + globalGitConfig: {}, + branches: [], + localGitConfig: {}, + disconnectingGitApp: {}, + }, + users: { + loadingStates: {}, + list: [], + users: [], + error: "", + currentUser: {}, + featureFlag: { + data: {}, + isFetched: true, + overriddenFlags: {}, + }, + productAlert: { + config: {}, + }, + }, + apiPane: { + isCreating: false, + isRunning: {}, + isSaving: {}, + isDeleting: {}, + isDirty: {}, + extraformData: {}, + selectedConfigTabIndex: 0, + debugger: { + open: false, + }, + }, + }, +}; +const mockStore = configureStore([]); +describe("CreateNewAppsOption", () => { + let store: any; + it("Should not render skip button if no application is present", () => { + store = mockStore(defaultStoreState); + + render( + + + + + + + , + ); + + const button = screen.queryAllByTestId("t--create-new-app-option-skip"); + + // Check that the skip button to be absent in the document + expect(button).toHaveLength(0); + }); + + it("Should render skip button if application is present", () => { + render( + + + + + + + , + ); + + const button = screen.queryAllByTestId("t--create-new-app-option-skip"); + + // Check that the skip button to be present in the document + expect(button).toHaveLength(1); + }); + + afterAll(() => { + jest.clearAllMocks(); + store.clearActions(); + }); +}); diff --git a/app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx b/app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx index c151b5663e..2de8283c35 100644 --- a/app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx +++ b/app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx @@ -140,25 +140,24 @@ const CreateNewAppsOption = ({ }; const onClickSkipButton = () => { - if (application) { - urlBuilder.updateURLParams( - { - applicationSlug: application.slug, - applicationVersion: application.applicationVersion, - applicationId: application.id, - }, - application.pages.map((page) => ({ - pageSlug: page.slug, - customSlug: page.customSlug, - pageId: page.id, - })), - ); - history.push( - builderURL({ - pageId: application.pages[0].id, - }), - ); - } + const applicationObject = application!; + urlBuilder.updateURLParams( + { + applicationSlug: applicationObject.slug, + applicationVersion: applicationObject.applicationVersion, + applicationId: applicationObject.id, + }, + applicationObject.pages.map((page) => ({ + pageSlug: page.slug, + customSlug: page.customSlug, + pageId: page.id, + })), + ); + history.push( + builderURL({ + pageId: applicationObject.pages[0].id, + }), + ); addAnalyticEventsForSkip(); }; @@ -214,15 +213,16 @@ const CreateNewAppsOption = ({ > {createMessage(GO_BACK)} - - - {createMessage(SKIP_START_WITH_USE_CASE_TEMPLATES)} - + {application && ( + + {createMessage(SKIP_START_WITH_USE_CASE_TEMPLATES)} + + )}
Date: Fri, 7 Jun 2024 15:24:07 +0530 Subject: [PATCH 04/12] test: Failing cypress tests due to removal of empty canvas prompts (#34037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR fixes impact of https://github.com/appsmithorg/appsmith/pull/33993 Refactors visual regression tests to use PageList for page generation; remove obsolete empty canvas spec and related selectors. * Removed unncessary: `cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js` * Fixes `cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js` **RCA:** The [original PR](https://github.com/appsmithorg/appsmith/pull/33993) catered to removal of empty canvas prompts and visual tests were not run leading to subsequent failures in the CI for EmptyCanvas_spec & AppPageLayout_spec. This PR caters to failing visual tests, while running `@tag.Visual` we noticed that other (unrelated) visual specs started failing. These new failures fail in local as well. Whereas they were not failing in TBP or `@tag.All` runs and `@tag.All` succeeded for this PR as well. Fixes https://github.com/appsmithorg/appsmith/issues/33874 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: b6d7f6012e0f2c3f3e030ad280354cc3d22f57ad > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Refactor** - Updated method call for adding a new page in visual regression tests to improve code clarity and maintainability. - **Chores** - Removed unused locators and declarations to clean up the codebase. --------- Co-authored-by: Apeksha Bhosale <7846888+ApekshaBhosale@users.noreply.github.com> --- .../OtherUIFeatures/EmptyCanvas_spec.js | 31 ------------------- .../VisualTests/AppPageLayout_spec.js | 3 +- app/client/cypress/locators/GeneratePage.json | 1 - app/client/cypress/support/Pages/HomePage.ts | 1 - 4 files changed, 2 insertions(+), 34 deletions(-) delete mode 100644 app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js diff --git a/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js b/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js deleted file mode 100644 index a3d9d72532..0000000000 --- a/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js +++ /dev/null @@ -1,31 +0,0 @@ -import { WIDGET } from "../../../../locators/WidgetLocators"; -import { ObjectsRegistry } from "../../../../support/Objects/Registry"; -import EditorNavigation, { - EntityType, -} from "../../../../support/Pages/EditorNavigation"; -import PageList from "../../../../support/Pages/PageList"; - -const { CommonLocators: locators, EntityExplorer: ee } = ObjectsRegistry; - -describe("Empty canvas ctas", () => { - it("1. Ctas validations", () => { - cy.wait(3000); // for page to load, failing in CI - //Ctas should not be shown in the second page - cy.get(locators._emptyCanvasCta).should("be.visible"); - PageList.AddNewPage(); - cy.get(locators._emptyCanvasCta).should("not.exist"); - EditorNavigation.SelectEntityByName("Page1", EntityType.Page); - - //Ctas should continue to show on refresh - cy.get(locators._emptyCanvasCta).should("be.visible"); - cy.reload(); - cy.get(locators._emptyCanvasCta).should("be.visible"); - - //Hide cta on adding a widget - cy.get(locators._emptyCanvasCta).should("be.visible"); - ee.DragDropWidgetNVerify(WIDGET.BUTTON, 200, 200); - cy.get(locators._emptyCanvasCta).should("not.exist"); - PageList.AddNewPage(); - cy.get(locators._emptyCanvasCta).should("not.exist"); - }); -}); diff --git a/app/client/cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js b/app/client/cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js index d46e558fa9..bc76f3bb8b 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js +++ b/app/client/cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js @@ -1,5 +1,6 @@ import homePage from "../../../../locators/HomePage"; import * as _ from "../../../../support/Objects/ObjectsCore"; +import PageList from "../../../../support/Pages/PageList"; describe("Visual regression tests", { tags: ["@tag.Visual"] }, () => { // for any changes in UI, update the screenshot in snapshot folder, to do so: @@ -18,7 +19,7 @@ describe("Visual regression tests", { tags: ["@tag.Visual"] }, () => { cy.get("#root").matchImageSnapshot("apppage"); //Layout validation for Quick page wizard - cy.get("[data-testid='generate-app']").click(); + PageList.AddNewPage(Cypress.env("MESSAGES").GENERATE_PAGE_ACTION_TITLE()); cy.wait(2000); // taking screenshot of generate crud page cy.get("#root").matchImageSnapshot("quickPageWizard"); diff --git a/app/client/cypress/locators/GeneratePage.json b/app/client/cypress/locators/GeneratePage.json index fafd5d1c4b..e6925eb1e7 100644 --- a/app/client/cypress/locators/GeneratePage.json +++ b/app/client/cypress/locators/GeneratePage.json @@ -1,5 +1,4 @@ { - "generateCRUDPageActionCard": "[data-testid='generate-app']", "selectDatasourceDropdown": "[data-testid=t--datasource-dropdown]", "datasourceDropdownOption": "[data-testid=t--datasource-dropdown-option]", "selectTableDropdown": "[data-testid=t--table-dropdown]", diff --git a/app/client/cypress/support/Pages/HomePage.ts b/app/client/cypress/support/Pages/HomePage.ts index ee55d6323b..2ae8b77ff8 100644 --- a/app/client/cypress/support/Pages/HomePage.ts +++ b/app/client/cypress/support/Pages/HomePage.ts @@ -68,7 +68,6 @@ export class HomePage { _applicationName = ".t--application-name"; private _editAppName = "bp3-editable-text-editing"; private _appMenu = ".ads-v2-menu__menu-item-children"; - _buildFromDataTableActionCard = "[data-testid='generate-app']"; private _selectRole = "//span[text()='Select a role']/ancestor::div"; private _searchInput = "input[type='text']"; _appHoverIcon = (action: string) => ".t--application-" + action + "-link"; From 282735cbf29918f1cc67aa4a019924524f662d8e Mon Sep 17 00:00:00 2001 From: Nirmal Sarswat <25587962+vivonk@users.noreply.github.com> Date: Fri, 7 Jun 2024 18:30:12 +0530 Subject: [PATCH 05/12] fix: fixing null check failure on pg branch (#34090) ## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### :mag: Cypress test results > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **Bug Fixes** - Improved the stability of the layout retrieval process by adding null checks for both `publishedPage` and `unpublishedPage` and their respective `layouts`. This prevents potential errors when these elements are missing. --- .../src/main/java/com/appsmith/server/domains/NewPage.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java index 5e890ae6e2..5295bff2ba 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java @@ -50,8 +50,11 @@ public class NewPage extends BranchAwareDomain implements Context { @JsonView(Views.Internal.class) @Override public Layout getLayout() { + if (this.getUnpublishedPage() == null || this.getUnpublishedPage().getLayouts() == null) { + return null; + } List layouts = this.getUnpublishedPage().getLayouts(); - return (layouts != null && !layouts.isEmpty()) ? layouts.get(0) : null; + return !layouts.isEmpty() ? layouts.get(0) : null; } public static class Fields extends BranchAwareDomain.Fields { From cf18829c7d577339eb678da2641463fd0a5eb56e Mon Sep 17 00:00:00 2001 From: Rajat Agrawal Date: Fri, 7 Jun 2024 18:38:42 +0530 Subject: [PATCH 06/12] chore: add first load telemetry to evaluations (#33989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR adds an attribute to evaluations span that tells whether the evaluation is first evaluation on page load or a subsequent one. This attribute will help us filter evaluations and measure gains in update evaluations and first page load evaluations separately. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 4cdd01dc24c70aa29b908420e60c984d20b2494d > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **Bug Fixes** - Added a conditional check before calling `endSpan` to handle `parentSpan` in action execution. - **Refactor** - Updated types and interfaces related to telemetry data handling to enhance type safety and clarity. - Adjusted telemetry data processing to accommodate new `SpanAttributes`. - **New Features** - Introduced filtering for telemetry span data to exclude specific entries, improving data management. --- app/client/src/UITelemetry/generateTraces.ts | 33 ++++----- .../UITelemetry/generateWebWorkerTraces.ts | 24 ++++-- app/client/src/ce/workers/common/types.ts | 3 +- .../sagas/ActionExecution/PluginActionSaga.ts | 4 +- app/client/src/utils/WorkerUtil.ts | 73 +++++++++++++------ .../workers/Evaluation/handlers/evalTree.ts | 4 + app/client/src/workers/Evaluation/types.ts | 3 +- .../workers/common/DataTreeEvaluator/index.ts | 5 +- 8 files changed, 94 insertions(+), 55 deletions(-) diff --git a/app/client/src/UITelemetry/generateTraces.ts b/app/client/src/UITelemetry/generateTraces.ts index b7e358f6a3..08b977a62d 100644 --- a/app/client/src/UITelemetry/generateTraces.ts +++ b/app/client/src/UITelemetry/generateTraces.ts @@ -14,6 +14,9 @@ import { matchBuilderPath, matchViewerPath } from "constants/routes"; const GENERATOR_TRACE = "generator-tracer"; +export type OtlpSpan = Span; +export type SpanAttributes = Attributes; + const getCommonTelemetryAttributes = () => { const pathname = window.location.pathname; const isEditorUrl = matchBuilderPath(pathname); @@ -33,16 +36,13 @@ const getCommonTelemetryAttributes = () => { export function startRootSpan( spanName: string, - spanAttributes: Attributes = {}, + spanAttributes: SpanAttributes = {}, startTime?: TimeInput, ) { const tracer = trace.getTracer(GENERATOR_TRACE); - if (!spanName) { - return; - } const commonAttributes = getCommonTelemetryAttributes(); - return tracer?.startSpan(spanName, { + return tracer.startSpan(spanName, { kind: SpanKind.CLIENT, attributes: { ...commonAttributes, @@ -56,15 +56,10 @@ export const generateContext = (span: Span) => { }; export function startNestedSpan( spanName: string, - parentSpan?: Span, - spanAttributes: Attributes = {}, + parentSpan: Span, + spanAttributes: SpanAttributes = {}, startTime?: TimeInput, ) { - if (!spanName || !parentSpan) { - // do not generate nested span without parentSpan..we cannot generate context out of it - return; - } - const parentContext = generateContext(parentSpan); const generatorTrace = trace.getTracer(GENERATOR_TRACE); @@ -81,14 +76,14 @@ export function startNestedSpan( return generatorTrace.startSpan(spanName, spanOptions, parentContext); } -export function endSpan(span?: Span) { - span?.end(); +export function endSpan(span: Span) { + span.end(); } -export function setAttributesToSpan(span: Span, spanAttributes: Attributes) { - if (!span) { - return; - } +export function setAttributesToSpan( + span: Span, + spanAttributes: SpanAttributes, +) { span.setAttributes(spanAttributes); } @@ -96,5 +91,3 @@ export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) { const parentContext = trace.setSpan(context.active(), parentSpan); return context.with(parentContext, fn); } - -export type OtlpSpan = Span; diff --git a/app/client/src/UITelemetry/generateWebWorkerTraces.ts b/app/client/src/UITelemetry/generateWebWorkerTraces.ts index 32c8ae3123..a9133cb32f 100644 --- a/app/client/src/UITelemetry/generateWebWorkerTraces.ts +++ b/app/client/src/UITelemetry/generateWebWorkerTraces.ts @@ -1,8 +1,9 @@ +import type { OtlpSpan, SpanAttributes } from "./generateTraces"; import { startNestedSpan } from "./generateTraces"; -import type { TimeInput, Attributes, Span } from "@opentelemetry/api"; +import type { TimeInput } from "@opentelemetry/api"; export interface WebworkerSpanData { - attributes: Attributes; + attributes: SpanAttributes; spanName: string; startTime: TimeInput; endTime: TimeInput; @@ -13,7 +14,7 @@ export interface WebworkerSpanData { //to regular otlp telemetry data and subsequently exported to our telemetry collector export const newWebWorkerSpanData = ( spanName: string, - attributes: Attributes, + attributes: SpanAttributes, ): WebworkerSpanData => { return { attributes, @@ -29,8 +30,8 @@ const addEndTimeForWebWorkerSpanData = (span: WebworkerSpanData) => { export const profileFn = ( spanName: string, - attributes: Attributes = {}, - allSpans: Record, + attributes: SpanAttributes = {}, + allSpans: Record, fn: (...args: any[]) => any, ) => { const span = newWebWorkerSpanData(spanName, attributes); @@ -42,7 +43,7 @@ export const profileFn = ( //convert webworker spans to OTLP spans export const convertWebworkerSpansToRegularSpans = ( - parentSpan: Span, + parentSpan: OtlpSpan, allSpans: Record = {}, ) => { Object.values(allSpans) @@ -53,3 +54,14 @@ export const convertWebworkerSpansToRegularSpans = ( span?.end(endTime); }); }; + +export const filterSpanData = ( + spanData: Record, +): Record => { + return Object.keys(spanData) + .filter((key) => !key.startsWith("__")) + .reduce>((obj, key) => { + obj[key] = spanData[key] as WebworkerSpanData; + return obj; + }, {}); +}; diff --git a/app/client/src/ce/workers/common/types.ts b/app/client/src/ce/workers/common/types.ts index e931d1c291..7fdccaa133 100644 --- a/app/client/src/ce/workers/common/types.ts +++ b/app/client/src/ce/workers/common/types.ts @@ -1,4 +1,5 @@ import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; +import type { SpanAttributes } from "UITelemetry/generateTraces"; export enum AppsmithWorkers { LINT_WORKER = "LINT_WORKER", @@ -12,5 +13,5 @@ export enum WorkerErrorTypes { export interface WorkerRequest { method: TActions; data: TData; - webworkerTelemetry: Record; + webworkerTelemetry: Record; } diff --git a/app/client/src/sagas/ActionExecution/PluginActionSaga.ts b/app/client/src/sagas/ActionExecution/PluginActionSaga.ts index 8da86d8cc3..f474374a59 100644 --- a/app/client/src/sagas/ActionExecution/PluginActionSaga.ts +++ b/app/client/src/sagas/ActionExecution/PluginActionSaga.ts @@ -1662,7 +1662,9 @@ function* handleUpdateActionData( EVAL_WORKER_ACTIONS.UPDATE_ACTION_DATA, actionDataPayload, ); - endSpan(parentSpan); + if (parentSpan) { + endSpan(parentSpan); + } } export function* watchPluginActionExecutionSagas() { diff --git a/app/client/src/utils/WorkerUtil.ts b/app/client/src/utils/WorkerUtil.ts index a431b12c51..b9033f6a29 100644 --- a/app/client/src/utils/WorkerUtil.ts +++ b/app/client/src/utils/WorkerUtil.ts @@ -5,11 +5,16 @@ import { uniqueId } from "lodash"; import log from "loglevel"; import type { TMessage } from "./MessageUtil"; import { MessageType, sendMessage } from "./MessageUtil"; -import type { OtlpSpan } from "UITelemetry/generateTraces"; -import { endSpan, startRootSpan } from "UITelemetry/generateTraces"; +import type { OtlpSpan, SpanAttributes } from "UITelemetry/generateTraces"; +import { + endSpan, + setAttributesToSpan, + startRootSpan, +} from "UITelemetry/generateTraces"; import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; import { convertWebworkerSpansToRegularSpans, + filterSpanData, newWebWorkerSpanData, } from "UITelemetry/generateWebWorkerTraces"; @@ -156,33 +161,35 @@ export class GracefulWorkerService { startTime, webworkerTelemetry, }: { - webworkerTelemetry: Record; + webworkerTelemetry: + | Record + | undefined; rootSpan: OtlpSpan | undefined; method: string; startTime: number; endTime: number; }) { - const webworkerTelemetryResponse = webworkerTelemetry as Record< - string, - WebworkerSpanData - >; - - if (webworkerTelemetryResponse) { - const { transferDataToMainThread } = webworkerTelemetryResponse; - if (transferDataToMainThread) { - transferDataToMainThread.endTime = Date.now(); - } - /// Add the completeWebworkerComputation span to the root span - webworkerTelemetryResponse["completeWebworkerComputation"] = { - startTime, - endTime, - attributes: {}, - spanName: "completeWebworkerComputation", - }; + if (!webworkerTelemetry) { + return; } + + const { transferDataToMainThread } = webworkerTelemetry; + if (transferDataToMainThread) { + transferDataToMainThread.endTime = Date.now(); + } + /// Add the completeWebworkerComputation span to the root span + webworkerTelemetry["completeWebworkerComputation"] = { + startTime, + endTime, + attributes: {}, + spanName: "completeWebworkerComputation", + }; //we are attaching the child spans to the root span over here rootSpan && - convertWebworkerSpansToRegularSpans(rootSpan, webworkerTelemetryResponse); + convertWebworkerSpansToRegularSpans( + rootSpan, + filterSpanData(webworkerTelemetry), + ); //genereate separate completeWebworkerComputationRoot root span // this span does not contain any child spans, it just captures the webworker computation alone @@ -218,11 +225,15 @@ export class GracefulWorkerService { let timeTaken; const rootSpan = startRootSpan(method); - const webworkerTelemetryData: Record = { + const webworkerTelemetryData: Record< + string, + WebworkerSpanData | SpanAttributes + > = { transferDataToWorkerThread: newWebWorkerSpanData( "transferDataToWorkerThread", {}, ), + __spanAttributes: {}, }; const body = { @@ -231,6 +242,11 @@ export class GracefulWorkerService { webworkerTelemetry: webworkerTelemetryData, }; + let webworkerTelemetryResponse: Record< + string, + WebworkerSpanData | SpanAttributes + > = {}; + try { sendMessage.call(this._Worker, { messageType: MessageType.REQUEST, @@ -241,9 +257,10 @@ export class GracefulWorkerService { // The `this._broker` method is listening to events and will pass response to us over this channel. const response = yield take(ch); const { data, endTime, startTime } = response; - const { webworkerTelemetry } = data; + webworkerTelemetryResponse = data.webworkerTelemetry; + this.addChildSpansToRootSpan({ - webworkerTelemetry, + webworkerTelemetry: webworkerTelemetryResponse, rootSpan, method, startTime, @@ -268,6 +285,14 @@ export class GracefulWorkerService { log.debug(` Worker ${method} took ${timeTaken}ms`); log.debug(` Transfer ${method} took ${transferTime}ms`); } + + if (webworkerTelemetryResponse) { + setAttributesToSpan( + rootSpan, + webworkerTelemetryResponse.__spanAttributes as SpanAttributes, + ); + } + endSpan(rootSpan); // Cleanup ch.close(); diff --git a/app/client/src/workers/Evaluation/handlers/evalTree.ts b/app/client/src/workers/Evaluation/handlers/evalTree.ts index 46160713e9..4c49a6c148 100644 --- a/app/client/src/workers/Evaluation/handlers/evalTree.ts +++ b/app/client/src/workers/Evaluation/handlers/evalTree.ts @@ -34,6 +34,7 @@ import { profileFn, newWebWorkerSpanData, } from "UITelemetry/generateWebWorkerTraces"; +import type { SpanAttributes } from "UITelemetry/generateTraces"; import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer"; import type { MetaWidgetsReduxState } from "reducers/entityReducers/metaWidgetsReducer"; @@ -85,6 +86,9 @@ export function evalTree(request: EvalWorkerSyncRequest) { let isNewTree = false; try { + (webworkerTelemetry.__spanAttributes as SpanAttributes)["firstEvaluation"] = + !dataTreeEvaluator; + if (!dataTreeEvaluator) { isCreateFirstTree = true; replayMap = replayMap || {}; diff --git a/app/client/src/workers/Evaluation/types.ts b/app/client/src/workers/Evaluation/types.ts index c93e566dca..e151e520eb 100644 --- a/app/client/src/workers/Evaluation/types.ts +++ b/app/client/src/workers/Evaluation/types.ts @@ -16,6 +16,7 @@ import type { WorkerRequest } from "@appsmith/workers/common/types"; import type { DataTreeDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; import type { APP_MODE } from "entities/App"; import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; +import type { SpanAttributes } from "UITelemetry/generateTraces"; import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; export type EvalWorkerSyncRequest = WorkerRequest< @@ -59,6 +60,6 @@ export interface EvalTreeResponseData { isNewWidgetAdded: boolean; undefinedEvalValuesMap: Record; jsVarsCreatedEvent?: { path: string; type: string }[]; - webworkerTelemetry?: Record; + webworkerTelemetry?: Record; updates: string; } diff --git a/app/client/src/workers/common/DataTreeEvaluator/index.ts b/app/client/src/workers/common/DataTreeEvaluator/index.ts index fe938bb0d4..e658728d9e 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/index.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/index.ts @@ -137,6 +137,7 @@ import { profileFn, type WebworkerSpanData, } from "UITelemetry/generateWebWorkerTraces"; +import type { SpanAttributes } from "UITelemetry/generateTraces"; import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; import generateOverrideContext from "@appsmith/workers/Evaluation/generateOverrideContext"; @@ -233,7 +234,7 @@ export default class DataTreeEvaluator { setupFirstTree( unEvalTree: any, configTree: ConfigTree, - webworkerTelemetry: Record = {}, + webworkerTelemetry: Record = {}, ): { jsUpdates: Record; evalOrder: string[]; @@ -489,7 +490,7 @@ export default class DataTreeEvaluator { setupUpdateTree( unEvalTree: any, configTree: ConfigTree, - webworkerTelemetry: Record = {}, + webworkerTelemetry: Record = {}, affectedJSObjects: AffectedJSObjects = { isAllAffected: false, ids: [] }, ): { unEvalUpdates: DataTreeDiff[]; From bf34395a5703968611b7e913697ff202e13bcffa Mon Sep 17 00:00:00 2001 From: NandanAnantharamu <67676905+NandanAnantharamu@users.noreply.github.com> Date: Fri, 7 Jun 2024 18:56:02 +0530 Subject: [PATCH 07/12] test: flay table header validation (#34080) Xpath used to read the header text is flaky. Solution: Replacing xpath with css locator to make it stable EE PR: https://github.com/appsmithorg/appsmith-ee/pull/4382 ## Summary by CodeRabbit - **Tests** - Updated spec names and file paths for tests related to Community Issues and TableV2 widget. - **Refactor** - Simplified the selector for table headers to improve readability and maintainability. --- app/client/cypress/support/Pages/Table.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/client/cypress/support/Pages/Table.ts b/app/client/cypress/support/Pages/Table.ts index 7847e109e1..bac5097877 100644 --- a/app/client/cypress/support/Pages/Table.ts +++ b/app/client/cypress/support/Pages/Table.ts @@ -37,9 +37,7 @@ export class Table { private assertHelper = ObjectsRegistry.AssertHelper; private _tableWrap = "//div[contains(@class,'tableWrap')]"; - private _tableHeader = - this._tableWrap + - "//div[contains(@class,'thead')]//div[contains(@class,'tr')][1]"; + private _tableHeader = ".thead div[role=columnheader]"; private _columnHeader = (columnName: string) => this._tableWrap + "//div[contains(@class,'thead')]//div[contains(@class,'tr')][1]//div[@role='columnheader']//div[contains(text(),'" + @@ -258,7 +256,7 @@ export class Table { } public AssertTableHeaderOrder(expectedOrder: string) { - cy.xpath(this._tableHeader) + cy.get(this._tableHeader) .invoke("text") .then((x) => { expect(x).to.eq(expectedOrder); From 12716f4a4430987ae5b7332dddc4c0015a8b232d Mon Sep 17 00:00:00 2001 From: Anagh Hegde Date: Fri, 7 Jun 2024 19:29:06 +0530 Subject: [PATCH 08/12] test: fix hung tests in CI (#34027) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fix hung tests on the CI Fixes #34026 ## Automation /ok-to-test tags="@tag.Templates" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: c3c9ab9db85850d7323658b364e65a095d769e9c > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **Refactor** - Improved test setup and scenarios in caching-related tests. - Replaced `cloudServicesConfig` with `baseUrl` for better clarity and maintainability in test methods. --- ...bleTemplateHelperTemplateJsonDataTest.java | 312 +++++++++--------- ...bleTemplateHelperTemplateMetadataTest.java | 20 +- 2 files changed, 167 insertions(+), 165 deletions(-) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateJsonDataTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateJsonDataTest.java index 5da52cf69b..6cbb82943f 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateJsonDataTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateJsonDataTest.java @@ -1,8 +1,34 @@ package com.appsmith.server.helpers; +import com.appsmith.server.constants.ArtifactType; +import com.appsmith.server.domains.Application; +import com.appsmith.server.dtos.ApplicationJson; +import com.appsmith.server.dtos.CacheableApplicationJson; +import com.appsmith.server.solutions.ApplicationPermission; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.gson.Gson; +import mockwebserver3.MockResponse; +import mockwebserver3.MockWebServer; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.test.mock.mockito.SpyBean; import org.springframework.test.context.junit.jupiter.SpringExtension; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.io.IOException; +import java.time.Instant; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; /** * This test is written based on the inspiration from the tutorial: @@ -11,155 +37,139 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; @ExtendWith(SpringExtension.class) @SpringBootTest public class CacheableTemplateHelperTemplateJsonDataTest { - // private static final ObjectMapper objectMapper = new ObjectMapper(); - // private static MockWebServer mockCloudServices; - // - // @MockBean - // ApplicationPermission applicationPermission; - // - // @MockBean - // private CloudServicesConfig cloudServicesConfig; - // - // @Autowired - // CacheableTemplateHelper cacheableTemplateHelper; - // - // @SpyBean - // CacheableTemplateHelper spyCacheableTemplateHelper; - // - // @BeforeAll - // public static void setUp() throws IOException { - // mockCloudServices = new MockWebServer(); - // mockCloudServices.start(); - // } - // - // @AfterAll - // public static void tearDown() throws IOException { - // mockCloudServices.shutdown(); - // } - // - // @BeforeEach - // public void initialize() { - // String baseUrl = String.format("http://localhost:%s", mockCloudServices.getPort()); - // - // // mock the cloud services config so that it returns mock server url as cloud - // // service base url - // Mockito.when(cloudServicesConfig.getBaseUrl()).thenReturn(baseUrl); - // } - // - // private ApplicationTemplate create(String id, String title) { - // ApplicationTemplate applicationTemplate = new ApplicationTemplate(); - // applicationTemplate.setId(id); - // applicationTemplate.setTitle(title); - // return applicationTemplate; - // } - // - // /* Scenarios covered via this test: - // * 1. CacheableTemplateHelper doesn't have the POJO or has an empty POJO. - // * 2. Fetch the templates via the normal flow by mocking CS. - // * 3. Check if the CacheableTemplateHelper.getApplicationTemplateList() is the same as the object returned by - // the normal flow function. This will ensure that the cache is being set correctly. - // * 4. From the above steps we now have the cache set. - // * 5. Fetch the templates again, verify the data is the same as the one fetched in step 2. - // * 6. Verify the cache is used and not the mock. This is done by asserting the lastUpdated time of the cache. - // */ - // @Test - // public void getApplicationJson_cacheIsEmpty_VerifyDataSavedInCache() throws JsonProcessingException { - // ApplicationJson applicationJson = new ApplicationJson(); - // applicationJson.setArtifactJsonType(ArtifactType.APPLICATION); - // applicationJson.setExportedApplication(new Application()); - // - // assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) - // .isEqualTo(0); - // - // // mock the server to return a template when it's called - // mockCloudServices.enqueue(new MockResponse() - // .setBody(objectMapper.writeValueAsString(applicationJson)) - // .addHeader("Content-Type", "application/json")); - // - // Mono templateListMono = - // cacheableTemplateHelper.getApplicationByTemplateId("templateId", - // cloudServicesConfig.getBaseUrl()); - // - // final Instant[] timeFromCache = {Instant.now()}; - // // make sure we've received the response returned by the mockCloudServices - // StepVerifier.create(templateListMono) - // .assertNext(cacheableApplicationJson1 -> { - // assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); - // timeFromCache[0] = cacheableApplicationJson1.getCacheExpiryTime(); - // }) - // .verifyComplete(); - // - // // Fetch the same application json again and verify the time stamp to confirm value is coming from POJO - // StepVerifier.create(cacheableTemplateHelper.getApplicationByTemplateId( - // "templateId", cloudServicesConfig.getBaseUrl())) - // .assertNext(cacheableApplicationJson1 -> { - // assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); - // assertThat(cacheableApplicationJson1.getCacheExpiryTime()).isEqualTo(timeFromCache[0]); - // }) - // .verifyComplete(); - // assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) - // .isEqualTo(1); - // } - // - // /* Scenarios covered via this test: - // * 1. Mock the cache isCacheValid to return false, so the cache is invalidated - // * 2. Fetch the templates again, verify the data is from the mock and not from the cache. - // */ - // @Test - // public void getApplicationJson_cacheIsDirty_verifyDataIsFetchedFromSource() { - // ApplicationJson applicationJson = new ApplicationJson(); - // Application test = new Application(); - // test.setName("New Application"); - // applicationJson.setArtifactJsonType(ArtifactType.APPLICATION); - // applicationJson.setExportedApplication(test); - // - // // mock the server to return the above three templates - // mockCloudServices.enqueue(new MockResponse() - // .setBody(new Gson().toJson(applicationJson)) - // .addHeader("Content-Type", "application/json")); - // - // Mockito.doReturn(false).when(spyCacheableTemplateHelper).isCacheValid(any()); - // - // // make sure we've received the response returned by the mock - // StepVerifier.create(spyCacheableTemplateHelper.getApplicationByTemplateId( - // "templateId", cloudServicesConfig.getBaseUrl())) - // .assertNext(cacheableApplicationJson1 -> { - // assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); - // assertThat(cacheableApplicationJson1 - // .getApplicationJson() - // .getExportedApplication() - // .getName()) - // .isEqualTo("New Application"); - // }) - // .verifyComplete(); - // } - // - // @Test - // public void getApplicationJson_cacheKeyIsMissing_verifyDataIsFetchedFromSource() { - // ApplicationJson applicationJson1 = new ApplicationJson(); - // Application application = new Application(); - // application.setName("Test Application"); - // applicationJson1.setArtifactJsonType(ArtifactType.APPLICATION); - // applicationJson1.setExportedApplication(application); - // - // assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) - // .isEqualTo(1); - // - // mockCloudServices.enqueue(new MockResponse() - // .setBody(new Gson().toJson(applicationJson1)) - // .addHeader("Content-Type", "application/json")); - // - // // make sure we've received the response returned by the mock - // StepVerifier.create(cacheableTemplateHelper.getApplicationByTemplateId( - // "templateId1", cloudServicesConfig.getBaseUrl())) - // .assertNext(cacheableApplicationJson1 -> { - // assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); - // assertThat(cacheableApplicationJson1 - // .getApplicationJson() - // .getExportedApplication() - // .getName()) - // .isEqualTo("Test Application"); - // }) - // .verifyComplete(); - // } + private static final ObjectMapper objectMapper = new ObjectMapper(); + private static MockWebServer mockCloudServices; + + @MockBean + ApplicationPermission applicationPermission; + + @Autowired + CacheableTemplateHelper cacheableTemplateHelper; + + @SpyBean + CacheableTemplateHelper spyCacheableTemplateHelper; + + String baseUrl; + + @BeforeAll + public static void setUp() throws IOException { + mockCloudServices = new MockWebServer(); + mockCloudServices.start(); + } + + @AfterAll + public static void tearDown() throws IOException { + mockCloudServices.shutdown(); + } + + @BeforeEach + public void initialize() { + baseUrl = String.format("http://localhost:%s", mockCloudServices.getPort()); + } + + /* Scenarios covered via this test: + * 1. CacheableTemplateHelper doesn't have the POJO or has an empty POJO. + * 2. Fetch the templates via the normal flow by mocking CS. + * 3. Check if the CacheableTemplateHelper.getApplicationTemplateList() is the same as the object returned by + the normal flow function. This will ensure that the cache is being set correctly. + * 4. From the above steps we now have the cache set. + * 5. Fetch the templates again, verify the data is the same as the one fetched in step 2. + * 6. Verify the cache is used and not the mock. This is done by asserting the lastUpdated time of the cache. + */ + @Test + public void getApplicationJson_cacheIsEmpty_VerifyDataSavedInCache() throws JsonProcessingException { + ApplicationJson applicationJson = new ApplicationJson(); + applicationJson.setArtifactJsonType(ArtifactType.APPLICATION); + applicationJson.setExportedApplication(new Application()); + + assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) + .isEqualTo(0); + + // mock the server to return a template when it's called + mockCloudServices.enqueue(new MockResponse() + .setBody(objectMapper.writeValueAsString(applicationJson)) + .addHeader("Content-Type", "application/json")); + + Mono templateListMono = + cacheableTemplateHelper.getApplicationByTemplateId("templateId", baseUrl); + + final Instant[] timeFromCache = {Instant.now()}; + // make sure we've received the response returned by the mockCloudServices + StepVerifier.create(templateListMono) + .assertNext(cacheableApplicationJson1 -> { + assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); + timeFromCache[0] = cacheableApplicationJson1.getCacheExpiryTime(); + }) + .verifyComplete(); + + // Fetch the same application json again and verify the time stamp to confirm value is coming from POJO + StepVerifier.create(cacheableTemplateHelper.getApplicationByTemplateId("templateId", baseUrl)) + .assertNext(cacheableApplicationJson1 -> { + assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); + assertThat(cacheableApplicationJson1.getCacheExpiryTime()).isEqualTo(timeFromCache[0]); + }) + .verifyComplete(); + assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) + .isEqualTo(1); + } + + /* Scenarios covered via this test: + * 1. Mock the cache isCacheValid to return false, so the cache is invalidated + * 2. Fetch the templates again, verify the data is from the mock and not from the cache. + */ + @Test + public void getApplicationJson_cacheIsDirty_verifyDataIsFetchedFromSource() { + ApplicationJson applicationJson = new ApplicationJson(); + Application test = new Application(); + test.setName("New Application"); + applicationJson.setArtifactJsonType(ArtifactType.APPLICATION); + applicationJson.setExportedApplication(test); + + // mock the server to return the above three templates + mockCloudServices.enqueue(new MockResponse() + .setBody(new Gson().toJson(applicationJson)) + .addHeader("Content-Type", "application/json")); + + Mockito.doReturn(false).when(spyCacheableTemplateHelper).isCacheValid(any()); + + // make sure we've received the response returned by the mock + StepVerifier.create(spyCacheableTemplateHelper.getApplicationByTemplateId("templateId", baseUrl)) + .assertNext(cacheableApplicationJson1 -> { + assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); + assertThat(cacheableApplicationJson1 + .getApplicationJson() + .getExportedApplication() + .getName()) + .isEqualTo("New Application"); + }) + .verifyComplete(); + } + + @Test + public void getApplicationJson_cacheKeyIsMissing_verifyDataIsFetchedFromSource() { + ApplicationJson applicationJson1 = new ApplicationJson(); + Application application = new Application(); + application.setName("Test Application"); + applicationJson1.setArtifactJsonType(ArtifactType.APPLICATION); + applicationJson1.setExportedApplication(application); + + assertThat(cacheableTemplateHelper.getCacheableApplicationJsonMap().size()) + .isEqualTo(1); + + mockCloudServices.enqueue(new MockResponse() + .setBody(new Gson().toJson(applicationJson1)) + .addHeader("Content-Type", "application/json")); + + // make sure we've received the response returned by the mock + StepVerifier.create(cacheableTemplateHelper.getApplicationByTemplateId("templateId1", baseUrl)) + .assertNext(cacheableApplicationJson1 -> { + assertThat(cacheableApplicationJson1.getApplicationJson()).isNotNull(); + assertThat(cacheableApplicationJson1 + .getApplicationJson() + .getExportedApplication() + .getName()) + .isEqualTo("Test Application"); + }) + .verifyComplete(); + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateMetadataTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateMetadataTest.java index 0e5cf1d5ef..aa2d273992 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateMetadataTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/CacheableTemplateHelperTemplateMetadataTest.java @@ -1,6 +1,5 @@ package com.appsmith.server.helpers; -import com.appsmith.server.configurations.CloudServicesConfig; import com.appsmith.server.dtos.ApplicationTemplate; import com.appsmith.server.dtos.CacheableApplicationTemplate; import com.appsmith.server.solutions.ApplicationPermission; @@ -11,7 +10,6 @@ import mockwebserver3.MockWebServer; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; @@ -32,7 +30,6 @@ import static org.mockito.ArgumentMatchers.any; @ExtendWith(SpringExtension.class) @SpringBootTest -@Disabled public class CacheableTemplateHelperTemplateMetadataTest { private static final ObjectMapper objectMapper = new ObjectMapper(); @@ -41,15 +38,14 @@ public class CacheableTemplateHelperTemplateMetadataTest { @MockBean ApplicationPermission applicationPermission; - @MockBean - private CloudServicesConfig cloudServicesConfig; - @Autowired CacheableTemplateHelper cacheableTemplateHelper; @SpyBean CacheableTemplateHelper spyCacheableTemplateHelper; + String baseUrl; + @BeforeAll public static void setUp() throws IOException { mockCloudServices = new MockWebServer(); @@ -63,11 +59,7 @@ public class CacheableTemplateHelperTemplateMetadataTest { @BeforeEach public void initialize() { - String baseUrl = String.format("http://localhost:%s", mockCloudServices.getPort()); - - // mock the cloud services config so that it returns mock server url as cloud - // service base url - Mockito.when(cloudServicesConfig.getBaseUrl()).thenReturn(baseUrl); + baseUrl = String.format("http://localhost:%s", mockCloudServices.getPort()); } private ApplicationTemplate create(String id, String title) { @@ -99,7 +91,7 @@ public class CacheableTemplateHelperTemplateMetadataTest { .addHeader("Content-Type", "application/json")); Mono templateListMono = - cacheableTemplateHelper.getTemplates("recently-used", cloudServicesConfig.getBaseUrl()); + cacheableTemplateHelper.getTemplates("recently-used", baseUrl); final Instant[] timeFromCache = {Instant.now()}; StepVerifier.create(templateListMono) @@ -114,7 +106,7 @@ public class CacheableTemplateHelperTemplateMetadataTest { .verifyComplete(); // Fetch again and verify the time stamp to confirm value is coming from POJO - StepVerifier.create(cacheableTemplateHelper.getTemplates("recently-used", cloudServicesConfig.getBaseUrl())) + StepVerifier.create(cacheableTemplateHelper.getTemplates("recently-used", baseUrl)) .assertNext(cacheableApplicationTemplate1 -> { assertThat(cacheableApplicationTemplate1.getApplicationTemplateList()) .hasSize(3); @@ -141,7 +133,7 @@ public class CacheableTemplateHelperTemplateMetadataTest { .setBody(objectMapper.writeValueAsString(List.of(templateFour, templateFive, templateSix))) .addHeader("Content-Type", "application/json")); - StepVerifier.create(spyCacheableTemplateHelper.getTemplates("recently-used", cloudServicesConfig.getBaseUrl())) + StepVerifier.create(spyCacheableTemplateHelper.getTemplates("recently-used", baseUrl)) .assertNext(cacheableApplicationTemplate1 -> { assertThat(cacheableApplicationTemplate1.getApplicationTemplateList()) .hasSize(3); From c605677f903e2594df8727f7ceb15a816f21f2bf Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Fri, 7 Jun 2024 19:35:01 +0530 Subject: [PATCH 09/12] chore: Don't deserialize any request body to `Layout` (#34086) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The presence of `@JsonProperty` screws up the way objects are stored in Postgres. This PR gets rid of it for the `Layout` class. **/test sanity** > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 3256d44dfdf436c8eec6fd9db81e5cbfc98758cd > Cypress dashboard url: Click here! ## Summary by CodeRabbit - **Refactor** - Improved serialization and deserialization process for layouts by updating annotations and introducing a new `LayoutDTO` class. - Simplified page layout updates by using `LayoutDTO` instead of `Layout`. - **Tests** - Updated tests to accommodate changes in layout handling, ensuring consistency and correctness. --- .../java/com/appsmith/server/domains/Layout.java | 2 -- .../appsmith/server/dtos/PageCreationDTO.java | 16 ++++++++++++++-- .../server/dtos/UpdateMultiplePageLayoutDTO.java | 6 ++++-- .../layouts/UpdateLayoutServiceCEImpl.java | 8 +++----- .../server/services/LayoutActionServiceTest.java | 6 +++--- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Layout.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Layout.java index 2c88a5cc8e..c99949d8d8 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Layout.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Layout.java @@ -7,7 +7,6 @@ import com.appsmith.external.views.Git; import com.appsmith.external.views.Views; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.CompareDslActionDTO; -import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; import lombok.AccessLevel; import lombok.Getter; @@ -50,7 +49,6 @@ public class Layout { // this attribute will be used to display errors caused white calculating allOnLoadAction // PageLoadActionsUtilCEImpl.java - @JsonProperty(access = JsonProperty.Access.READ_ONLY) @JsonView({Views.Public.class, Views.Export.class}) List layoutOnLoadActionErrors; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java index cf567a80a2..dc44f933e3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java @@ -1,22 +1,34 @@ package com.appsmith.server.dtos; +import com.appsmith.external.dtos.DslExecutableDTO; import com.appsmith.server.domains.Layout; import com.appsmith.server.meta.validations.FileName; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.Size; +import net.minidev.json.JSONObject; import java.util.List; +import java.util.Set; public record PageCreationDTO( @FileName(message = "Page names must be valid file names", isNullValid = false) @Size(max = 30) String name, @NotEmpty @Size(min = 24, max = 50) String applicationId, - @NotEmpty List layouts) { + @NotEmpty List layouts) { + + public record LayoutDTO(JSONObject dsl, List> layoutOnLoadActions) {} public PageDTO toPageDTO() { final PageDTO page = new PageDTO(); page.setName(name.trim()); page.setApplicationId(applicationId); - page.setLayouts(layouts); + page.setLayouts(layouts.stream() + .map(layoutDto -> { + final Layout l = new Layout(); + l.setDsl(layoutDto.dsl); + l.setLayoutOnLoadActions(layoutDto.layoutOnLoadActions); + return l; + }) + .toList()); return page; } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UpdateMultiplePageLayoutDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UpdateMultiplePageLayoutDTO.java index ee21c95e0d..a401ac5945 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UpdateMultiplePageLayoutDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/UpdateMultiplePageLayoutDTO.java @@ -1,10 +1,10 @@ package com.appsmith.server.dtos; -import com.appsmith.server.domains.Layout; import jakarta.validation.Valid; import jakarta.validation.constraints.NotNull; import lombok.Getter; import lombok.Setter; +import net.minidev.json.JSONObject; import java.util.List; @@ -22,6 +22,8 @@ public class UpdateMultiplePageLayoutDTO { @NotNull private String layoutId; - private Layout layout; + private LayoutDTO layout; } + + public record LayoutDTO(JSONObject dsl) {} } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java index 53ced69c3e..3518b3e9c9 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java @@ -244,12 +244,10 @@ public class UpdateLayoutServiceCEImpl implements UpdateLayoutServiceCE { List> monoList = new ArrayList<>(); for (UpdateMultiplePageLayoutDTO.UpdatePageLayoutDTO pageLayout : updateMultiplePageLayoutDTO.getPageLayouts()) { + final Layout layout = new Layout(); + layout.setDsl(pageLayout.getLayout().dsl()); Mono updatedLayoutMono = this.updateLayout( - pageLayout.getPageId(), - defaultApplicationId, - pageLayout.getLayoutId(), - pageLayout.getLayout(), - branchName); + pageLayout.getPageId(), defaultApplicationId, pageLayout.getLayoutId(), layout, branchName); monoList.add(updatedLayoutMono); } return Flux.merge(monoList).then(Mono.just(monoList.size())); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java index 9afcf75a56..76ec9d3e78 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java @@ -1023,13 +1023,13 @@ public class LayoutActionServiceTest { for (int i = 0; i < testPages.size(); i++) { PageDTO page = testPages.get(i); - Layout layout = page.getLayouts().get(0); - layout.setDsl(createTestDslWithTestWidget("Layout" + (i + 1))); + final UpdateMultiplePageLayoutDTO.LayoutDTO layout = + new UpdateMultiplePageLayoutDTO.LayoutDTO(createTestDslWithTestWidget("Layout" + (i + 1))); UpdateMultiplePageLayoutDTO.UpdatePageLayoutDTO pageLayoutDTO = new UpdateMultiplePageLayoutDTO.UpdatePageLayoutDTO(); pageLayoutDTO.setPageId(page.getId()); - pageLayoutDTO.setLayoutId(layout.getId()); + pageLayoutDTO.setLayoutId(page.getLayouts().get(0).getId()); pageLayoutDTO.setLayout(layout); multiplePageLayoutDTO.getPageLayouts().add(pageLayoutDTO); } From 347d4318876278db4ad738cb83d442765265ddba Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Mon, 10 Jun 2024 09:23:43 +0530 Subject: [PATCH 10/12] chore: Add time to logs from entrypoint.sh (#34116) Logs messages from `entrypoint.sh` and the other `run-*.sh` scripts don't show timestamp today, and its getting hard to see the order of things in the logs, especially between different processes. This PR adds a command `tlog` to print logs with UTC timestamp prefixed. We're only using it in `entrypoint.sh` now, but follow-up PR(s) will add it to the other `run-*` scripts as well. ## Summary by CodeRabbit - **Chores** - Updated Dockerfile to include `/opt/bin` in the `PATH` and modified permissions settings for executable files. - **Refactor** - Enhanced logging in the entrypoint script by replacing `echo` statements with `tlog` for better clarity and debugging. - **New Features** - Introduced `tlog`, a new shell script for consistent and timestamped logging. --- Dockerfile | 4 +- deploy/docker/fs/opt/appsmith/entrypoint.sh | 77 ++++++++++----------- deploy/docker/fs/opt/bin/tlog | 5 ++ 3 files changed, 45 insertions(+), 41 deletions(-) create mode 100644 deploy/docker/fs/opt/bin/tlog diff --git a/Dockerfile b/Dockerfile index 795368d8fc..8b72f70618 100644 --- a/Dockerfile +++ b/Dockerfile @@ -30,10 +30,10 @@ COPY ./app/client/build editor/ # Add RTS - Application Layer COPY ./app/client/packages/rts/dist rts/ -ENV PATH /opt/appsmith/utils/node_modules/.bin:/opt/java/bin:/opt/node/bin:$PATH +ENV PATH /opt/bin:/opt/appsmith/utils/node_modules/.bin:/opt/java/bin:/opt/node/bin:$PATH RUN cd ./utils && npm install --only=prod && npm install --only=prod -g . && cd - \ - && chmod +x *.sh /watchtower-hooks/*.sh \ + && chmod +x /opt/bin/* *.sh /watchtower-hooks/*.sh \ # Disable setuid/setgid bits for the files inside container. && find / \( -path /proc -prune \) -o \( \( -perm -2000 -o -perm -4000 \) -print -exec chmod -s '{}' + \) || true \ && mkdir -p /.mongodb/mongosh /appsmith-stacks \ diff --git a/deploy/docker/fs/opt/appsmith/entrypoint.sh b/deploy/docker/fs/opt/appsmith/entrypoint.sh index d5899e33ae..59dea4b7e5 100644 --- a/deploy/docker/fs/opt/appsmith/entrypoint.sh +++ b/deploy/docker/fs/opt/appsmith/entrypoint.sh @@ -2,7 +2,7 @@ set -e -echo "Running as: $(id)" +tlog "Running as: $(id)" stacks_path=/appsmith-stacks @@ -48,7 +48,7 @@ init_env_file() { # Build an env file with current env variables. We single-quote the values, as well as escaping any single-quote characters. printenv | grep -E '^APPSMITH_|^MONGO_' | sed "s/'/'\\\''/g; s/=/='/; s/$/'/" > "$TMP/pre-define.env" - echo "Initialize .env file" + tlog "Initialize .env file" if ! [[ -e "$ENV_PATH" ]]; then # Generate new docker.env file when initializing container for first time or in Heroku which does not have persistent volume echo "Generating default configuration file" @@ -74,7 +74,7 @@ init_env_file() { fi - echo "Load environment configuration" + tlog "Load environment configuration" set -o allexport . "$ENV_PATH" . "$TMP/pre-define.env" @@ -111,20 +111,20 @@ if [[ -n "${FILESTORE_IP_ADDRESS-}" ]]; then FILESTORE_IP_ADDRESS="$(echo "$FILESTORE_IP_ADDRESS" | xargs)" FILE_SHARE_NAME="$(echo "$FILE_SHARE_NAME" | xargs)" - echo "Running appsmith for cloudRun" - echo "creating mount point" + tlog "Running appsmith for cloudRun" + tlog "creating mount point" mkdir -p "$stacks_path" - echo "Mounting File Sytem" + tlog "Mounting File Sytem" mount -t nfs -o nolock "$FILESTORE_IP_ADDRESS:/$FILE_SHARE_NAME" /appsmith-stacks - echo "Mounted File Sytem" - echo "Setting HOSTNAME for Cloudrun" + tlog "Mounted File Sytem" + tlog "Setting HOSTNAME for Cloudrun" export HOSTNAME="cloudrun" fi function get_maximum_heap() { resource=$(ulimit -u) - echo "Resource : $resource" + tlog "Resource : $resource" if [[ "$resource" -le 256 ]]; then maximum_heap=128 elif [[ "$resource" -le 512 ]]; then @@ -140,7 +140,7 @@ function setup_backend_heap_arg() { unset_unused_variables() { # Check for enviroment vairalbes - echo "Checking environment configuration" + tlog "Checking environment configuration" if [[ -z "${APPSMITH_MAIL_ENABLED}" ]]; then unset APPSMITH_MAIL_ENABLED # If this field is empty is might cause application crash fi @@ -169,7 +169,7 @@ unset_unused_variables() { } configure_database_connection_url() { - echo "Configuring database connection URL" + tlog "Configuring database connection URL" isPostgresUrl=0 isMongoUrl=0 # Check if APPSMITH_DB_URL is not set @@ -186,17 +186,17 @@ configure_database_connection_url() { } check_db_uri() { - echo "Checking APPSMITH_DB_URL" + tlog "Checking APPSMITH_DB_URL" isUriLocal=1 if [[ $APPSMITH_DB_URL == *"localhost"* || $APPSMITH_DB_URL == *"127.0.0.1"* ]]; then - echo "Detected local DB" + tlog "Detected local DB" isUriLocal=0 fi } init_mongodb() { if [[ $isUriLocal -eq 0 ]]; then - echo "Initializing local database" + tlog "Initializing local database" MONGO_DB_PATH="$stacks_path/data/mongodb" MONGO_LOG_PATH="$MONGO_DB_PATH/log" MONGO_DB_KEY="$MONGO_DB_PATH/key" @@ -213,7 +213,7 @@ init_mongodb() { } init_replica_set() { - echo "Checking initialized database" + tlog "Checking initialized database" shouldPerformInitdb=1 for path in \ "$MONGO_DB_PATH/WiredTiger" \ @@ -227,21 +227,21 @@ init_replica_set() { done if [[ $isUriLocal -gt 0 && -f /proc/cpuinfo ]] && ! grep --quiet avx /proc/cpuinfo; then - echo "====================================================================================================" >&2 - echo "==" >&2 - echo "== AVX instruction not found in your CPU. Appsmith's embedded MongoDB may not start. Please use an external MongoDB instance instead." >&2 - echo "== See https://docs.appsmith.com/getting-started/setup/instance-configuration/custom-mongodb-redis#custom-mongodb for instructions." >&2 - echo "==" >&2 - echo "====================================================================================================" >&2 + tlog "====================================================================================================" >&2 + tlog "==" >&2 + tlog "== AVX instruction not found in your CPU. Appsmith's embedded MongoDB may not start. Please use an external MongoDB instance instead." >&2 + tlog "== See https://docs.appsmith.com/getting-started/setup/instance-configuration/custom-mongodb-redis#custom-mongodb for instructions." >&2 + tlog "==" >&2 + tlog "====================================================================================================" >&2 fi if [[ $shouldPerformInitdb -gt 0 && $isUriLocal -eq 0 ]]; then - echo "Initializing Replica Set for local database" + tlog "Initializing Replica Set for local database" # Start installed MongoDB service - Dependencies Layer mongod --fork --port 27017 --dbpath "$MONGO_DB_PATH" --logpath "$MONGO_LOG_PATH" - echo "Waiting 10s for MongoDB to start" + tlog "Waiting 10s for MongoDB to start" sleep 10 - echo "Creating MongoDB user" + tlog "Creating MongoDB user" mongosh "127.0.0.1/appsmith" --eval "db.createUser({ user: '$APPSMITH_MONGODB_USER', pwd: '$APPSMITH_MONGODB_PASSWORD', @@ -251,20 +251,20 @@ init_replica_set() { }, 'readWrite'] } )" - echo "Enabling Replica Set" + tlog "Enabling Replica Set" mongod --dbpath "$MONGO_DB_PATH" --shutdown || true mongod --fork --port 27017 --dbpath "$MONGO_DB_PATH" --logpath "$MONGO_LOG_PATH" --replSet mr1 --keyFile "$MONGODB_TMP_KEY_PATH" --bind_ip localhost - echo "Waiting 10s for MongoDB to start with Replica Set" + tlog "Waiting 10s for MongoDB to start with Replica Set" sleep 10 mongosh "$APPSMITH_DB_URL" --eval 'rs.initiate()' mongod --dbpath "$MONGO_DB_PATH" --shutdown || true fi if [[ $isUriLocal -gt 0 ]]; then - echo "Checking Replica Set of external MongoDB" + tlog "Checking Replica Set of external MongoDB" if appsmithctl check-replica-set; then - echo "MongoDB ReplicaSet is enabled" + tlog "MongoDB ReplicaSet is enabled" else echo -e "\033[0;31m***************************************************************************************\033[0m" echo -e "\033[0;31m* MongoDB Replica Set is not enabled *\033[0m" @@ -301,7 +301,7 @@ check_setup_custom_ca_certificates() { if is_empty_directory "$container_ca_certs_path"; then rmdir -v "$container_ca_certs_path" else - echo "The 'ca-certificates' directory inside the container is not empty. Please clear it and restart to use certs from 'stacks/ca-certs' directory." >&2 + tlog "The 'ca-certificates' directory inside the container is not empty. Please clear it and restart to use certs from 'stacks/ca-certs' directory." >&2 return fi fi @@ -325,11 +325,11 @@ setup-custom-ca-certificates() ( rm -f "$store" "$opts_file" if [[ -n "$(ls "$stacks_ca_certs_path"/*.pem 2>/dev/null)" ]]; then - echo "Looks like you have some '.pem' files in your 'ca-certs' folder. Please rename them to '.crt' to be picked up automatically.". + tlog "Looks like you have some '.pem' files in your 'ca-certs' folder. Please rename them to '.crt' to be picked up automatically.". fi if ! [[ -d "$stacks_ca_certs_path" && "$(find "$stacks_ca_certs_path" -maxdepth 1 -type f -name '*.crt' | wc -l)" -gt 0 ]]; then - echo "No custom CA certificates found." + tlog "No custom CA certificates found." return fi @@ -389,7 +389,7 @@ check_redis_compatible_page_size() { --data '{ "userId": "'"$HOSTNAME"'", "event":"RedisCompile" }' \ https://api.segment.io/v1/track \ || true - echo "Compile Redis stable with page size of $page_size" + tlog "Compile Redis stable with page size of $page_size" apt-get update apt-get install --yes build-essential curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp @@ -399,15 +399,14 @@ check_redis_compatible_page_size() { popd rm -rf /tmp/redis-stable else - echo "Redis is compatible with page size of $page_size" + tlog "Redis is compatible with page size of $page_size" fi } init_postgres() { # Initialize embedded postgres by default; set APPSMITH_ENABLE_EMBEDDED_DB to 0, to use existing cloud postgres mockdb instance if [[ ${APPSMITH_ENABLE_EMBEDDED_DB: -1} != 0 ]]; then - echo "" - echo "Checking initialized local postgres" + tlog "Checking initialized local postgres" POSTGRES_DB_PATH="$stacks_path/data/postgres/main" mkdir -p "$POSTGRES_DB_PATH" "$TMP/pg-runtime" @@ -416,9 +415,9 @@ init_postgres() { chown -R postgres:postgres "$POSTGRES_DB_PATH" "$TMP/pg-runtime" if [[ -e "$POSTGRES_DB_PATH/PG_VERSION" ]]; then - echo "Found existing Postgres, Skipping initialization" + tlog "Found existing Postgres, Skipping initialization" else - echo "Initializing local postgresql database" + tlog "Initializing local postgresql database" mkdir -p "$POSTGRES_DB_PATH" # Postgres does not allow it's server to be run with super user access, we use user postgres and the file system owner also needs to be the same user postgres @@ -510,11 +509,11 @@ check_db_uri if [[ -z "${DYNO}" ]]; then if [[ $isMongoUrl -eq 1 ]]; then # Setup MongoDB and initialize replica set - echo "Initializing MongoDB" + tlog "Initializing MongoDB" init_mongodb init_replica_set elif [[ $isPostgresUrl -eq 1 ]]; then - echo "Initializing Postgres" + tlog "Initializing Postgres" # init_postgres fi else diff --git a/deploy/docker/fs/opt/bin/tlog b/deploy/docker/fs/opt/bin/tlog new file mode 100644 index 0000000000..7aea2f3c4d --- /dev/null +++ b/deploy/docker/fs/opt/bin/tlog @@ -0,0 +1,5 @@ +#!/bin/sh +# Running this with `sh` since it's very tiny, and doesn't need the big bash. + +# Printing time in UTC. +echo "$(date -u +%FT%T.%3NZ)" "$@" From 187c543b9e023defe78ef5f0255b41e94aafb44a Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Mon, 10 Jun 2024 11:03:30 +0530 Subject: [PATCH 11/12] fix: adds tenant config to `CreateNewAppsOption.test.tsx` (#34121) ## Description Fixes failing client jest test cases by adding tenant config to mocked store of `CreateNewAppsOption.test.tsx` added by #34048 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="" ### :mag: Cypress test results > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Tests** - Updated test configuration to include a new `tenant` object with an empty `tenantConfiguration` in the default store state. --- .../src/ce/pages/Applications/CreateNewAppsOption.test.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx b/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx index d299b7ef94..97f9290e98 100644 --- a/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx +++ b/app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx @@ -11,6 +11,9 @@ import { unitTestBaseMockStore } from "layoutSystems/common/dropTarget/unitTestU const defaultStoreState = { ...unitTestBaseMockStore, + tenant: { + tenantConfiguration: {}, + }, entities: { ...unitTestBaseMockStore.entities, plugins: { From bf92a52f5a59f3350a915e652270763f68270fa1 Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Mon, 10 Jun 2024 12:35:23 +0530 Subject: [PATCH 12/12] chore: autocommit feature branch (#34062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description The changes are related to the "auto-commit" migration feature. **Server changes** - Fixed issue with concurrent git calls for the same repo - Trigger issues with auto-commit **UI changes** - Introduced new REST api for triggering auto-commit via client - Updated logic for saga to use trigger auto-commit api for checking whether to poll for auto-commit progress - Updated logic to make status api call blocking - Response structure change for auto-commit apis Fixes #30110 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 129eaee76d3810e5e0ea9b6391048ff9338c9c8a > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **New Features** - Introduced enhanced Git autocommit functionality with improved error handling and progress tracking. - Added new autocommit actions and state management for better synchronization. - **Bug Fixes** - Improved error handling during Git synchronization processes. - **Refactor** - Updated method names and parameters for clarity and consistency in Git synchronization. - **Chores** - Renamed feature flags and constants related to Git autocommit for better readability and maintainability. --------- Co-authored-by: brayn003 --- app/client/src/actions/gitSyncActions.ts | 58 +- app/client/src/api/GitSyncAPI.tsx | 27 + .../src/ce/constants/ReduxActionConstants.tsx | 5 +- .../src/entities/Engine/AppEditorEngine.ts | 6 +- .../gitSync/QuickGitActions/BranchButton.tsx | 11 + .../src/reducers/uiReducers/gitSyncReducer.ts | 14 + app/client/src/sagas/GitSyncSagas.ts | 160 ++-- app/client/src/selectors/gitSyncSelectors.tsx | 3 + .../appsmith/git/files/FileUtilsCEImpl.java | 68 +- .../operations/FileOperationsCEv2Impl.java | 14 +- .../external/enums/FeatureFlagEnum.java | 12 +- .../appsmith/external/git/FileInterface.java | 11 +- .../git/constants/ce/GitConstantsCE.java | 4 + .../server/controllers/GitController.java | 7 +- .../controllers/ce/GitControllerCE.java | 16 +- .../server/domains/ModuleInstance.java | 0 .../com/appsmith/server/domains/Theme.java | 6 +- .../server/dtos/AutoCommitProgressDTO.java | 17 - .../server/dtos/AutoCommitResponseDTO.java | 60 ++ .../AutoCommitEventHandler.java | 2 +- .../AutoCommitEventHandlerCE.java | 2 +- .../AutoCommitEventHandlerCEImpl.java | 3 +- .../AutoCommitEventHandlerImpl.java | 3 +- .../git/autocommit/AutoCommitService.java | 3 + .../git/autocommit/AutoCommitServiceCE.java | 9 + .../autocommit/AutoCommitServiceCEImpl.java | 147 ++++ .../git/autocommit/AutoCommitServiceImpl.java | 29 + .../helpers/AutoCommitEligibilityHelper.java | 2 + ...toCommitEligibilityHelperFallbackImpl.java | 6 + .../AutoCommitEligibilityHelperImpl.java | 104 ++- .../helpers/GitAutoCommitHelper.java | 6 +- .../GitAutoCommitHelperFallbackImpl.java | 8 +- .../helpers/GitAutoCommitHelperImpl.java | 37 +- .../server/git/common/CommonGitServiceCE.java | 5 +- .../git/common/CommonGitServiceCEImpl.java | 7 +- .../server/helpers/CommonGitFileUtils.java | 12 +- .../com/appsmith/server/helpers/GitUtils.java | 12 + .../helpers/ce/CommonGitFileUtilsCE.java | 97 ++- .../migrations/JsonSchemaMigration.java | 4 +- .../server/migrations/JsonSchemaVersions.java | 2 +- .../newpages/base/NewPageServiceCE.java | 3 + .../newpages/base/NewPageServiceCEImpl.java | 19 + .../services/ApplicationPageServiceImpl.java | 2 - .../ce/ApplicationPageServiceCEImpl.java | 76 +- .../ServerSchemaMigrationEnforcerTest.java | 182 ++++- .../ApplicationPageServiceAutoCommitTest.java | 394 ---------- .../AutoCommitEventHandlerImplTest.java | 10 +- .../git/autocommit/AutoCommitServiceTest.java | 686 ++++++++++++++++++ .../AutoCommitEligibilityHelperTest.java | 79 +- .../helpers/GitAutoCommitHelperImplTest.java | 29 +- 50 files changed, 1768 insertions(+), 711 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ModuleInstance.java delete mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitProgressDTO.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitResponseDTO.java rename app/server/appsmith-server/src/main/java/com/appsmith/server/git/{ => autocommit}/AutoCommitEventHandler.java (63%) rename app/server/appsmith-server/src/main/java/com/appsmith/server/git/{ => autocommit}/AutoCommitEventHandlerCE.java (89%) rename app/server/appsmith-server/src/main/java/com/appsmith/server/git/{ => autocommit}/AutoCommitEventHandlerCEImpl.java (99%) rename app/server/appsmith-server/src/main/java/com/appsmith/server/git/{ => autocommit}/AutoCommitEventHandlerImpl.java (93%) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitService.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCE.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceImpl.java create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java diff --git a/app/client/src/actions/gitSyncActions.ts b/app/client/src/actions/gitSyncActions.ts index 29032f6435..fdda241f5a 100644 --- a/app/client/src/actions/gitSyncActions.ts +++ b/app/client/src/actions/gitSyncActions.ts @@ -6,7 +6,10 @@ import { ReduxActionErrorTypes, ReduxActionTypes, } from "@appsmith/constants/ReduxActionConstants"; -import type { ConnectToGitPayload } from "api/GitSyncAPI"; +import type { + ConnectToGitPayload, + GitAutocommitProgressResponse, +} from "api/GitSyncAPI"; import type { GitConfig, GitSyncModalTab, MergeStatus } from "entities/GitSync"; import type { GitApplicationMetadata } from "@appsmith/api/ApplicationApi"; import { @@ -480,6 +483,7 @@ export const setShowBranchPopupAction = (show: boolean) => { }; }; +// START autocommit export const toggleAutocommitEnabledInit = () => ({ type: ReduxActionTypes.GIT_TOGGLE_AUTOCOMMIT_ENABLED_INIT, }); @@ -489,14 +493,60 @@ export const setIsAutocommitModalOpen = (isAutocommitModalOpen: boolean) => ({ payload: { isAutocommitModalOpen }, }); -export const startAutocommitProgressPolling = () => ({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_INITIATE_PROGRESS_POLLING, +export const triggerAutocommitInitAction = () => ({ + type: ReduxActionTypes.GIT_AUTOCOMMIT_TRIGGER_INIT, }); -export const stopAutocommitProgressPolling = () => ({ +export const triggerAutocommitSuccessAction = () => ({ + type: ReduxActionTypes.GIT_AUTOCOMMIT_TRIGGER_SUCCESS, +}); + +export interface TriggerAutocommitErrorActionPayload { + error: any; + show: boolean; +} + +export const triggerAutocommitErrorAction = ( + payload: TriggerAutocommitErrorActionPayload, +) => ({ + type: ReduxActionErrorTypes.GIT_AUTOCOMMIT_TRIGGER_ERROR, + payload, +}); + +export const startAutocommitProgressPollingAction = () => ({ + type: ReduxActionTypes.GIT_AUTOCOMMIT_START_PROGRESS_POLLING, +}); + +export const stopAutocommitProgressPollingAction = () => ({ type: ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING, }); +export type SetAutocommitActionPayload = GitAutocommitProgressResponse; + +export const setAutocommitProgressAction = ( + payload: SetAutocommitActionPayload, +) => ({ + type: ReduxActionTypes.GIT_SET_AUTOCOMMIT_PROGRESS, + payload, +}); + +export const resetAutocommitProgressAction = () => ({ + type: ReduxActionTypes.GIT_RESET_AUTOCOMMIT_PROGRESS, +}); + +export interface AutocommitProgressErrorActionPayload { + error: any; + show: boolean; +} + +export const autoCommitProgressErrorAction = ( + payload: AutocommitProgressErrorActionPayload, +) => ({ + type: ReduxActionErrorTypes.GIT_AUTOCOMMIT_PROGRESS_POLLING_ERROR, + payload, +}); +// END autocommit + export const getGitMetadataInitAction = () => ({ type: ReduxActionTypes.GIT_GET_METADATA_INIT, }); diff --git a/app/client/src/api/GitSyncAPI.tsx b/app/client/src/api/GitSyncAPI.tsx index c3c0150b86..22509b3041 100644 --- a/app/client/src/api/GitSyncAPI.tsx +++ b/app/client/src/api/GitSyncAPI.tsx @@ -43,6 +43,27 @@ interface GitRemoteStatusParam { branch: string; } +export enum AutocommitResponseEnum { + IN_PROGRESS = "IN_PROGRESS", + LOCKED = "LOCKED", + PUBLISHED = "PUBLISHED", + IDLE = "IDLE", + NOT_REQUIRED = "NOT_REQUIRED", + NON_GIT_APP = "NON_GIT_APP", +} + +export interface GitAutocommitProgressResponse { + autoCommitResponse: AutocommitResponseEnum; + progress: number; + branchName: string; +} + +export interface GitTriggerAutocommitResponse { + autoCommitResponse: AutocommitResponseEnum; + progress: number; + branchName: string; +} + class GitSyncAPI extends Api { static baseURL = `/v1/git`; @@ -227,6 +248,12 @@ class GitSyncAPI extends Api { ); } + static async triggerAutocommit(applicationId: string, branchName: string) { + return Api.post( + `${GitSyncAPI.baseURL}/auto-commit/app/${applicationId}?branchName=${branchName}`, + ); + } + static async getAutocommitProgress(applicationId: string) { return Api.get( `${GitSyncAPI.baseURL}/auto-commit/progress/app/${applicationId}`, diff --git a/app/client/src/ce/constants/ReduxActionConstants.tsx b/app/client/src/ce/constants/ReduxActionConstants.tsx index 558200332e..8d49cfbc7b 100644 --- a/app/client/src/ce/constants/ReduxActionConstants.tsx +++ b/app/client/src/ce/constants/ReduxActionConstants.tsx @@ -131,8 +131,8 @@ const ActionTypes = { GIT_TOGGLE_AUTOCOMMIT_ENABLED_INIT: "GIT_TOGGLE_AUTOCOMMIT_ENABLED_INIT", GIT_TOGGLE_AUTOCOMMIT_ENABLED_SUCCESS: "GIT_TOGGLE_AUTOCOMMIT_ENABLED_SUCCESS", - GIT_AUTOCOMMIT_INITIATE_PROGRESS_POLLING: - "GIT_AUTOCOMMIT_INITIATE_PROGRESS_POLLING", + GIT_AUTOCOMMIT_TRIGGER_INIT: "GIT_AUTOCOMMIT_TRIGGER_INIT", + GIT_AUTOCOMMIT_TRIGGER_SUCCESS: "GIT_AUTOCOMMIT_TRIGGER_SUCCESS", GIT_AUTOCOMMIT_START_PROGRESS_POLLING: "GIT_AUTOCOMMIT_START_PROGRESS_POLLING", GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING: "GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING", @@ -962,6 +962,7 @@ export const ReduxActionErrorTypes = { GIT_TOGGLE_AUTOCOMMIT_ENABLED_ERROR: "GIT_TOGGLE_AUTOCOMMIT_ENABLED_ERROR", GIT_AUTOCOMMIT_PROGRESS_POLLING_ERROR: "GIT_AUTOCOMMIT_PROGRESS_POLLING_ERROR", + GIT_AUTOCOMMIT_TRIGGER_ERROR: "GIT_AUTOCOMMIT_TRIGGER_ERROR", GIT_GET_METADATA_ERROR: "GIT_GET_METADATA_ERROR", FETCH_FEATURE_FLAGS_ERROR: "FETCH_FEATURE_FLAGS_ERROR", FETCH_NOTIFICATIONS_ERROR: "FETCH_NOTIFICATIONS_ERROR", diff --git a/app/client/src/entities/Engine/AppEditorEngine.ts b/app/client/src/entities/Engine/AppEditorEngine.ts index 092f856a2c..407c72fa0b 100644 --- a/app/client/src/entities/Engine/AppEditorEngine.ts +++ b/app/client/src/entities/Engine/AppEditorEngine.ts @@ -5,7 +5,7 @@ import { remoteUrlInputValue, resetPullMergeStatus, fetchBranchesInit, - startAutocommitProgressPolling, + triggerAutocommitInitAction, getGitMetadataInitAction, } from "actions/gitSyncActions"; import { restoreRecentEntitiesRequest } from "actions/globalSearchActions"; @@ -296,10 +296,8 @@ export default class AppEditorEngine extends AppEngine { yield put(fetchGitProtectedBranchesInit()); yield put(fetchGitProtectedBranchesInit()); yield put(getGitMetadataInitAction()); - + yield put(triggerAutocommitInitAction()); yield put(fetchGitStatusInit({ compareRemote: true })); - - yield put(startAutocommitProgressPolling()); yield put(resetPullMergeStatus()); } } diff --git a/app/client/src/pages/Editor/gitSync/QuickGitActions/BranchButton.tsx b/app/client/src/pages/Editor/gitSync/QuickGitActions/BranchButton.tsx index 16a9fd459e..0f11070f48 100644 --- a/app/client/src/pages/Editor/gitSync/QuickGitActions/BranchButton.tsx +++ b/app/client/src/pages/Editor/gitSync/QuickGitActions/BranchButton.tsx @@ -9,6 +9,8 @@ import { getCurrentAppGitMetaData } from "@appsmith/selectors/applicationSelecto import BranchList from "../components/BranchList"; import { getGitStatus, + getIsPollingAutocommit, + getIsTriggeringAutocommit, protectedModeSelector, showBranchPopupSelector, } from "selectors/gitSyncSelectors"; @@ -28,6 +30,10 @@ const ButtonContainer = styled(Button)` margin: 0 ${(props) => props.theme.spaces[4]}px; max-width: 122px; min-width: unset !important; + + :active { + border: 1px solid var(--ads-v2-color-border-muted); + } `; function BranchButton() { @@ -38,6 +44,9 @@ function BranchButton() { const labelTarget = useRef(null); const status = useSelector(getGitStatus); const isOpen = useSelector(showBranchPopupSelector); + const triggeringAutocommit = useSelector(getIsTriggeringAutocommit); + const pollingAutocommit = useSelector(getIsPollingAutocommit); + const isBranchChangeDisabled = triggeringAutocommit || pollingAutocommit; const setIsOpen = (isOpen: boolean) => { dispatch(setShowBranchPopupAction(isOpen)); @@ -47,6 +56,7 @@ function BranchButton() { } data-testid={"t--git-branch-button-popover"} + disabled={isBranchChangeDisabled} hasBackdrop isOpen={isOpen} minimal @@ -69,6 +79,7 @@ function BranchButton() { {isProtectedMode ? ( diff --git a/app/client/src/reducers/uiReducers/gitSyncReducer.ts b/app/client/src/reducers/uiReducers/gitSyncReducer.ts index 454a080061..8112865971 100644 --- a/app/client/src/reducers/uiReducers/gitSyncReducer.ts +++ b/app/client/src/reducers/uiReducers/gitSyncReducer.ts @@ -55,6 +55,7 @@ const initialState: GitSyncReducerState = { isAutocommitModalOpen: false, togglingAutocommit: false, + triggeringAutocommit: false, pollingAutocommitStatus: false, gitMetadata: null, @@ -619,6 +620,18 @@ const gitSyncReducer = createReducer(initialState, { ...state, togglingAutocommit: false, }), + [ReduxActionTypes.GIT_AUTOCOMMIT_TRIGGER_INIT]: (state) => ({ + ...state, + triggeringAutocommit: true, + }), + [ReduxActionTypes.GIT_AUTOCOMMIT_TRIGGER_SUCCESS]: (state) => ({ + ...state, + triggeringAutocommit: false, + }), + [ReduxActionErrorTypes.GIT_AUTOCOMMIT_TRIGGER_ERROR]: (state) => ({ + ...state, + triggeringAutocommit: false, + }), [ReduxActionTypes.GIT_AUTOCOMMIT_START_PROGRESS_POLLING]: (state) => ({ ...state, pollingAutocommitStatus: true, @@ -833,6 +846,7 @@ export type GitSyncReducerState = GitBranchDeleteState & { isAutocommitModalOpen: boolean; togglingAutocommit: boolean; + triggeringAutocommit: boolean; pollingAutocommitStatus: boolean; gitMetadata: GitMetadata | null; diff --git a/app/client/src/sagas/GitSyncSagas.ts b/app/client/src/sagas/GitSyncSagas.ts index 095689e541..75e2309b2b 100644 --- a/app/client/src/sagas/GitSyncSagas.ts +++ b/app/client/src/sagas/GitSyncSagas.ts @@ -20,8 +20,13 @@ import { takeLatest, } from "redux-saga/effects"; import type { TakeableChannel } from "@redux-saga/core"; -import type { MergeBranchPayload, MergeStatusPayload } from "api/GitSyncAPI"; -import GitSyncAPI from "api/GitSyncAPI"; +import type { + GitAutocommitProgressResponse, + GitTriggerAutocommitResponse, + MergeBranchPayload, + MergeStatusPayload, +} from "api/GitSyncAPI"; +import GitSyncAPI, { AutocommitResponseEnum } from "api/GitSyncAPI"; import { getCurrentApplicationId, getCurrentPageId, @@ -40,6 +45,13 @@ import { updateGitProtectedBranchesInit, clearCommitSuccessfulState, setShowBranchPopupAction, + stopAutocommitProgressPollingAction, + startAutocommitProgressPollingAction, + setAutocommitProgressAction, + autoCommitProgressErrorAction, + resetAutocommitProgressAction, + triggerAutocommitErrorAction, + triggerAutocommitSuccessAction, } from "actions/gitSyncActions"; import { commitToRepoSuccess, @@ -103,6 +115,7 @@ import { addBranchParam, GIT_BRANCH_QUERY_KEY } from "constants/routes"; import { getCurrentGitBranch, getDisconnectingGitApplication, + getGitMetadataSelector, } from "selectors/gitSyncSelectors"; import { initEditor } from "actions/initActions"; import { fetchPage } from "actions/pageActions"; @@ -113,7 +126,10 @@ import { log } from "loglevel"; import GIT_ERROR_CODES from "constants/GitErrorCodes"; import { builderURL } from "@appsmith/RouteBuilder"; import { APP_MODE } from "entities/App"; -import type { GitDiscardResponse } from "reducers/uiReducers/gitSyncReducer"; +import type { + GitDiscardResponse, + GitMetadata, +} from "reducers/uiReducers/gitSyncReducer"; import { FocusEntity, identifyEntityFromPath } from "navigation/FocusEntity"; import { getActions, @@ -123,6 +139,8 @@ import type { Action } from "entities/Action"; import type { JSCollectionDataState } from "@appsmith/reducers/entityReducers/jsActionsReducer"; import { toast } from "design-system"; import { gitExtendedSagas } from "@appsmith/sagas/GitExtendedSagas"; +import { selectFeatureFlagCheck } from "@appsmith/selectors/featureFlagsSelectors"; +import { FEATURE_FLAG } from "@appsmith/entities/FeatureFlag"; export function* handleRepoLimitReachedError(response?: ApiResponse) { const { responseMeta } = response || {}; @@ -1159,78 +1177,110 @@ function* getGitMetadataSaga() { } } +function isAutocommitHappening( + response: + | GitTriggerAutocommitResponse + | GitAutocommitProgressResponse + | undefined, +): boolean { + return ( + !!response && + !!( + response.autoCommitResponse === AutocommitResponseEnum.PUBLISHED || + response.autoCommitResponse === AutocommitResponseEnum.IN_PROGRESS || + response.autoCommitResponse === AutocommitResponseEnum.LOCKED + ) + ); +} + function* pollAutocommitProgressSaga(): any { const applicationId: string = yield select(getCurrentApplicationId); + const branchName: string = yield select(getCurrentGitBranch); + + let triggerResponse: ApiResponse | undefined; try { - const response: ApiResponse = yield call( - GitSyncAPI.getAutocommitProgress, + const res = yield call( + GitSyncAPI.triggerAutocommit, applicationId, + branchName, ); const isValidResponse: boolean = yield validateResponse( - response, + res, false, - getLogToSentryFromResponse(response), + getLogToSentryFromResponse(res), ); - if (isValidResponse && response?.data?.isRunning) { - yield put({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_START_PROGRESS_POLLING, - }); + if (isValidResponse) { + triggerResponse = res; + yield put(triggerAutocommitSuccessAction()); + } else { + yield put( + triggerAutocommitErrorAction({ + error: res?.responseMeta?.error?.message, + show: true, + }), + ); + } + } catch (err) { + yield put(triggerAutocommitErrorAction({ error: err, show: false })); + } + + try { + if (isAutocommitHappening(triggerResponse?.data)) { + yield put(startAutocommitProgressPollingAction()); while (true) { - const response: ApiResponse = yield call( - GitSyncAPI.getAutocommitProgress, - applicationId, - ); + const progressResponse: ApiResponse = + yield call(GitSyncAPI.getAutocommitProgress, applicationId); const isValidResponse: boolean = yield validateResponse( - response, + progressResponse, false, - getLogToSentryFromResponse(response), + getLogToSentryFromResponse(progressResponse), ); if (isValidResponse) { - yield put({ - type: ReduxActionTypes.GIT_SET_AUTOCOMMIT_PROGRESS, - payload: response.data, - }); - if (!response?.data?.isRunning) { - yield put({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING, - }); + yield put(setAutocommitProgressAction(progressResponse.data)); + if (!isAutocommitHappening(progressResponse?.data)) { + yield put(stopAutocommitProgressPollingAction()); } } else { - yield put({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING, - }); - yield put({ - type: ReduxActionErrorTypes.GIT_AUTOCOMMIT_PROGRESS_POLLING_ERROR, - payload: { - error: response?.responseMeta?.error?.message, + yield put(stopAutocommitProgressPollingAction()); + yield put( + autoCommitProgressErrorAction({ + error: progressResponse?.responseMeta?.error?.message, show: true, - }, - }); + }), + ); } yield delay(1000); } } else { - yield put({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING, - }); + yield put(stopAutocommitProgressPollingAction()); } } catch (error) { - yield put({ - type: ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING, - }); - yield put({ - type: ReduxActionErrorTypes.GIT_AUTOCOMMIT_PROGRESS_POLLING_ERROR, - payload: { error }, - }); + yield put(stopAutocommitProgressPollingAction()); + yield put(autoCommitProgressErrorAction({ error, show: false })); } finally { if (yield cancelled()) { - yield put({ - type: ReduxActionTypes.GIT_RESET_AUTOCOMMIT_PROGRESS, - }); + yield put(resetAutocommitProgressAction()); } } } +function* triggerAutocommitSaga() { + const isAutocommitFeatureEnabled: boolean = yield select( + selectFeatureFlagCheck, + FEATURE_FLAG.release_git_autocommit_feature_enabled, + ); + const gitMetadata: GitMetadata = yield select(getGitMetadataSelector); + const isAutocommitEnabled: boolean = !!gitMetadata?.autoCommitConfig?.enabled; + if (isAutocommitFeatureEnabled && isAutocommitEnabled) { + /* @ts-expect-error: not sure how to do typings of this */ + const pollTask = yield fork(pollAutocommitProgressSaga); + yield take(ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING); + yield cancel(pollTask); + } else { + yield put(triggerAutocommitSuccessAction()); + } +} + const gitRequestBlockingActions: Record< (typeof ReduxActionTypes)[keyof typeof ReduxActionTypes], (...args: any[]) => any @@ -1252,6 +1302,9 @@ const gitRequestBlockingActions: Record< [ReduxActionTypes.GIT_DISCARD_CHANGES]: discardChanges, [ReduxActionTypes.GIT_UPDATE_PROTECTED_BRANCHES_INIT]: updateGitProtectedBranchesSaga, + [ReduxActionTypes.GIT_AUTOCOMMIT_TRIGGER_INIT]: triggerAutocommitSaga, + [ReduxActionTypes.FETCH_GIT_STATUS_INIT]: fetchGitStatusSaga, + [ReduxActionTypes.GIT_GET_METADATA_INIT]: getGitMetadataSaga, }; const gitRequestNonBlockingActions: Record< @@ -1261,13 +1314,11 @@ const gitRequestNonBlockingActions: Record< ...gitExtendedSagas, [ReduxActionTypes.FETCH_GLOBAL_GIT_CONFIG_INIT]: fetchGlobalGitConfig, [ReduxActionTypes.FETCH_LOCAL_GIT_CONFIG_INIT]: fetchLocalGitConfig, - [ReduxActionTypes.FETCH_GIT_STATUS_INIT]: fetchGitStatusSaga, [ReduxActionTypes.SHOW_CONNECT_GIT_MODAL]: showConnectGitModal, [ReduxActionTypes.FETCH_SSH_KEY_PAIR_INIT]: getSSHKeyPairSaga, [ReduxActionTypes.GIT_FETCH_PROTECTED_BRANCHES_INIT]: fetchGitProtectedBranchesSaga, [ReduxActionTypes.GIT_TOGGLE_AUTOCOMMIT_ENABLED_INIT]: toggleAutocommitSaga, - [ReduxActionTypes.GIT_GET_METADATA_INIT]: getGitMetadataSaga, }; /** @@ -1298,18 +1349,7 @@ function* watchGitNonBlockingRequests() { } } -function* watchGitAutocommitPolling() { - while (true) { - yield take(ReduxActionTypes.GIT_AUTOCOMMIT_INITIATE_PROGRESS_POLLING); - /* @ts-expect-error: not sure how to do typings of this */ - const pollTask = yield fork(pollAutocommitProgressSaga); - yield take(ReduxActionTypes.GIT_AUTOCOMMIT_STOP_PROGRESS_POLLING); - yield cancel(pollTask); - } -} - export default function* gitSyncSagas() { yield fork(watchGitNonBlockingRequests); yield fork(watchGitBlockingRequests); - yield fork(watchGitAutocommitPolling); } diff --git a/app/client/src/selectors/gitSyncSelectors.tsx b/app/client/src/selectors/gitSyncSelectors.tsx index 73fa17e8ee..9c9e5e5b69 100644 --- a/app/client/src/selectors/gitSyncSelectors.tsx +++ b/app/client/src/selectors/gitSyncSelectors.tsx @@ -257,6 +257,9 @@ export const getIsAutocommitToggling = (state: AppState) => export const getIsAutocommitModalOpen = (state: AppState) => state.ui.gitSync.isAutocommitModalOpen; +export const getIsTriggeringAutocommit = (state: AppState) => + state.ui.gitSync.triggeringAutocommit; + export const getIsPollingAutocommit = (state: AppState) => state.ui.gitSync.pollingAutocommitStatus; diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java index a058557dac..c960e50a7e 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java @@ -288,8 +288,10 @@ public class FileUtilsCEImpl implements FileInterface { // Save JS Libs if there's at least one change if (modifiedResources != null - && !CollectionUtils.isEmpty( - modifiedResources.getModifiedResourceMap().get(CUSTOM_JS_LIB_LIST))) { + && (modifiedResources.isAllModified() + || !CollectionUtils.isEmpty( + modifiedResources.getModifiedResourceMap().get(CUSTOM_JS_LIB_LIST)))) { + Path jsLibDirectory = baseRepo.resolve(JS_LIB_DIRECTORY); Set> jsLibEntries = applicationGitReference.getJsLibraries().entrySet(); @@ -953,24 +955,62 @@ public class FileUtilsCEImpl implements FileInterface { @Override public Mono reconstructMetadataFromGitRepo( - String workspaceId, String defaultArtifactId, String repoName, String branchName, Path baseRepoSuffix) { + String workspaceId, + String defaultArtifactId, + String repoName, + String branchName, + Path baseRepoSuffix, + Boolean isResetToLastCommitRequired) { Mono metadataMono; try { - // instead of checking out to last branch we are first cleaning the git repo, - // then checking out to the desired branch - metadataMono = gitExecutor - .resetToLastCommit(baseRepoSuffix, branchName) - .map(isSwitched -> { - Path baseRepoPath = - Paths.get(gitServiceConfig.getGitRootPath()).resolve(baseRepoSuffix); - Object metadata = fileOperations.readFile( - baseRepoPath.resolve(CommonConstants.METADATA + CommonConstants.JSON_EXTENSION)); - return metadata; - }); + Mono gitResetMono = Mono.just(Boolean.TRUE); + if (Boolean.TRUE.equals(isResetToLastCommitRequired)) { + // instead of checking out to last branch we are first cleaning the git repo, + // then checking out to the desired branch + gitResetMono = gitExecutor.resetToLastCommit(baseRepoSuffix, branchName); + } + + metadataMono = gitResetMono.map(isSwitched -> { + Path baseRepoPath = Paths.get(gitServiceConfig.getGitRootPath()).resolve(baseRepoSuffix); + Object metadata = fileOperations.readFile( + baseRepoPath.resolve(CommonConstants.METADATA + CommonConstants.JSON_EXTENSION)); + return metadata; + }); } catch (GitAPIException | IOException exception) { metadataMono = Mono.error(exception); } return metadataMono.subscribeOn(scheduler); } + + @Override + public Mono reconstructPageFromGitRepo( + String pageName, String branchName, Path baseRepoSuffixPath, Boolean resetToLastCommitRequired) { + Mono pageObjectMono; + try { + Mono resetToLastCommit = Mono.just(Boolean.TRUE); + + if (Boolean.TRUE.equals(resetToLastCommitRequired)) { + // instead of checking out to last branch we are first cleaning the git repo, + // then checking out to the desired branch + resetToLastCommit = gitExecutor.resetToLastCommit(baseRepoSuffixPath, branchName); + } + + pageObjectMono = resetToLastCommit.map(isSwitched -> { + Path pageSuffix = Paths.get(PAGE_DIRECTORY, pageName); + Path repoPath = Paths.get(gitServiceConfig.getGitRootPath()) + .resolve(baseRepoSuffixPath) + .resolve(pageSuffix); + + Object pageObject = + fileOperations.readFile(repoPath.resolve(pageName + CommonConstants.JSON_EXTENSION)); + + return pageObject; + }); + } catch (GitAPIException | IOException exception) { + pageObjectMono = Mono.error(exception); + } + + return pageObjectMono.subscribeOn(scheduler); + } } diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java index dc07005a78..2d233af3b9 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java @@ -70,7 +70,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File this.observationHelper = observationHelper; } - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public void saveMetadataResource(ApplicationGitReference applicationGitReference, Path baseRepo) { ObjectNode metadata = objectMapper.valueToTree(applicationGitReference.getMetadata()); @@ -78,7 +78,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File saveResource(metadata, baseRepo.resolve(CommonConstants.METADATA + CommonConstants.JSON_EXTENSION)); } - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public void saveWidgets(JSONObject sourceEntity, String resourceName, Path path) { Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); @@ -98,7 +98,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File } } - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public boolean writeToFile(Object sourceEntity, Path path) throws IOException { Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); @@ -123,7 +123,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File * @param filePath file on which the read operation will be performed * @return resource stored in the JSON file */ - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public Object readFile(Path filePath) { Span span = observationHelper.createSpan(GitSpan.FILE_READ); @@ -147,7 +147,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File * @param directoryPath directory path for files on which read operation will be performed * @return resources stored in the directory */ - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public Map readFiles(Path directoryPath, String keySuffix) { Map resource = new HashMap<>(); @@ -168,7 +168,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File return resource; } - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public Integer getFileFormatVersion(Object metadata) { if (metadata == null) { @@ -179,7 +179,7 @@ public class FileOperationsCEv2Impl extends FileOperationsCEImpl implements File return fileFormatVersion; } - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_cleanup_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) @Override public JSONObject getMainContainer(Object pageJson) { JsonNode pageJSON = objectMapper.valueToTree(pageJson); diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java index beef82f43d..719a243350 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java @@ -15,7 +15,15 @@ public enum FeatureFlagEnum { release_embed_hide_share_settings_enabled, rollout_datasource_test_rate_limit_enabled, release_git_autocommit_feature_enabled, - release_git_cleanup_feature_enabled, - release_git_server_autocommit_feature_enabled, + /** + * Since checking eligibility for autocommit is an expensive operation, + * We want to roll out this feature on cloud in a controlled manner. + * We could have used the autocommit flag itself, however it is on tenant level, + * and it can't be moved to user level due to its usage on non-reactive code paths. + * We would keep the main autocommit flag false on production for the version <= testing versions, + * and turn it to true for later versions + * We would remove this feature flag once the testing is complete. + */ + release_git_autocommit_eligibility_enabled, // Add EE flags below this line, to avoid conflicts. } diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java index 6502ac4014..332b833a44 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java @@ -54,10 +54,19 @@ public interface FileInterface { * @param repoName * @param branchName * @param repoSuffixPath + * @param isResetToLastCommitRequired * @return */ Mono reconstructMetadataFromGitRepo( - String workspaceId, String defaultApplicationId, String repoName, String branchName, Path repoSuffixPath); + String workspaceId, + String defaultApplicationId, + String repoName, + String branchName, + Path repoSuffixPath, + Boolean isResetToLastCommitRequired); + + Mono reconstructPageFromGitRepo( + String pageName, String branchName, Path repoSuffixPath, Boolean checkoutRequired); /** * Once the user connects the existing application to a remote repo, we will initialize the repo with Readme.md - diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java index 8fd9d3d881..e77db0e04f 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java @@ -22,6 +22,8 @@ public class GitConstantsCE { public static final String GIT_PROFILE_ERROR = "Unable to find git author configuration for logged-in user. You can" + " set up a git profile from the user profile section."; + public static final String RECONSTRUCT_PAGE = "reconstruct page"; + public class GitMetricConstantsCE { public static final String CHECKOUT_REMOTE = "checkout-remote"; public static final String HARD_RESET = "hard-reset"; @@ -47,5 +49,7 @@ public class GitConstantsCE { public static final String MERGE_BRANCH = "mergeBranch"; public static final String DELETE = "delete"; public static final String DISCARD = "discard"; + public static final String PAGE_DSL_VERSION = "pageDslVersion"; + public static final String AUTO_COMMIT_ELIGIBILITY = "autoCommitEligibility"; } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/GitController.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/GitController.java index a8278337c7..04f7ef695e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/GitController.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/GitController.java @@ -2,8 +2,10 @@ package com.appsmith.server.controllers; import com.appsmith.server.constants.Url; import com.appsmith.server.controllers.ce.GitControllerCE; +import com.appsmith.server.git.autocommit.AutoCommitService; import com.appsmith.server.git.common.CommonGitService; import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; @@ -12,7 +14,8 @@ import org.springframework.web.bind.annotation.RestController; @RequestMapping(Url.GIT_URL) public class GitController extends GitControllerCE { - public GitController(CommonGitService service) { - super(service); + @Autowired + public GitController(CommonGitService service, AutoCommitService autoCommitService) { + super(service, autoCommitService); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java index e45695d9d0..c058097cc2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java @@ -13,7 +13,7 @@ import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.GitAuth; import com.appsmith.server.domains.GitProfile; import com.appsmith.server.dtos.ApplicationImportDTO; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.BranchProtectionRequestDTO; import com.appsmith.server.dtos.GitCommitDTO; import com.appsmith.server.dtos.GitConnectDTO; @@ -22,6 +22,7 @@ import com.appsmith.server.dtos.GitDocsDTO; import com.appsmith.server.dtos.GitMergeDTO; import com.appsmith.server.dtos.GitPullDTO; import com.appsmith.server.dtos.ResponseDTO; +import com.appsmith.server.git.autocommit.AutoCommitService; import com.appsmith.server.git.common.CommonGitService; import com.appsmith.server.helpers.GitDeployKeyGenerator; import com.fasterxml.jackson.annotation.JsonView; @@ -53,6 +54,7 @@ import java.util.Map; public class GitControllerCE { private final CommonGitService service; + private final AutoCommitService autoCommitService; /** * applicationId is the defaultApplicationId @@ -333,16 +335,18 @@ public class GitControllerCE { @JsonView(Views.Public.class) @PostMapping("/auto-commit/app/{defaultApplicationId}") - public Mono> autoCommit( - @PathVariable String defaultApplicationId, @RequestParam String branchName) { - return service.autoCommitApplication(defaultApplicationId, branchName, ArtifactType.APPLICATION) + public Mono> autoCommitApplication( + @PathVariable String defaultApplicationId, @RequestHeader(name = FieldName.BRANCH_NAME) String branchName) { + return autoCommitService + .autoCommitApplication(defaultApplicationId, branchName) .map(data -> new ResponseDTO<>(HttpStatus.OK.value(), data, null)); } @JsonView(Views.Public.class) @GetMapping("/auto-commit/progress/app/{defaultApplicationId}") - public Mono> getAutoCommitProgress(@PathVariable String defaultApplicationId) { - return service.getAutoCommitProgress(defaultApplicationId, ArtifactType.APPLICATION) + public Mono> getAutoCommitProgress( + @PathVariable String defaultApplicationId, @RequestHeader(name = FieldName.BRANCH_NAME) String branchName) { + return service.getAutoCommitProgress(defaultApplicationId, branchName, ArtifactType.APPLICATION) .map(data -> new ResponseDTO<>(HttpStatus.OK.value(), data, null)); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ModuleInstance.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ModuleInstance.java new file mode 100644 index 0000000000..e69de29bb2 diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java index 5e38193e0d..3c82a0e2b4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java @@ -38,13 +38,13 @@ public class Theme extends BaseDomain { @JsonView(Views.Public.class) String workspaceId; - @JsonView(Views.Public.class) + @JsonView({Views.Public.class, Git.class}) private Object config; - @JsonView(Views.Public.class) + @JsonView({Views.Public.class, Git.class}) private Object properties; - @JsonView(Views.Public.class) + @JsonView({Views.Public.class, Git.class}) private Map stylesheet; @JsonProperty("isSystemTheme") // manually setting property name to make sure it's compatible with Gson diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitProgressDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitProgressDTO.java deleted file mode 100644 index c37bc7cb77..0000000000 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitProgressDTO.java +++ /dev/null @@ -1,17 +0,0 @@ -package com.appsmith.server.dtos; - -import lombok.Data; -import lombok.NoArgsConstructor; -import lombok.NonNull; -import lombok.RequiredArgsConstructor; - -@Data -@RequiredArgsConstructor -@NoArgsConstructor -public class AutoCommitProgressDTO { - @NonNull private Boolean isRunning; - - // using primitive type int instead of Integer because we want to 0 as default value. Integer have default null - private int progress; - private String branchName; -} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitResponseDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitResponseDTO.java new file mode 100644 index 0000000000..7e241026b9 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AutoCommitResponseDTO.java @@ -0,0 +1,60 @@ +package com.appsmith.server.dtos; + +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.RequiredArgsConstructor; + +@Data +@AllArgsConstructor +@RequiredArgsConstructor +public class AutoCommitResponseDTO { + + public enum AutoCommitResponse { + /** + * This enum is used when an autocommit is in progress for a different branch from the branch on which + * the autocommit is requested. + */ + LOCKED, + + /** + * This enum is used when an autocommit has been published. + */ + PUBLISHED, + /** + * This enum is used when an autocommit is in progress for the branch from which + * the autocommit is requested. + */ + IN_PROGRESS, + + /** + * This enum is used when an autocommit has been requested however it did not fulfil the pre-requisite. + */ + REQUIRED, + + /** + * This enum is used when an autocommit is requested however it's not required. + */ + IDLE, + + /** + * This enum is used when the app on which the autocommit is requested is a non git app. + */ + NON_GIT_APP + } + + /** + * Enum to denote the current state of autocommit + */ + private AutoCommitResponse autoCommitResponse; + + /** + * progress of the already-running auto-commit. + */ + private int progress; + + private String branchName; + + public AutoCommitResponseDTO(AutoCommitResponse autoCommitResponse) { + this.autoCommitResponse = autoCommitResponse; + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandler.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandler.java similarity index 63% rename from app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandler.java rename to app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandler.java index 88e294a214..11385a9c4d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandler.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandler.java @@ -1,3 +1,3 @@ -package com.appsmith.server.git; +package com.appsmith.server.git.autocommit; public interface AutoCommitEventHandler extends AutoCommitEventHandlerCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCE.java similarity index 89% rename from app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCE.java rename to app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCE.java index 177233f571..8255d95781 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCE.java @@ -1,4 +1,4 @@ -package com.appsmith.server.git; +package com.appsmith.server.git.autocommit; import com.appsmith.server.events.AutoCommitEvent; import reactor.core.publisher.Mono; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCEImpl.java similarity index 99% rename from app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCEImpl.java rename to app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCEImpl.java index 96f8b5c3cc..8e9313b90c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCEImpl.java @@ -1,4 +1,4 @@ -package com.appsmith.server.git; +package com.appsmith.server.git.autocommit; import com.appsmith.external.constants.AnalyticsEvents; import com.appsmith.external.dtos.ModifiedResources; @@ -13,6 +13,7 @@ import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.events.AutoCommitEvent; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImpl.java similarity index 93% rename from app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerImpl.java rename to app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImpl.java index 45f804c4f5..5856e0bf50 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImpl.java @@ -1,7 +1,8 @@ -package com.appsmith.server.git; +package com.appsmith.server.git.autocommit; import com.appsmith.external.git.GitExecutor; import com.appsmith.server.configurations.ProjectProperties; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; import com.appsmith.server.helpers.RedisUtils; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitService.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitService.java new file mode 100644 index 0000000000..6048036cd6 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitService.java @@ -0,0 +1,3 @@ +package com.appsmith.server.git.autocommit; + +public interface AutoCommitService extends AutoCommitServiceCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCE.java new file mode 100644 index 0000000000..b65b0a8307 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCE.java @@ -0,0 +1,9 @@ +package com.appsmith.server.git.autocommit; + +import com.appsmith.server.dtos.AutoCommitResponseDTO; +import reactor.core.publisher.Mono; + +public interface AutoCommitServiceCE { + + Mono autoCommitApplication(String defaultApplicationId, String branchName); +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java new file mode 100644 index 0000000000..829dba7ed0 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceCEImpl.java @@ -0,0 +1,147 @@ +package com.appsmith.server.git.autocommit; + +import com.appsmith.server.applications.base.ApplicationService; +import com.appsmith.server.constants.FieldName; +import com.appsmith.server.domains.Application; +import com.appsmith.server.domains.ApplicationMode; +import com.appsmith.server.domains.GitArtifactMetadata; +import com.appsmith.server.dtos.AutoCommitResponseDTO; +import com.appsmith.server.dtos.AutoCommitTriggerDTO; +import com.appsmith.server.dtos.PageDTO; +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.git.autocommit.helpers.AutoCommitEligibilityHelper; +import com.appsmith.server.git.autocommit.helpers.GitAutoCommitHelperImpl; +import com.appsmith.server.newpages.base.NewPageService; +import com.appsmith.server.solutions.ApplicationPermission; +import com.appsmith.server.solutions.PagePermission; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import reactor.core.publisher.Mono; + +import java.util.Set; + +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IDLE; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IN_PROGRESS; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.LOCKED; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.NON_GIT_APP; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.REQUIRED; +import static java.lang.Boolean.TRUE; +import static org.springframework.util.StringUtils.hasText; + +@Slf4j +@RequiredArgsConstructor +public class AutoCommitServiceCEImpl implements AutoCommitServiceCE { + + private final ApplicationService applicationService; + private final ApplicationPermission applicationPermission; + + private final NewPageService newPageService; + private final PagePermission pagePermission; + + private final AutoCommitEligibilityHelper autoCommitEligibilityHelper; + private final GitAutoCommitHelperImpl gitAutoCommitHelper; + + @Override + public Mono autoCommitApplication(String defaultApplicationId, String branchName) { + + if (!hasText(defaultApplicationId)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.APPLICATION_ID)); + } + + if (!hasText(branchName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); + } + + Mono applicationMonoCached = applicationService + .findByBranchNameAndDefaultApplicationId( + branchName, defaultApplicationId, applicationPermission.getEditPermission()) + .switchIfEmpty(Mono.error(new AppsmithException( + AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION, defaultApplicationId))) + .cache(); + + // A page-dto which must exist in the git file system is required, + // this existence can be guaranteed by using application mode = published + // in appsmith git system an application could be only published if it's commited, converse is also true, + // hence a published page would definitely be present in git fs. + Mono pageDTOMono = applicationMonoCached + .flatMap(application -> newPageService + .findByApplicationIdAndApplicationMode( + application.getId(), pagePermission.getEditPermission(), ApplicationMode.PUBLISHED) + .next()) + .switchIfEmpty(Mono.error( + new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, defaultApplicationId))); + + return applicationMonoCached.zipWith(pageDTOMono).flatMap(tuple2 -> { + Application branchedApplication = tuple2.getT1(); + PageDTO pageDTO = tuple2.getT2(); + + AutoCommitResponseDTO autoCommitResponseDTO = new AutoCommitResponseDTO(); + + if (branchedApplication.getGitApplicationMetadata() == null) { + autoCommitResponseDTO.setAutoCommitResponse(NON_GIT_APP); + return Mono.just(autoCommitResponseDTO); + } + + String workspaceId = branchedApplication.getWorkspaceId(); + GitArtifactMetadata gitArtifactMetadata = branchedApplication.getGitArtifactMetadata(); + + Mono isAutoCommitRequiredMono = + autoCommitEligibilityHelper.isAutoCommitRequired(workspaceId, gitArtifactMetadata, pageDTO); + + Mono autoCommitProgressDTOMono = + gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId, branchName); + + return autoCommitProgressDTOMono.flatMap(autoCommitProgressDTO -> { + if (Set.of(LOCKED, IN_PROGRESS).contains(autoCommitProgressDTO.getAutoCommitResponse())) { + log.info( + "application with id: {}, has requested auto-commit for branch name: {}, however an event for branch name: {} is already in progress", + defaultApplicationId, + branchName, + autoCommitProgressDTO.getBranchName()); + autoCommitResponseDTO.setAutoCommitResponse(autoCommitProgressDTO.getAutoCommitResponse()); + autoCommitResponseDTO.setProgress(autoCommitProgressDTO.getProgress()); + autoCommitResponseDTO.setBranchName(autoCommitProgressDTO.getBranchName()); + return Mono.just(autoCommitResponseDTO); + } + + return isAutoCommitRequiredMono.flatMap(autoCommitTriggerDTO -> { + if (!Boolean.TRUE.equals(autoCommitTriggerDTO.getIsAutoCommitRequired())) { + log.info( + "application with id: {}, and branch name: {} is not eligible for autocommit", + defaultApplicationId, + branchName); + autoCommitResponseDTO.setAutoCommitResponse(IDLE); + return Mono.just(autoCommitResponseDTO); + } + + // Autocommit can be started + log.info( + "application with id: {}, and branch name: {} is eligible for autocommit", + defaultApplicationId, + branchName); + return gitAutoCommitHelper + .publishAutoCommitEvent(autoCommitTriggerDTO, defaultApplicationId, branchName) + .map(isEventPublished -> { + if (TRUE.equals(isEventPublished)) { + log.info( + "autocommit event for application with id: {}, and branch name: {} is published", + defaultApplicationId, + branchName); + autoCommitResponseDTO.setAutoCommitResponse(PUBLISHED); + return autoCommitResponseDTO; + } + + log.info( + "application with id: {}, and branch name: {} does not fulfil the prerequisite for autocommit", + defaultApplicationId, + branchName); + autoCommitResponseDTO.setAutoCommitResponse(REQUIRED); + return autoCommitResponseDTO; + }); + }); + }); + }); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceImpl.java new file mode 100644 index 0000000000..cca2b10615 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitServiceImpl.java @@ -0,0 +1,29 @@ +package com.appsmith.server.git.autocommit; + +import com.appsmith.server.applications.base.ApplicationService; +import com.appsmith.server.git.autocommit.helpers.AutoCommitEligibilityHelper; +import com.appsmith.server.git.autocommit.helpers.GitAutoCommitHelperImpl; +import com.appsmith.server.newpages.base.NewPageService; +import com.appsmith.server.solutions.ApplicationPermission; +import com.appsmith.server.solutions.PagePermission; +import org.springframework.stereotype.Component; + +@Component +public class AutoCommitServiceImpl extends AutoCommitServiceCEImpl implements AutoCommitService { + + public AutoCommitServiceImpl( + ApplicationService applicationService, + ApplicationPermission applicationPermission, + NewPageService newPageService, + PagePermission pagePermission, + AutoCommitEligibilityHelper autoCommitEligibilityHelper, + GitAutoCommitHelperImpl gitAutoCommitHelper) { + super( + applicationService, + applicationPermission, + newPageService, + pagePermission, + autoCommitEligibilityHelper, + gitAutoCommitHelper); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelper.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelper.java index 0174ba43b9..4eeed641f9 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelper.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelper.java @@ -11,6 +11,8 @@ public interface AutoCommitEligibilityHelper { Mono isClientMigrationRequired(PageDTO pageDTO); + Mono isClientMigrationRequiredFSOps(String workspaceId, GitArtifactMetadata gitMetadata, PageDTO pageDTO); + Mono isAutoCommitRequired( String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperFallbackImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperFallbackImpl.java index aa065319c2..a2e58db165 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperFallbackImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperFallbackImpl.java @@ -23,6 +23,12 @@ public class AutoCommitEligibilityHelperFallbackImpl implements AutoCommitEligib return Mono.just(FALSE); } + @Override + public Mono isClientMigrationRequiredFSOps( + String workspaceId, GitArtifactMetadata gitMetadata, PageDTO pageDTO) { + return Mono.just(FALSE); + } + @Override public Mono isAutoCommitRequired( String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java index 239e9c430c..2e96e38c51 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java @@ -2,8 +2,6 @@ package com.appsmith.server.git.autocommit.helpers; import com.appsmith.external.annotations.FeatureFlagged; import com.appsmith.external.enums.FeatureFlagEnum; -import com.appsmith.external.git.constants.GitConstants.GitCommandConstants; -import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.Layout; import com.appsmith.server.dtos.AutoCommitTriggerDTO; @@ -20,6 +18,8 @@ import org.springframework.context.annotation.Primary; import org.springframework.stereotype.Component; import reactor.core.publisher.Mono; +import static com.appsmith.external.git.constants.GitConstants.GitCommandConstants.AUTO_COMMIT_ELIGIBILITY; +import static com.appsmith.server.constants.ArtifactType.APPLICATION; import static java.lang.Boolean.FALSE; import static java.lang.Boolean.TRUE; @@ -35,16 +35,14 @@ public class AutoCommitEligibilityHelperImpl extends AutoCommitEligibilityHelper private final GitRedisUtils gitRedisUtils; @Override - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_server_autocommit_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_eligibility_enabled) public Mono isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata) { String defaultApplicationId = gitMetadata.getDefaultArtifactId(); String branchName = gitMetadata.getBranchName(); - String repoName = gitMetadata.getRepoName(); Mono isServerMigrationRequiredMonoCached = commonGitFileUtils - .getMetadataServerSchemaMigrationVersion( - workspaceId, defaultApplicationId, repoName, branchName, ArtifactType.APPLICATION) + .getMetadataServerSchemaMigrationVersion(workspaceId, gitMetadata, FALSE, APPLICATION) .map(serverSchemaVersion -> { log.info( "server schema for application id : {} and branch name : {} is : {}", @@ -56,22 +54,27 @@ public class AutoCommitEligibilityHelperImpl extends AutoCommitEligibilityHelper .defaultIfEmpty(FALSE) .cache(); - return Mono.defer(() -> gitRedisUtils.addFileLock(defaultApplicationId, GitCommandConstants.METADATA, false)) - .then(Mono.defer(() -> isServerMigrationRequiredMonoCached)) - .then(Mono.defer(() -> gitRedisUtils.releaseFileLock(defaultApplicationId))) - .then(Mono.defer(() -> isServerMigrationRequiredMonoCached)) - .onErrorResume(error -> { - log.debug( - "error while retrieving the metadata for defaultApplicationId : {}, branchName : {} error : {}", - defaultApplicationId, - branchName, - error.getMessage()); - return gitRedisUtils.releaseFileLock(defaultApplicationId).then(Mono.just(FALSE)); - }); + return Mono.defer(() -> isServerMigrationRequiredMonoCached).onErrorResume(error -> { + log.debug( + "error while retrieving the metadata for defaultApplicationId : {}, branchName : {} error : {}", + defaultApplicationId, + branchName, + error.getMessage()); + return Mono.just(FALSE); + }); } + /** + * This method has been deprecated and is not being used anymore. + * It's been deprecated because, we are using the absolute source of truth + * that is the version key in the layout. + * /pages/.json in file system for the finding out the Dsl in layout. + * @param pageDTO : pageDTO for the page for which migration was required. + * @return : a boolean whether the client requires a migration or not + */ @Override - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) + @Deprecated + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_eligibility_enabled) public Mono isClientMigrationRequired(PageDTO pageDTO) { return dslMigrationUtils .getLatestDslVersion() @@ -91,26 +94,71 @@ public class AutoCommitEligibilityHelperImpl extends AutoCommitEligibilityHelper } @Override - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_eligibility_enabled) + public Mono isClientMigrationRequiredFSOps( + String workspaceId, GitArtifactMetadata gitMetadata, PageDTO pageDTO) { + String defaultApplicationId = gitMetadata.getDefaultArtifactId(); + String branchName = gitMetadata.getBranchName(); + + Mono latestDslVersionMono = dslMigrationUtils.getLatestDslVersion(); + + Mono isClientMigrationRequired = latestDslVersionMono + .zipWith(commonGitFileUtils.getPageDslVersionNumber( + workspaceId, gitMetadata, pageDTO, TRUE, APPLICATION)) + .map(tuple2 -> { + Integer latestDslVersion = tuple2.getT1(); + org.json.JSONObject pageDSL = tuple2.getT2(); + log.info("page dsl retrieved from file system"); + return GitUtils.isMigrationRequired(pageDSL, latestDslVersion); + }) + .defaultIfEmpty(FALSE) + .cache(); + + return Mono.defer(() -> isClientMigrationRequired).onErrorResume(error -> { + log.debug( + "error while fetching the dsl version for page : {}, defaultApplicationId : {}, branchName : {} error : {}", + pageDTO.getName(), + defaultApplicationId, + branchName, + error.getMessage()); + return Mono.just(FALSE); + }); + } + + @Override + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_eligibility_enabled) public Mono isAutoCommitRequired( String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) { + String defaultApplicationId = gitArtifactMetadata.getDefaultApplicationId(); + Mono isClientAutocommitRequiredMono = - isClientMigrationRequired(pageDTO).defaultIfEmpty(FALSE); + isClientMigrationRequiredFSOps(workspaceId, gitArtifactMetadata, pageDTO); - Mono isServerAutocommitRequiredMono = isServerAutoCommitRequired(workspaceId, gitArtifactMetadata); + Mono isServerAutocommitRequiredMono = + isServerAutoCommitRequired(workspaceId, gitArtifactMetadata).cache(); - return isServerAutocommitRequiredMono - .zipWith(isClientAutocommitRequiredMono) - .map(tuple2 -> { - Boolean serverFlag = tuple2.getT1(); - Boolean clientFlag = tuple2.getT2(); + return Mono.defer(() -> gitRedisUtils.addFileLock(defaultApplicationId, AUTO_COMMIT_ELIGIBILITY)) + .then(isClientAutocommitRequiredMono.zipWhen(clientFlag -> { + Mono serverFlagMono = isServerAutocommitRequiredMono; + // if client is required to migrate then, + // there is no requirement to fetch server flag as server is subset of client migration. + if (Boolean.TRUE.equals(clientFlag)) { + serverFlagMono = Mono.just(TRUE); + } + + return serverFlagMono; + })) + .flatMap(tuple2 -> { + Boolean clientFlag = tuple2.getT1(); + Boolean serverFlag = tuple2.getT2(); AutoCommitTriggerDTO autoCommitTriggerDTO = new AutoCommitTriggerDTO(); autoCommitTriggerDTO.setIsClientAutoCommitRequired(TRUE.equals(clientFlag)); autoCommitTriggerDTO.setIsServerAutoCommitRequired(TRUE.equals(serverFlag)); autoCommitTriggerDTO.setIsAutoCommitRequired((TRUE.equals(serverFlag) || TRUE.equals(clientFlag))); - return autoCommitTriggerDTO; + + return gitRedisUtils.releaseFileLock(defaultApplicationId).then(Mono.just(autoCommitTriggerDTO)); }); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelper.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelper.java index be02072ba4..398b0325e4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelper.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelper.java @@ -1,17 +1,17 @@ package com.appsmith.server.git.autocommit.helpers; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.AutoCommitTriggerDTO; import reactor.core.publisher.Mono; public interface GitAutoCommitHelper { - Mono getAutoCommitProgress(String applicationId); + Mono getAutoCommitProgress(String defaultApplicationId, String branchName); Mono autoCommitClientMigration(String defaultApplicationId, String branchName); Mono autoCommitServerMigration(String defaultApplicationId, String branchName); - Mono autoCommitApplication( + Mono publishAutoCommitEvent( AutoCommitTriggerDTO autoCommitTriggerDTO, String defaultApplicationId, String branchName); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperFallbackImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperFallbackImpl.java index 6ab3928118..07a5ff8526 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperFallbackImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperFallbackImpl.java @@ -1,6 +1,6 @@ package com.appsmith.server.git.autocommit.helpers; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.AutoCommitTriggerDTO; import org.springframework.stereotype.Component; import reactor.core.publisher.Mono; @@ -19,12 +19,12 @@ public class GitAutoCommitHelperFallbackImpl implements GitAutoCommitHelper { } @Override - public Mono getAutoCommitProgress(String applicationId) { - return Mono.empty(); + public Mono getAutoCommitProgress(String defaultApplicationId, String branchName) { + return Mono.just(new AutoCommitResponseDTO(AutoCommitResponseDTO.AutoCommitResponse.IDLE)); } @Override - public Mono autoCommitApplication( + public Mono publishAutoCommitEvent( AutoCommitTriggerDTO autoCommitTriggerDTO, String defaultApplicationId, String branchName) { return Mono.just(Boolean.FALSE); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImpl.java index e4897cb0e7..61476e3dea 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImpl.java @@ -6,10 +6,10 @@ import com.appsmith.server.applications.base.ApplicationService; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.GitProfile; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.AutoCommitTriggerDTO; import com.appsmith.server.events.AutoCommitEvent; -import com.appsmith.server.git.AutoCommitEventHandler; +import com.appsmith.server.git.autocommit.AutoCommitEventHandler; import com.appsmith.server.git.common.CommonGitService; import com.appsmith.server.helpers.GitPrivateRepoHelper; import com.appsmith.server.helpers.GitUtils; @@ -23,6 +23,10 @@ import org.springframework.stereotype.Service; import org.springframework.util.StringUtils; import reactor.core.publisher.Mono; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IDLE; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IN_PROGRESS; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.LOCKED; + @Slf4j @Primary @Service @@ -53,17 +57,26 @@ public class GitAutoCommitHelperImpl extends GitAutoCommitHelperFallbackImpl imp } @Override - public Mono getAutoCommitProgress(String applicationId) { + public Mono getAutoCommitProgress(String defaultApplicationId, String branchName) { return redisUtils - .getRunningAutoCommitBranchName(applicationId) - .zipWith(redisUtils.getAutoCommitProgress(applicationId)) + .getRunningAutoCommitBranchName(defaultApplicationId) + .zipWith(redisUtils.getAutoCommitProgress(defaultApplicationId)) .map(tuple2 -> { - AutoCommitProgressDTO autoCommitProgressDTO = new AutoCommitProgressDTO(Boolean.TRUE); - autoCommitProgressDTO.setBranchName(tuple2.getT1()); - autoCommitProgressDTO.setProgress(tuple2.getT2()); - return autoCommitProgressDTO; + String branchNameFromRedis = tuple2.getT1(); + + AutoCommitResponseDTO autoCommitResponseDTO = new AutoCommitResponseDTO(); + autoCommitResponseDTO.setProgress(tuple2.getT2()); + autoCommitResponseDTO.setBranchName(branchNameFromRedis); + + if (branchNameFromRedis.equals(branchName)) { + autoCommitResponseDTO.setAutoCommitResponse(IN_PROGRESS); + } else { + autoCommitResponseDTO.setAutoCommitResponse(LOCKED); + } + + return autoCommitResponseDTO; }) - .defaultIfEmpty(new AutoCommitProgressDTO(Boolean.FALSE)); + .defaultIfEmpty(new AutoCommitResponseDTO(IDLE)); } /** @@ -114,7 +127,7 @@ public class GitAutoCommitHelperImpl extends GitAutoCommitHelperFallbackImpl imp } @Override - @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_server_autocommit_feature_enabled) + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) public Mono autoCommitServerMigration(String defaultApplicationId, String branchName) { return autoCommitApplication(defaultApplicationId, branchName, Boolean.FALSE); } @@ -203,7 +216,7 @@ public class GitAutoCommitHelperImpl extends GitAutoCommitHelperFallbackImpl imp }); } - public Mono autoCommitApplication( + public Mono publishAutoCommitEvent( AutoCommitTriggerDTO autoCommitTriggerDTO, String defaultApplicationId, String branchName) { if (!Boolean.TRUE.equals(autoCommitTriggerDTO.getIsAutoCommitRequired())) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCE.java index aeaae728bc..4c19d46218 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCE.java @@ -10,7 +10,7 @@ import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.GitAuth; import com.appsmith.server.domains.GitProfile; import com.appsmith.server.dtos.ArtifactImportDTO; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.GitCommitDTO; import com.appsmith.server.dtos.GitConnectDTO; import com.appsmith.server.dtos.GitDocsDTO; @@ -105,7 +105,8 @@ public interface CommonGitServiceCE { Mono toggleAutoCommitEnabled(String defaultArtifactId, ArtifactType artifactType); - Mono getAutoCommitProgress(String applicationId, ArtifactType artifactType); + Mono getAutoCommitProgress( + String applicationId, String branchName, ArtifactType artifactType); Mono autoCommitApplication(String defaultApplicationId, String branchName, ArtifactType artifactType); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java index 9fe9ba316c..b5e0871bdc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java @@ -34,7 +34,7 @@ import com.appsmith.server.domains.Workspace; import com.appsmith.server.dtos.ApplicationImportDTO; import com.appsmith.server.dtos.ArtifactExchangeJson; import com.appsmith.server.dtos.ArtifactImportDTO; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.dtos.GitCommitDTO; import com.appsmith.server.dtos.GitConnectDTO; import com.appsmith.server.dtos.GitDocsDTO; @@ -3377,8 +3377,9 @@ public class CommonGitServiceCEImpl implements CommonGitServiceCE { } @Override - public Mono getAutoCommitProgress(String artifactId, ArtifactType artifactType) { - return gitAutoCommitHelper.getAutoCommitProgress(artifactId); + public Mono getAutoCommitProgress( + String artifactId, String branchName, ArtifactType artifactType) { + return gitAutoCommitHelper.getAutoCommitProgress(artifactId, branchName); } @Override diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java index 1a3cfb78e6..ea3a9e7eaf 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java @@ -1,11 +1,13 @@ package com.appsmith.server.helpers; import com.appsmith.external.git.FileInterface; +import com.appsmith.external.git.operations.FileOperations; +import com.appsmith.external.models.ApplicationGitReference; import com.appsmith.git.files.FileUtilsImpl; -import com.appsmith.server.applications.git.ApplicationGitFileUtilsImpl; import com.appsmith.server.helpers.ce.CommonGitFileUtilsCE; import com.appsmith.server.services.AnalyticsService; import com.appsmith.server.services.SessionUserService; +import com.google.gson.Gson; import lombok.extern.slf4j.Slf4j; import org.springframework.context.annotation.Import; import org.springframework.stereotype.Component; @@ -16,10 +18,12 @@ import org.springframework.stereotype.Component; public class CommonGitFileUtils extends CommonGitFileUtilsCE { public CommonGitFileUtils( - ApplicationGitFileUtilsImpl applicationGitFileUtils, + ArtifactGitFileUtils applicationGitFileUtils, FileInterface fileUtils, + FileOperations fileOperations, AnalyticsService analyticsService, - SessionUserService sessionUserService) { - super(applicationGitFileUtils, fileUtils, analyticsService, sessionUserService); + SessionUserService sessionUserService, + Gson gson) { + super(applicationGitFileUtils, fileUtils, fileOperations, analyticsService, sessionUserService); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java index fd7ecc01f9..ce2d1eac96 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java @@ -177,6 +177,18 @@ public class GitUtils { return isMigrationRequired; } + public static boolean isMigrationRequired(org.json.JSONObject layoutDsl, Integer latestDslVersion) { + boolean isMigrationRequired = true; + String versionKey = "version"; + if (layoutDsl.has(versionKey)) { + int currentDslVersion = layoutDsl.getInt(versionKey); + if (currentDslVersion >= latestDslVersion) { + isMigrationRequired = false; + } + } + return isMigrationRequired; + } + public static boolean isAutoCommitEnabled(GitArtifactMetadata gitArtifactMetadata) { return gitArtifactMetadata.getAutoCommitConfig() == null || gitArtifactMetadata.getAutoCommitConfig().getEnabled(); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java index 819c225898..f5a0a66ed6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java @@ -2,6 +2,7 @@ package com.appsmith.server.helpers.ce; import com.appsmith.external.constants.AnalyticsEvents; import com.appsmith.external.git.FileInterface; +import com.appsmith.external.git.operations.FileOperations; import com.appsmith.external.helpers.Stopwatch; import com.appsmith.external.models.ApplicationGitReference; import com.appsmith.external.models.ArtifactGitReference; @@ -11,8 +12,10 @@ import com.appsmith.git.constants.CommonConstants; import com.appsmith.git.files.FileUtilsImpl; import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.constants.FieldName; +import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.dtos.ArtifactExchangeJson; +import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.ArtifactGitFileUtils; @@ -25,6 +28,7 @@ import com.google.gson.JsonObject; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.eclipse.jgit.api.errors.GitAPIException; +import org.json.JSONObject; import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Import; import org.springframework.stereotype.Component; @@ -37,6 +41,8 @@ import java.nio.file.Paths; import java.util.HashMap; import java.util.Map; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.GitCommandConstantsCE.CHECKOUT_BRANCH; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.RECONSTRUCT_PAGE; import static com.appsmith.git.constants.CommonConstants.CLIENT_SCHEMA_VERSION; import static com.appsmith.git.constants.CommonConstants.FILE_FORMAT_VERSION; import static com.appsmith.git.constants.CommonConstants.SERVER_SCHEMA_VERSION; @@ -50,6 +56,7 @@ public class CommonGitFileUtilsCE { protected final ArtifactGitFileUtils applicationGitFileUtils; private final FileInterface fileUtils; + private final FileOperations fileOperations; private final AnalyticsService analyticsService; private final SessionUserService sessionUserService; @@ -282,15 +289,21 @@ public class CommonGitFileUtilsCE { } public Mono> reconstructMetadataFromRepo( - String workspaceId, String applicationId, String repoName, String branchName, ArtifactType artifactType) { + String workspaceId, + String applicationId, + String repoName, + String branchName, + Boolean isResetToLastCommitRequired, + ArtifactType artifactType) { ArtifactGitFileUtils artifactGitFileUtils = getArtifactBasedFileHelper(artifactType); Path baseRepoSuffix = artifactGitFileUtils.getRepoSuffixPath(workspaceId, applicationId, repoName); return fileUtils - .reconstructMetadataFromGitRepo(workspaceId, applicationId, repoName, branchName, baseRepoSuffix) + .reconstructMetadataFromGitRepo( + workspaceId, applicationId, repoName, branchName, baseRepoSuffix, isResetToLastCommitRequired) .onErrorResume(error -> Mono.error( - new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout", error.getMessage()))) + new AppsmithException(AppsmithError.GIT_ACTION_FAILED, CHECKOUT_BRANCH, error.getMessage()))) .map(metadata -> { Gson gson = new Gson(); JsonObject metadataJsonObject = @@ -310,20 +323,22 @@ public class CommonGitFileUtilsCE { /** * Provides the server schema version in the application json for the given branch * - * @param workspaceId : workspaceId of the artifact - * @param defaultArtifactId : default branch id of the artifact - * @param repoName : repository name - * @param branchName : current branch name of the artifact - * @param artifactType : artifact type of this operation + * @param workspaceId : workspaceId of the artifact + * @param gitArtifactMetadata : git artifact metadata of the application + * @param isResetToLastCommitRequired : would we need to execute reset command + * @param artifactType : artifact type of this operation * @return the server schema migration version number */ public Mono getMetadataServerSchemaMigrationVersion( String workspaceId, - String defaultArtifactId, - String repoName, - String branchName, + GitArtifactMetadata gitArtifactMetadata, + Boolean isResetToLastCommitRequired, ArtifactType artifactType) { + String defaultArtifactId = gitArtifactMetadata.getDefaultArtifactId(); + String branchName = gitArtifactMetadata.getBranchName(); + String repoName = gitArtifactMetadata.getRepoName(); + if (!hasText(workspaceId)) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.WORKSPACE_ID)); } @@ -340,11 +355,69 @@ public class CommonGitFileUtilsCE { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.REPO_NAME)); } - return reconstructMetadataFromRepo(workspaceId, defaultArtifactId, repoName, branchName, artifactType) + Mono serverSchemaNumberMono = reconstructMetadataFromRepo( + workspaceId, defaultArtifactId, repoName, branchName, isResetToLastCommitRequired, artifactType) .map(metadataMap -> { return metadataMap.getOrDefault( CommonConstants.SERVER_SCHEMA_VERSION, JsonSchemaVersions.serverVersion); }); + + return Mono.create( + sink -> serverSchemaNumberMono.subscribe(sink::success, sink::error, null, sink.currentContext())); + } + + /** + * Provides the server schema version in the application json for the given branch + * + * @param workspaceId : workspace id of the application + * @param gitArtifactMetadata : git artifact metadata + * @param isResetToLastCommitRequired : whether git reset hard is required + * @param artifactType : artifact type of this operation + * @return the server schema migration version number + */ + public Mono getPageDslVersionNumber( + String workspaceId, + GitArtifactMetadata gitArtifactMetadata, + PageDTO pageDTO, + Boolean isResetToLastCommitRequired, + ArtifactType artifactType) { + + String defaultArtifactId = gitArtifactMetadata.getDefaultArtifactId(); + String branchName = gitArtifactMetadata.getBranchName(); + String repoName = gitArtifactMetadata.getRepoName(); + + if (!hasText(workspaceId)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.WORKSPACE_ID)); + } + + if (!hasText(defaultArtifactId)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID)); + } + + if (!hasText(branchName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); + } + + if (!hasText(repoName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.REPO_NAME)); + } + + if (pageDTO == null) { + return Mono.error(new AppsmithException(AppsmithError.PAGE_ID_NOT_GIVEN, FieldName.PAGE)); + } + + ArtifactGitFileUtils artifactGitFileUtils = getArtifactBasedFileHelper(artifactType); + Path baseRepoSuffix = artifactGitFileUtils.getRepoSuffixPath(workspaceId, defaultArtifactId, repoName); + + Mono jsonObjectMono = fileUtils + .reconstructPageFromGitRepo(pageDTO.getName(), branchName, baseRepoSuffix, isResetToLastCommitRequired) + .onErrorResume(error -> Mono.error( + new AppsmithException(AppsmithError.GIT_ACTION_FAILED, RECONSTRUCT_PAGE, error.getMessage()))) + .map(pageJson -> { + return fileOperations.getMainContainer(pageJson); + }); + + return Mono.create(sink -> jsonObjectMono.subscribe(sink::success, sink::error, null, sink.currentContext())); } private Integer getServerSchemaVersion(JsonObject metadataJsonObject) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java index ff883653a4..34cc075b16 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java @@ -123,8 +123,10 @@ public class JsonSchemaMigration { applicationJson.setServerSchemaVersion(6); case 6: MigrationHelperMethods.ensureXmlParserPresenceInCustomJsLibList(applicationJson); - applicationJson.setServerSchemaVersion(7); + case 7: + applicationJson.setServerSchemaVersion(8); + default: // Unable to detect the serverSchema } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersions.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersions.java index 2b81d9a020..dc55314e09 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersions.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersions.java @@ -12,6 +12,6 @@ import lombok.Getter; */ @Getter public class JsonSchemaVersions { - public static final Integer serverVersion = 7; + public static final Integer serverVersion = 8; public static final Integer clientVersion = 1; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java index 2423ea5b1c..946b439a98 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java @@ -101,4 +101,7 @@ public interface NewPageServiceCE extends CrudService { Application branchedApplication, List newPages, boolean viewMode, boolean isRecentlyAccessed); Mono updateDependencyMap(String pageId, Map> dependencyMap, String branchName); + + Flux findByApplicationIdAndApplicationMode( + String applicationId, AclPermission permission, ApplicationMode applicationMode); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java index baf462f4b7..64125e6f3b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java @@ -687,4 +687,23 @@ public class NewPageServiceCEImpl extends BaseService findByApplicationIdAndApplicationMode( + String applicationId, AclPermission permission, ApplicationMode applicationMode) { + Boolean viewMode = ApplicationMode.PUBLISHED.equals(applicationMode); + return findNewPagesByApplicationId(applicationId, permission) + .filter(page -> { + PageDTO pageDTO; + if (ApplicationMode.PUBLISHED.equals(applicationMode)) { + pageDTO = page.getPublishedPage(); + } else { + pageDTO = page.getUnpublishedPage(); + } + + boolean isDeletedOrNull = pageDTO == null || pageDTO.getDeletedAt() != null; + return !isDeletedOrNull; + }) + .flatMap(page -> getPageByViewMode(page, viewMode)); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java index 3c91125686..b594e89d97 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java @@ -95,8 +95,6 @@ public class ApplicationPageServiceImpl extends ApplicationPageServiceCEImpl imp datasourceRepository, datasourcePermission, dslMigrationUtils, - gitAutoCommitHelper, - autoCommitEligibilityHelper, actionClonePageService, actionCollectionClonePageService); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java index 46fa552753..f0bb7158cc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java @@ -34,9 +34,6 @@ import com.appsmith.server.dtos.PageNameIdDTO; import com.appsmith.server.dtos.PluginTypeAndCountDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; -import com.appsmith.server.git.autocommit.helpers.AutoCommitEligibilityHelper; -import com.appsmith.server.git.autocommit.helpers.GitAutoCommitHelper; -import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; import com.appsmith.server.helpers.GitUtils; @@ -127,8 +124,6 @@ public class ApplicationPageServiceCEImpl implements ApplicationPageServiceCE { private final DatasourceRepository datasourceRepository; private final DatasourcePermission datasourcePermission; private final DSLMigrationUtils dslMigrationUtils; - private final GitAutoCommitHelper gitAutoCommitHelper; - private final AutoCommitEligibilityHelper autoCommitEligibilityHelper; private final ClonePageService actionClonePageService; private final ClonePageService actionCollectionClonePageService; @@ -299,76 +294,7 @@ public class ApplicationPageServiceCEImpl implements ApplicationPageServiceCE { return newPageService .findNewPagesByApplicationId(branchedApplication.getId(), pagePermission.getReadPermission()) .filter(newPage -> pageIds.contains(newPage.getId())) - .collectList() - .flatMap(newPageList -> { - if (Boolean.TRUE.equals(viewMode)) { - return Mono.just(newPageList); - } - - // autocommit if migration is required - return migrateSchemasForGitConnectedApps(branchedApplication, newPageList) - .onErrorResume(error -> { - log.debug( - "Skipping the autocommit for applicationId : {} due to error; {}", - branchedApplication.getId(), - error.getMessage()); - - return Mono.just(Boolean.FALSE); - }) - .thenReturn(newPageList); - }); - } - - /** - * Publishes the autocommit if it's eligible for one - * @param application : the branched application which requires schemaMigration - * @param newPages : list of pages from db - * @return : a boolean publisher - */ - private Mono migrateSchemasForGitConnectedApps(Application application, List newPages) { - - if (CollectionUtils.isNullOrEmpty(newPages)) { - return Mono.just(Boolean.FALSE); - } - - GitArtifactMetadata gitMetadata = application.getGitArtifactMetadata(); - - if (application.getGitArtifactMetadata() == null) { - return Mono.just(Boolean.FALSE); - } - - String defaultApplicationId = gitMetadata.getDefaultArtifactId(); - String branchName = gitMetadata.getBranchName(); - String workspaceId = application.getWorkspaceId(); - - if (!StringUtils.hasText(branchName)) { - log.debug( - "Skipping the autocommit for applicationId : {}, branch name is not present", application.getId()); - return Mono.just(Boolean.FALSE); - } - - if (!StringUtils.hasText(defaultApplicationId)) { - log.debug( - "Skipping the autocommit for applicationId : {}, defaultApplicationId is not present", - application.getId()); - return Mono.just(Boolean.FALSE); - } - - // since this method is only called when the app is in edit mode - Mono pageDTOMono = getPage(newPages.get(0), false); - - return pageDTOMono.flatMap(pageDTO -> { - return autoCommitEligibilityHelper - .isAutoCommitRequired(workspaceId, gitMetadata, pageDTO) - .flatMap(autoCommitTriggerDTO -> { - if (Boolean.TRUE.equals(autoCommitTriggerDTO.getIsAutoCommitRequired())) { - return gitAutoCommitHelper.autoCommitApplication( - autoCommitTriggerDTO, defaultApplicationId, branchName); - } - - return Mono.just(Boolean.FALSE); - }); - }); + .collectList(); } @Override diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java index 8d9542cd3f..9bf2f1c001 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java @@ -1,25 +1,37 @@ package com.appsmith.server.git; +import com.appsmith.external.converters.ISOStringToInstantConverter; +import com.appsmith.external.dtos.ModifiedResources; +import com.appsmith.external.enums.FeatureFlagEnum; +import com.appsmith.external.git.GitExecutor; import com.appsmith.external.models.ApplicationGitReference; -import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.constants.SerialiseArtifactObjective; import com.appsmith.server.domains.Workspace; import com.appsmith.server.dtos.ApplicationImportDTO; import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.exports.internal.ExportService; +import com.appsmith.server.featureflags.CachedFeatures; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.MockPluginExecutor; import com.appsmith.server.helpers.PluginExecutorHelper; import com.appsmith.server.imports.internal.ImportService; +import com.appsmith.server.services.FeatureFlagService; import com.appsmith.server.services.WorkspaceService; +import com.appsmith.server.testhelpers.git.GitFileSystemTestHelper; import com.google.gson.Gson; +import com.google.gson.GsonBuilder; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import lombok.extern.slf4j.Slf4j; +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.diff.DiffEntry; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; +import org.mockito.stubbing.Answer; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.data.mongo.AutoConfigureDataMongo; import org.springframework.boot.test.context.SpringBootTest; @@ -37,10 +49,19 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.io.IOException; import java.net.URISyntaxException; import java.net.URL; import java.nio.file.Path; +import java.nio.file.Paths; +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Set; +import static com.appsmith.server.constants.ArtifactType.APPLICATION; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -78,9 +99,6 @@ import static org.mockito.ArgumentMatchers.any; @DirtiesContext public class ServerSchemaMigrationEnforcerTest { - @Autowired - Gson gson; - @Autowired WorkspaceService workspaceService; @@ -93,9 +111,28 @@ public class ServerSchemaMigrationEnforcerTest { @Autowired CommonGitFileUtils commonGitFileUtils; + @Autowired + GitFileSystemTestHelper gitFileSystemTestHelper; + + @SpyBean + GitExecutor gitExecutor; + + @SpyBean + FeatureFlagService featureFlagService; + @MockBean PluginExecutorHelper pluginExecutorHelper; + private final Gson gson = new GsonBuilder() + .registerTypeAdapter(Instant.class, new ISOStringToInstantConverter()) + .setPrettyPrinting() + .create(); + + private static final String DEFAULT_APPLICATION_ID = "default-app-id", + BRANCH_NAME = "develop", + REPO_NAME = "repoName", + WORKSPACE_ID = "test-workspace-id"; + public static final String CUSTOM_JS_LIB_LIST = "jsLibraries"; public static final String EXPORTED_APPLICATION = "application"; public static final String UNPUBLISHED_CUSTOM_JS_LIBS = "unpublishedCustomJSLibs"; @@ -167,6 +204,7 @@ public class ServerSchemaMigrationEnforcerTest { } @Test + @Disabled @WithUserDetails(value = "api_user") public void importApplication_ThenExportApplication_MatchJson_equals_Success() throws URISyntaxException { String filePath = "ce-automation-test.json"; @@ -199,7 +237,7 @@ public class ServerSchemaMigrationEnforcerTest { .exportByArtifactId( applicationImportDTO.getApplication().getId(), SerialiseArtifactObjective.VERSION_CONTROL, - ArtifactType.APPLICATION) + APPLICATION) .map(artifactExchangeJson -> (ApplicationJson) artifactExchangeJson); }); @@ -254,4 +292,138 @@ public class ServerSchemaMigrationEnforcerTest { } } } + + @Test + public void savedFile_reSavedWithDifferentSerialisationLogic_diffOccurs() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource("ce-automation-test.json")); + + ModifiedResources modifiedResources = new ModifiedResources(); + modifiedResources.setAllModified(true); + applicationJson.setModifiedResources(modifiedResources); + + CachedFeatures cachedFeatures = new CachedFeatures(); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), FALSE)); + Mockito.when(featureFlagService.getCachedTenantFeatureFlags()) + .thenAnswer((Answer) invocations -> cachedFeatures); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), TRUE)); + Path suffixPath = Paths.get(WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME); + Path gitCompletePath = gitExecutor.createRepoPath(suffixPath); + + commonGitFileUtils + .saveArtifactToLocalRepo(suffixPath, applicationJson, BRANCH_NAME) + .block(); + + try (Git gitRepo = Git.open(gitCompletePath.toFile())) { + List diffEntries = gitRepo.diff().call(); + Set fileChanges = Set.of( + "application.json", + "metadata.json", + "theme.json", + "datasources/JSON typicode API (1).json", + "datasources/TED postgres (1).json", + "datasources/mainGoogleSheetDS.json"); + for (DiffEntry diff : diffEntries) { + assertThat(fileChanges).contains(diff.getOldPath()); + assertThat(fileChanges).contains(diff.getNewPath()); + assertThat(diff.getChangeType()).isEqualTo(DiffEntry.ChangeType.MODIFY); + } + assertThat(diffEntries.size()).isNotZero(); + } + } + + @Test + public void savedFile_reSavedWithSameSerialisationLogic_noDiffOccurs() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource("ce-automation-test.json")); + + ModifiedResources modifiedResources = new ModifiedResources(); + modifiedResources.setAllModified(true); + applicationJson.setModifiedResources(modifiedResources); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + Path suffixPath = Paths.get(WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME); + Path gitCompletePath = gitExecutor.createRepoPath(suffixPath); + + commonGitFileUtils + .saveArtifactToLocalRepo(suffixPath, applicationJson, BRANCH_NAME) + .block(); + + try (Git gitRepo = Git.open(gitCompletePath.toFile())) { + List diffEntries = gitRepo.diff().call(); + assertThat(diffEntries.size()).isZero(); + } + } + + @Test + @WithUserDetails(value = "api_user") + public void saveGitRepo_ImportAndThenExport_diffOccurs() throws URISyntaxException, IOException, GitAPIException { + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource("ce-automation-test.json")); + + ModifiedResources modifiedResources = new ModifiedResources(); + modifiedResources.setAllModified(true); + applicationJson.setModifiedResources(modifiedResources); + + CachedFeatures cachedFeatures = new CachedFeatures(); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), TRUE)); + Mockito.when(featureFlagService.getCachedTenantFeatureFlags()) + .thenAnswer((Answer) invocations -> cachedFeatures); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + ApplicationJson jsonToBeImported = commonGitFileUtils + .reconstructArtifactExchangeJsonFromGitRepo( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, APPLICATION) + .map(artifactExchangeJson -> (ApplicationJson) artifactExchangeJson) + .block(); + + Workspace newWorkspace = new Workspace(); + newWorkspace.setName("Template Workspace1"); + Workspace workspace = workspaceService.create(newWorkspace).block(); + + ApplicationJson exportedJson = importService + .importNewArtifactInWorkspaceFromJson(workspace.getId(), jsonToBeImported) + .flatMap(artifactExchangeJson -> { + return exportService + .exportByArtifactId( + artifactExchangeJson.getId(), + SerialiseArtifactObjective.VERSION_CONTROL, + APPLICATION) + .map(exportArtifactJson -> { + ApplicationJson applicationJson1 = (ApplicationJson) exportArtifactJson; + applicationJson1.setModifiedResources(modifiedResources); + return applicationJson1; + }); + }) + .block(); + + Path suffixPath = Paths.get(WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME); + Path gitCompletePath = gitExecutor.createRepoPath(suffixPath); + + // save back to the repository in order to compare the diff. + commonGitFileUtils + .saveArtifactToLocalRepo(suffixPath, exportedJson, BRANCH_NAME) + .block(); + + try (Git gitRepo = Git.open(gitCompletePath.toFile())) { + List diffEntries = gitRepo.diff().call(); + assertThat(diffEntries.size()).isNotZero(); + for (DiffEntry diffEntry : diffEntries) { + // assertion that no new file has been created + assertThat(diffEntry.getOldPath()).isEqualTo(diffEntry.getNewPath()); + } + } + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java index 2a6ac95ac0..e69de29bb2 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java @@ -1,394 +0,0 @@ -package com.appsmith.server.git.autocommit; - -import com.appsmith.external.dtos.GitLogDTO; -import com.appsmith.external.enums.FeatureFlagEnum; -import com.appsmith.external.git.GitExecutor; -import com.appsmith.external.helpers.AppsmithBeanUtils; -import com.appsmith.server.acl.AclPermission; -import com.appsmith.server.applications.base.ApplicationService; -import com.appsmith.server.domains.Application; -import com.appsmith.server.domains.ApplicationMode; -import com.appsmith.server.domains.ApplicationPage; -import com.appsmith.server.domains.GitArtifactMetadata; -import com.appsmith.server.domains.GitAuth; -import com.appsmith.server.domains.GitProfile; -import com.appsmith.server.domains.Layout; -import com.appsmith.server.domains.NewPage; -import com.appsmith.server.dtos.ApplicationJson; -import com.appsmith.server.dtos.AutoCommitTriggerDTO; -import com.appsmith.server.dtos.PageDTO; -import com.appsmith.server.git.autocommit.helpers.AutoCommitEligibilityHelper; -import com.appsmith.server.git.common.CommonGitService; -import com.appsmith.server.helpers.DSLMigrationUtils; -import com.appsmith.server.helpers.GitPrivateRepoHelper; -import com.appsmith.server.migrations.JsonSchemaMigration; -import com.appsmith.server.migrations.JsonSchemaVersions; -import com.appsmith.server.newpages.base.NewPageService; -import com.appsmith.server.services.ApplicationPageService; -import com.appsmith.server.services.FeatureFlagService; -import com.appsmith.server.services.UserDataService; -import com.appsmith.server.testhelpers.git.GitFileSystemTestHelper; -import lombok.extern.slf4j.Slf4j; -import net.minidev.json.JSONObject; -import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.lib.BranchTrackingStatus; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mockito; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.mock.mockito.MockBean; -import org.springframework.boot.test.mock.mockito.SpyBean; -import org.springframework.test.context.junit.jupiter.SpringExtension; -import reactor.core.publisher.Flux; -import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; - -import java.io.IOException; -import java.net.URISyntaxException; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.time.Duration; -import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; - -import static com.appsmith.server.git.AutoCommitEventHandlerCEImpl.AUTO_COMMIT_MSG_FORMAT; -import static java.lang.Boolean.FALSE; -import static java.lang.Boolean.TRUE; -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.doReturn; - -@ExtendWith(SpringExtension.class) -@SpringBootTest -@Slf4j -public class ApplicationPageServiceAutoCommitTest { - - @SpyBean - ApplicationPageService applicationPageService; - - @Autowired - GitFileSystemTestHelper gitFileSystemTestHelper; - - @SpyBean - GitExecutor gitExecutor; - - @MockBean - FeatureFlagService featureFlagService; - - @MockBean - DSLMigrationUtils dslMigrationUtils; - - @MockBean - ApplicationService applicationService; - - @MockBean - NewPageService newPageService; - - @MockBean - CommonGitService commonGitService; - - @MockBean - GitPrivateRepoHelper gitPrivateRepoHelper; - - @SpyBean - AutoCommitEligibilityHelper autoCommitEligibilityHelper; - - @MockBean - BranchTrackingStatus branchTrackingStatus; - - @MockBean - UserDataService userDataService; - - @SpyBean - JsonSchemaMigration jsonSchemaMigration; - - Application testApplication; - - Path baseRepoSuffix; - - private static final Integer DSL_VERSION_NUMBER = 88; - private static final String WORKSPACE_ID = "test-workspace"; - private static final String REPO_NAME = "test-repo"; - private static final String BRANCH_NAME = "develop"; - private static final String APP_JSON_NAME = "autocommit.json"; - private static final String APP_NAME = "autocommit"; - private static final Integer WAIT_DURATION_FOR_ASYNC_EVENT = 5; - private static final String PUBLIC_KEY = "public-key"; - private static final String PRIVATE_KEY = "private-key"; - private static final String REPO_URL = "domain.xy"; - private static final String DEFAULT_APP_ID = "default-app-id", DEFAULT_BRANCH_NAME = "master"; - - private Application createApplication() { - Application application = new Application(); - application.setName(APP_NAME); - application.setWorkspaceId(WORKSPACE_ID); - application.setId(DEFAULT_APP_ID); - - ApplicationPage applicationPage = new ApplicationPage(); - applicationPage.setId("testPageId"); - applicationPage.setIsDefault(TRUE); - - application.setPages(List.of(applicationPage)); - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - gitArtifactMetadata.setBranchName(BRANCH_NAME); - gitArtifactMetadata.setDefaultBranchName(DEFAULT_BRANCH_NAME); - gitArtifactMetadata.setRepoName(REPO_NAME); - gitArtifactMetadata.setDefaultApplicationId(DEFAULT_APP_ID); - gitArtifactMetadata.setRemoteUrl(REPO_URL); - - GitAuth gitAuth = new GitAuth(); - gitAuth.setPrivateKey(PRIVATE_KEY); - gitAuth.setPublicKey(PUBLIC_KEY); - gitArtifactMetadata.setGitAuth(gitAuth); - - application.setGitApplicationMetadata(gitArtifactMetadata); - return application; - } - - private GitProfile createGitProfile() { - GitProfile gitProfile = new GitProfile(); - gitProfile.setAuthorName("authorName"); - gitProfile.setAuthorEmail("author@domain.xy"); - return gitProfile; - } - - private NewPage createNewPage() { - JSONObject jsonObject = new JSONObject(); - jsonObject.put("key", "value"); - jsonObject.put("version", DSL_VERSION_NUMBER); - - Layout layout1 = new Layout(); - layout1.setId("testLayoutId"); - layout1.setDsl(jsonObject); - - PageDTO pageDTO = new PageDTO(); - pageDTO.setId("testPageId"); - pageDTO.setApplicationId(DEFAULT_APP_ID); - pageDTO.setLayouts(List.of(layout1)); - - NewPage newPage = new NewPage(); - newPage.setId("testPageId"); - newPage.setApplicationId(DEFAULT_APP_ID); - newPage.setUnpublishedPage(pageDTO); - return newPage; - } - - @BeforeEach - public void beforeTest() { - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) - .thenReturn(Mono.just(TRUE)); - - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) - .thenReturn(Mono.just(TRUE)); - - Mockito.when(commonGitService.fetchRemoteChanges( - any(Application.class), any(Application.class), anyString(), anyBoolean())) - .thenReturn(Mono.just(branchTrackingStatus)); - - Mockito.when(branchTrackingStatus.getBehindCount()).thenReturn(0); - - // create New Pages - NewPage newPage = createNewPage(); - - // create application - testApplication = createApplication(); - baseRepoSuffix = Paths.get(WORKSPACE_ID, DEFAULT_APP_ID, REPO_NAME); - - doReturn(Mono.just("success")) - .when(gitExecutor) - .pushApplication(baseRepoSuffix, REPO_URL, PUBLIC_KEY, PRIVATE_KEY, BRANCH_NAME); - - doReturn(Mono.just(newPage.getUnpublishedPage())) - .when(applicationPageService) - .getPage(any(NewPage.class), anyBoolean()); - - Mockito.when(newPageService.findNewPagesByApplicationId(anyString(), any(AclPermission.class))) - .thenReturn(Flux.just(newPage)); - - Mockito.when(applicationService.findByBranchNameAndDefaultApplicationId( - anyString(), anyString(), any(AclPermission.class))) - .thenReturn(Mono.just(testApplication)); - - Mockito.when(applicationService.findById(anyString(), any(AclPermission.class))) - .thenReturn(Mono.just(testApplication)); - - Mockito.when(gitPrivateRepoHelper.isBranchProtected(any(), anyString())).thenReturn(Mono.just(FALSE)); - - Mockito.when(userDataService.getGitProfileForCurrentUser(any())).thenReturn(Mono.just(createGitProfile())); - } - - @AfterEach - public void afterTest() { - gitFileSystemTestHelper.deleteWorkspaceDirectory(WORKSPACE_ID); - } - - @Test - @Disabled - public void testAutoCommit_whenOnlyServerIsEligibleForMigration_commitSuccess() - throws URISyntaxException, IOException, GitAPIException { - - ApplicationJson applicationJson = - gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); - - doReturn(Mono.just(new AutoCommitTriggerDTO(TRUE, FALSE, TRUE))) - .when(autoCommitEligibilityHelper) - .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); - - ApplicationJson applicationJson1 = new ApplicationJson(); - AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(applicationJson, applicationJson1); - applicationJson1.setServerSchemaVersion(JsonSchemaVersions.serverVersion + 1); - - doReturn(Mono.just(applicationJson1)) - .when(jsonSchemaMigration) - .migrateApplicationJsonToLatestSchema(any(ApplicationJson.class)); - - gitFileSystemTestHelper.setupGitRepository( - WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); - - // verifying the initial number of commits - StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(2); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - - // this would trigger autocommit - Mono> gitlogDTOsMono = applicationPageService - .getPagesBasedOnApplicationMode(testApplication, ApplicationMode.EDIT) - .then(Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT))) - .then(gitExecutor.getCommitHistory(baseRepoSuffix)); - - // verifying final number of commits - StepVerifier.create(gitlogDTOsMono) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(3); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - } - - @Test - @Disabled - public void testAutoCommit_whenOnlyClientIsEligibleForMigration_commitSuccess() - throws GitAPIException, IOException, URISyntaxException { - ApplicationJson applicationJson = - gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); - - int pageDSLNumber = applicationJson - .getPageList() - .get(0) - .getUnpublishedPage() - .getLayouts() - .get(0) - .getDsl() - .getAsNumber("version") - .intValue(); - - doReturn(Mono.just(new AutoCommitTriggerDTO(TRUE, TRUE, FALSE))) - .when(autoCommitEligibilityHelper) - .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); - - Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(pageDSLNumber + 1)); - - JSONObject dslAfterMigration = new JSONObject(); - dslAfterMigration.put("key", "after migration"); - - // mock the dsl migration utils to return updated dsl when requested with older dsl - Mockito.when(dslMigrationUtils.migratePageDsl(any(JSONObject.class))).thenReturn(Mono.just(dslAfterMigration)); - - gitFileSystemTestHelper.setupGitRepository( - WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); - - // verifying the initial number of commits - StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(2); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - - // this would trigger autocommit - Mono> gitlogDTOsMono = applicationPageService - .getPagesBasedOnApplicationMode(testApplication, ApplicationMode.EDIT) - .then(Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT))) - .then(gitExecutor.getCommitHistory(baseRepoSuffix)); - - // verifying final number of commits - StepVerifier.create(gitlogDTOsMono) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(3); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - } - - @Test - @Disabled - public void testAutoCommit_whenAutoCommitNotEligible_returnsFalse() - throws URISyntaxException, IOException, GitAPIException { - - ApplicationJson applicationJson = - gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); - - doReturn(Mono.just(new AutoCommitTriggerDTO(FALSE, FALSE, FALSE))) - .when(autoCommitEligibilityHelper) - .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); - - gitFileSystemTestHelper.setupGitRepository( - WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); - - // verifying the initial number of commits - StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(2); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - - // this would not trigger autocommit - Mono> gitlogDTOsMono = applicationPageService - .getPagesBasedOnApplicationMode(testApplication, ApplicationMode.EDIT) - .then(Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT))) - .then(gitExecutor.getCommitHistory(baseRepoSuffix)); - - // verifying final number of commits - StepVerifier.create(gitlogDTOsMono) - .assertNext(gitLogDTOs -> { - assertThat(gitLogDTOs).isNotEmpty(); - assertThat(gitLogDTOs.size()).isEqualTo(2); - - Set commitMessages = - gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); - assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); - }) - .verifyComplete(); - } -} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java index f046da6005..2047552234 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java @@ -14,8 +14,6 @@ import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.events.AutoCommitEvent; import com.appsmith.server.featureflags.CachedFeatures; -import com.appsmith.server.git.AutoCommitEventHandler; -import com.appsmith.server.git.AutoCommitEventHandlerImpl; import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; @@ -54,7 +52,7 @@ import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; -import static com.appsmith.server.git.AutoCommitEventHandlerCEImpl.AUTO_COMMIT_MSG_FORMAT; +import static com.appsmith.server.git.autocommit.AutoCommitEventHandlerCEImpl.AUTO_COMMIT_MSG_FORMAT; import static java.lang.Boolean.FALSE; import static java.lang.Boolean.TRUE; import static org.assertj.core.api.Assertions.assertThat; @@ -498,12 +496,12 @@ public class AutoCommitEventHandlerImplTest { autoCommitEvent.getBranchName()); CachedFeatures cachedFeatures = new CachedFeatures(); - cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_cleanup_feature_enabled.name(), FALSE)); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), FALSE)); Mockito.when(featureFlagService.getCachedTenantFeatureFlags()) .thenAnswer((Answer) invocations -> cachedFeatures); gitFileSystemTestHelper.setupGitRepository(autoCommitEvent, applicationJson); - cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_cleanup_feature_enabled.name(), TRUE)); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), TRUE)); StepVerifier.create(autoCommitEventHandler .autoCommitServerMigration(autoCommitEvent) @@ -536,7 +534,7 @@ public class AutoCommitEventHandlerImplTest { gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource("application.json")); CachedFeatures cachedFeatures = new CachedFeatures(); - cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_cleanup_feature_enabled.name(), FALSE)); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.release_git_autocommit_feature_enabled.name(), FALSE)); Mockito.when(featureFlagService.getCachedTenantFeatureFlags()) .thenAnswer((Answer) invocations -> cachedFeatures); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java new file mode 100644 index 0000000000..bdf2d4984e --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java @@ -0,0 +1,686 @@ +package com.appsmith.server.git.autocommit; + +import com.appsmith.external.dtos.GitLogDTO; +import com.appsmith.external.enums.FeatureFlagEnum; +import com.appsmith.external.git.GitExecutor; +import com.appsmith.external.helpers.AppsmithBeanUtils; +import com.appsmith.server.acl.AclPermission; +import com.appsmith.server.applications.base.ApplicationService; +import com.appsmith.server.domains.Application; +import com.appsmith.server.domains.ApplicationMode; +import com.appsmith.server.domains.ApplicationPage; +import com.appsmith.server.domains.GitArtifactMetadata; +import com.appsmith.server.domains.GitAuth; +import com.appsmith.server.domains.GitProfile; +import com.appsmith.server.domains.Layout; +import com.appsmith.server.dtos.ApplicationJson; +import com.appsmith.server.dtos.AutoCommitResponseDTO; +import com.appsmith.server.dtos.PageDTO; +import com.appsmith.server.git.autocommit.helpers.AutoCommitEligibilityHelper; +import com.appsmith.server.git.common.CommonGitService; +import com.appsmith.server.helpers.CommonGitFileUtils; +import com.appsmith.server.helpers.DSLMigrationUtils; +import com.appsmith.server.helpers.GitPrivateRepoHelper; +import com.appsmith.server.helpers.RedisUtils; +import com.appsmith.server.migrations.JsonSchemaMigration; +import com.appsmith.server.migrations.JsonSchemaVersions; +import com.appsmith.server.newpages.base.NewPageService; +import com.appsmith.server.services.FeatureFlagService; +import com.appsmith.server.services.UserDataService; +import com.appsmith.server.solutions.PagePermission; +import com.appsmith.server.testhelpers.git.GitFileSystemTestHelper; +import lombok.extern.slf4j.Slf4j; +import net.minidev.json.JSONObject; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.BranchTrackingStatus; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.test.mock.mockito.SpyBean; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.time.Duration; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import static com.appsmith.server.git.autocommit.AutoCommitEventHandlerCEImpl.AUTO_COMMIT_MSG_FORMAT; +import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; + +@ExtendWith(SpringExtension.class) +@SpringBootTest +@Slf4j +public class AutoCommitServiceTest { + + @SpyBean + AutoCommitService autoCommitService; + + @Autowired + GitFileSystemTestHelper gitFileSystemTestHelper; + + @SpyBean + GitExecutor gitExecutor; + + @MockBean + FeatureFlagService featureFlagService; + + @MockBean + DSLMigrationUtils dslMigrationUtils; + + @MockBean + ApplicationService applicationService; + + @MockBean + NewPageService newPageService; + + @Autowired + PagePermission pagePermission; + + @SpyBean + RedisUtils redisUtils; + + @MockBean + CommonGitService commonGitService; + + @SpyBean + CommonGitFileUtils commonGitFileUtils; + + @MockBean + GitPrivateRepoHelper gitPrivateRepoHelper; + + @SpyBean + AutoCommitEligibilityHelper autoCommitEligibilityHelper; + + @MockBean + BranchTrackingStatus branchTrackingStatus; + + @MockBean + UserDataService userDataService; + + @SpyBean + JsonSchemaMigration jsonSchemaMigration; + + Application testApplication; + + Path baseRepoSuffix; + + private static final Integer DSL_VERSION_NUMBER = 88; + private static final String WORKSPACE_ID = "test-workspace"; + private static final String REPO_NAME = "test-repo"; + private static final String BRANCH_NAME = "develop"; + private static final String APP_JSON_NAME = "autocommit.json"; + private static final String APP_NAME = "autocommit"; + private static final Integer WAIT_DURATION_FOR_ASYNC_EVENT = 5; + private static final String PUBLIC_KEY = "public-key"; + private static final String PRIVATE_KEY = "private-key"; + private static final String REPO_URL = "domain.xy"; + private static final String DEFAULT_APP_ID = "default-app-id", DEFAULT_BRANCH_NAME = "master"; + private static final Integer SERVER_SCHEMA_VERSION = JsonSchemaVersions.serverVersion; + + private Application createApplication() { + Application application = new Application(); + application.setName(APP_NAME); + application.setWorkspaceId(WORKSPACE_ID); + application.setId(DEFAULT_APP_ID); + + ApplicationPage applicationPage = new ApplicationPage(); + applicationPage.setId("testPageId"); + applicationPage.setIsDefault(TRUE); + + application.setPages(List.of(applicationPage)); + GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); + gitArtifactMetadata.setBranchName(BRANCH_NAME); + gitArtifactMetadata.setDefaultBranchName(DEFAULT_BRANCH_NAME); + gitArtifactMetadata.setRepoName(REPO_NAME); + gitArtifactMetadata.setDefaultApplicationId(DEFAULT_APP_ID); + gitArtifactMetadata.setRemoteUrl(REPO_URL); + + GitAuth gitAuth = new GitAuth(); + gitAuth.setPrivateKey(PRIVATE_KEY); + gitAuth.setPublicKey(PUBLIC_KEY); + gitArtifactMetadata.setGitAuth(gitAuth); + + application.setGitApplicationMetadata(gitArtifactMetadata); + return application; + } + + private GitProfile createGitProfile() { + GitProfile gitProfile = new GitProfile(); + gitProfile.setAuthorName("authorName"); + gitProfile.setAuthorEmail("author@domain.xy"); + return gitProfile; + } + + private PageDTO createPageDTO() { + JSONObject jsonObject = new JSONObject(); + jsonObject.put("key", "value"); + jsonObject.put("version", DSL_VERSION_NUMBER); + + Layout layout1 = new Layout(); + layout1.setId("testLayoutId"); + layout1.setDsl(jsonObject); + + PageDTO pageDTO = new PageDTO(); + pageDTO.setId("testPageId"); + pageDTO.setApplicationId(DEFAULT_APP_ID); + pageDTO.setLayouts(List.of(layout1)); + return pageDTO; + } + + private org.json.JSONObject getMockedDsl() { + org.json.JSONObject jsonObject = new org.json.JSONObject(); + jsonObject.put("version", DSL_VERSION_NUMBER); + return jsonObject; + } + + private void mockAutoCommitTriggerResponse(Boolean serverMigration, Boolean clientMigration) { + doReturn(Mono.just(getMockedDsl())) + .when(commonGitFileUtils) + .getPageDslVersionNumber(anyString(), any(), any(), anyBoolean(), any()); + + Integer dslVersionNumber = clientMigration ? DSL_VERSION_NUMBER + 1 : DSL_VERSION_NUMBER; + Integer serverSchemaVersionNumber = serverMigration ? SERVER_SCHEMA_VERSION - 1 : SERVER_SCHEMA_VERSION; + + doReturn(Mono.just(dslVersionNumber)).when(dslMigrationUtils).getLatestDslVersion(); + + // server as true + doReturn(Mono.just(serverSchemaVersionNumber)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion(anyString(), any(), anyBoolean(), any()); + } + + @BeforeEach + public void beforeTest() { + + // create application + testApplication = createApplication(); + baseRepoSuffix = Paths.get(WORKSPACE_ID, DEFAULT_APP_ID, REPO_NAME); + + // used for fetching application on autocommit service and gitAutoCommitHelper.autocommit + Mockito.when(applicationService.findByBranchNameAndDefaultApplicationId( + anyString(), anyString(), any(AclPermission.class))) + .thenReturn(Mono.just(testApplication)); + + // create page-dto + PageDTO pageDTO = createPageDTO(); + + Mockito.when(newPageService.findByApplicationIdAndApplicationMode( + DEFAULT_APP_ID, pagePermission.getEditPermission(), ApplicationMode.PUBLISHED)) + .thenReturn(Flux.just(pageDTO)); + + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_eligibility_enabled)) + .thenReturn(Mono.just(TRUE)); + + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) + .thenReturn(Mono.just(TRUE)); + + Mockito.when(commonGitService.fetchRemoteChanges( + any(Application.class), any(Application.class), anyString(), anyBoolean())) + .thenReturn(Mono.just(branchTrackingStatus)); + + Mockito.when(branchTrackingStatus.getBehindCount()).thenReturn(0); + + doReturn(Mono.just("success")) + .when(gitExecutor) + .pushApplication(baseRepoSuffix, REPO_URL, PUBLIC_KEY, PRIVATE_KEY, BRANCH_NAME); + + Mockito.when(applicationService.findById(anyString(), any(AclPermission.class))) + .thenReturn(Mono.just(testApplication)); + + Mockito.when(gitPrivateRepoHelper.isBranchProtected(any(), anyString())).thenReturn(Mono.just(FALSE)); + + Mockito.when(userDataService.getGitProfileForCurrentUser(any())).thenReturn(Mono.just(createGitProfile())); + } + + @AfterEach + public void afterTest() { + gitFileSystemTestHelper.deleteWorkspaceDirectory(WORKSPACE_ID); + } + + @Test + public void testAutoCommit_whenOnlyServerIsEligibleForMigration_commitSuccess() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); + + mockAutoCommitTriggerResponse(TRUE, FALSE); + + ApplicationJson applicationJson1 = new ApplicationJson(); + AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(applicationJson, applicationJson1); + applicationJson1.setServerSchemaVersion(JsonSchemaVersions.serverVersion + 1); + + doReturn(Mono.just(applicationJson1)) + .when(jsonSchemaMigration) + .migrateApplicationJsonToLatestSchema(any(ApplicationJson.class)); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + // verifying the initial number of commits + StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED); + }) + .verifyComplete(); + + // this would trigger autocommit + Mono> gitlogDTOsMono = Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT)) + .then(gitExecutor.getCommitHistory(baseRepoSuffix)); + + // verifying final number of commits + StepVerifier.create(gitlogDTOsMono) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(3); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenOnlyClientIsEligibleForMigration_commitSuccess() + throws GitAPIException, IOException, URISyntaxException { + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); + + int pageDSLNumber = applicationJson + .getPageList() + .get(0) + .getUnpublishedPage() + .getLayouts() + .get(0) + .getDsl() + .getAsNumber("version") + .intValue(); + + mockAutoCommitTriggerResponse(FALSE, TRUE); + + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(pageDSLNumber + 1)); + + JSONObject dslAfterMigration = new JSONObject(); + dslAfterMigration.put("key", "after migration"); + + // mock the dsl migration utils to return updated dsl when requested with older dsl + Mockito.when(dslMigrationUtils.migratePageDsl(any(JSONObject.class))).thenReturn(Mono.just(dslAfterMigration)); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + // verifying the initial number of commits + StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + // this would trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED); + }) + .verifyComplete(); + + Mono> gitlogDTOsMono = Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT)) + .then(gitExecutor.getCommitHistory(baseRepoSuffix)); + + // verifying final number of commits + StepVerifier.create(gitlogDTOsMono) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(3); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenAutoCommitNotEligible_returnsFalse() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); + + mockAutoCommitTriggerResponse(FALSE, FALSE); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + // verifying the initial number of commits + StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + // this would not trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.IDLE); + }) + .verifyComplete(); + + Mono> gitlogDTOsMono = Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT)) + .then(gitExecutor.getCommitHistory(baseRepoSuffix)); + + // verifying final number of commits + StepVerifier.create(gitlogDTOsMono) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenAutoCommitAlreadyInProgressOnAnotherBranch_returnsLocked() { + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)) + .thenReturn(Mono.just(DEFAULT_BRANCH_NAME)); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.just(70)); + + // this would not trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.LOCKED); + assertThat(autoCommitResponseDTO.getBranchName()).isEqualTo(DEFAULT_BRANCH_NAME); + assertThat(autoCommitResponseDTO.getProgress()).isEqualTo(70); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenAutoCommitAlreadyInProgressOnSameBranch_returnsInProgress() { + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.just(BRANCH_NAME)); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.just(70)); + + // this would not trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.IN_PROGRESS); + assertThat(autoCommitResponseDTO.getBranchName()).isEqualTo(BRANCH_NAME); + assertThat(autoCommitResponseDTO.getProgress()).isEqualTo(70); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenNoGitMetadata_returnsNonGitApp() { + testApplication.setGitApplicationMetadata(null); + // used for fetching application on autocommit service and gitAutoCommitHelper.autocommit + Mockito.when(applicationService.findByBranchNameAndDefaultApplicationId( + anyString(), anyString(), any(AclPermission.class))) + .thenReturn(Mono.just(testApplication)); + + // this would not trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.NON_GIT_APP); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenAutoCommitEligibleButPrerequisiteNotComplete_returnsRequired() { + + mockAutoCommitTriggerResponse(TRUE, FALSE); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + // number of commits behind to make the pre-req fail + Mockito.when(branchTrackingStatus.getBehindCount()).thenReturn(1); + + // this would not trigger autocommit + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.REQUIRED); + }) + .verifyComplete(); + } + + @Test + public void + testAutoCommit_whenServerIsRunningMigrationCallsAutocommitAgainOnSameBranch_ReturnsAutoCommitInProgress() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); + + mockAutoCommitTriggerResponse(TRUE, FALSE); + + ApplicationJson applicationJson1 = new ApplicationJson(); + AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(applicationJson, applicationJson1); + applicationJson1.setServerSchemaVersion(JsonSchemaVersions.serverVersion + 1); + + doReturn(Mono.just(applicationJson1)) + .when(jsonSchemaMigration) + .migrateApplicationJsonToLatestSchema(any(ApplicationJson.class)); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + // verifying the initial number of commits + StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED)) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.just(BRANCH_NAME)); + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.just(20)); + + StepVerifier.create(autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME)) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.IN_PROGRESS); + assertThat(autoCommitResponseDTO.getBranchName()).isEqualTo(BRANCH_NAME); + }) + .verifyComplete(); + + // this would trigger autocommit + Mono> gitlogDTOsMono = Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT)) + .then(gitExecutor.getCommitHistory(baseRepoSuffix)); + + // verifying final number of commits + StepVerifier.create(gitlogDTOsMono) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(3); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + } + + @Test + public void testAutoCommit_whenServerIsRunningMigrationCallsAutocommitAgainOnDiffBranch_ReturnsAutoCommitLocked() + throws URISyntaxException, IOException, GitAPIException { + + ApplicationJson applicationJson = + gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); + + mockAutoCommitTriggerResponse(TRUE, FALSE); + + ApplicationJson applicationJson1 = new ApplicationJson(); + AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(applicationJson, applicationJson1); + applicationJson1.setServerSchemaVersion(JsonSchemaVersions.serverVersion + 1); + + doReturn(Mono.just(applicationJson1)) + .when(jsonSchemaMigration) + .migrateApplicationJsonToLatestSchema(any(ApplicationJson.class)); + + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + // verifying the initial number of commits + StepVerifier.create(gitExecutor.getCommitHistory(baseRepoSuffix)) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(2); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).doesNotContain(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.empty()); + + Mono autoCommitResponseDTOMono = + autoCommitService.autoCommitApplication(testApplication.getId(), BRANCH_NAME); + + StepVerifier.create(autoCommitResponseDTOMono) + .assertNext(autoCommitResponseDTO -> assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED)) + .verifyComplete(); + + // redis-utils fixing + Mockito.when(redisUtils.getRunningAutoCommitBranchName(DEFAULT_APP_ID)).thenReturn(Mono.just(BRANCH_NAME)); + + Mockito.when(redisUtils.getAutoCommitProgress(DEFAULT_APP_ID)).thenReturn(Mono.just(20)); + + StepVerifier.create(autoCommitService.autoCommitApplication(testApplication.getId(), DEFAULT_BRANCH_NAME)) + .assertNext(autoCommitResponseDTO -> { + assertThat(autoCommitResponseDTO.getAutoCommitResponse()) + .isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.LOCKED); + assertThat(autoCommitResponseDTO.getBranchName()).isEqualTo(BRANCH_NAME); + }) + .verifyComplete(); + + // this would trigger autocommit + Mono> gitlogDTOsMono = Mono.delay(Duration.ofSeconds(WAIT_DURATION_FOR_ASYNC_EVENT)) + .then(gitExecutor.getCommitHistory(baseRepoSuffix)); + + // verifying final number of commits + StepVerifier.create(gitlogDTOsMono) + .assertNext(gitLogDTOs -> { + assertThat(gitLogDTOs).isNotEmpty(); + assertThat(gitLogDTOs.size()).isEqualTo(3); + + Set commitMessages = + gitLogDTOs.stream().map(GitLogDTO::getCommitMessage).collect(Collectors.toSet()); + assertThat(commitMessages).contains(String.format(AUTO_COMMIT_MSG_FORMAT, "UNKNOWN")); + }) + .verifyComplete(); + } +} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java index 6dc66baa55..01feec4317 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java @@ -2,7 +2,6 @@ package com.appsmith.server.git.autocommit.helpers; import com.appsmith.external.dtos.ModifiedResources; import com.appsmith.external.enums.FeatureFlagEnum; -import com.appsmith.external.git.constants.GitConstants; import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.GitArtifactMetadata; @@ -36,6 +35,7 @@ import java.io.IOException; import java.util.HashSet; import java.util.List; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.GitCommandConstantsCE.AUTO_COMMIT_ELIGIBILITY; import static org.assertj.core.api.Assertions.assertThat; @Slf4j @@ -92,18 +92,23 @@ public class AutoCommitEligibilityHelperTest { return pageDTO; } + private org.json.JSONObject getPageDSl(Integer dslVersionNumber) { + org.json.JSONObject jsonObject = new org.json.JSONObject(); + jsonObject.put("version", dslVersionNumber); + return jsonObject; + } + @BeforeEach public void beforeEach() { Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) .thenReturn(Mono.just(Boolean.TRUE)); - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_eligibility_enabled)) .thenReturn(Mono.just(Boolean.TRUE)); Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); - Mockito.when(gitRedisUtils.addFileLock( - DEFAULT_APPLICATION_ID, GitConstants.GitCommandConstants.METADATA, false)) + Mockito.when(gitRedisUtils.addFileLock(DEFAULT_APPLICATION_ID, AUTO_COMMIT_ELIGIBILITY)) .thenReturn(Mono.just(Boolean.TRUE)); Mockito.when(gitRedisUtils.releaseFileLock(DEFAULT_APPLICATION_ID)).thenReturn(Mono.just(Boolean.TRUE)); } @@ -114,11 +119,16 @@ public class AutoCommitEligibilityHelperTest { GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER - 1))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + // this leads to server migration requirement as true Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.TRUE, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); @@ -140,11 +150,16 @@ public class AutoCommitEligibilityHelperTest { GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + // this leads to server migration requirement as false Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.FALSE, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); @@ -166,11 +181,16 @@ public class AutoCommitEligibilityHelperTest { GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER - 1))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + // this leads to server migration requirement as false Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.FALSE, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); @@ -178,8 +198,9 @@ public class AutoCommitEligibilityHelperTest { StepVerifier.create(autoCommitTriggerDTOMono) .assertNext(autoCommitTriggerDTO -> { assertThat(autoCommitTriggerDTO.getIsAutoCommitRequired()).isTrue(); + // Since client is true, server is true as well assertThat(autoCommitTriggerDTO.getIsServerAutoCommitRequired()) - .isFalse(); + .isTrue(); assertThat(autoCommitTriggerDTO.getIsClientAutoCommitRequired()) .isTrue(); }) @@ -192,11 +213,16 @@ public class AutoCommitEligibilityHelperTest { GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + // this leads to server migration requirement as true Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.FALSE, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); @@ -220,7 +246,7 @@ public class AutoCommitEligibilityHelperTest { Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.TRUE, ArtifactType.APPLICATION); Mono isServerMigrationRequiredMono = autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); @@ -239,7 +265,7 @@ public class AutoCommitEligibilityHelperTest { Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) .when(commonGitFileUtils) .getMetadataServerSchemaMigrationVersion( - WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); + WORKSPACE_ID, gitArtifactMetadata, Boolean.FALSE, ArtifactType.APPLICATION); Mono isServerMigrationRequiredMono = autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); @@ -252,7 +278,7 @@ public class AutoCommitEligibilityHelperTest { @Test public void isServerMigrationRequired_whenFeatureIsFlagFalse_returnsFalse() { - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_eligibility_enabled)) .thenReturn(Mono.just(Boolean.FALSE)); GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); @@ -268,10 +294,18 @@ public class AutoCommitEligibilityHelperTest { @Test public void isClientMigrationRequired_whenLatestDslIsNotAhead_returnsFalse() { + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); - Mono isClientMigrationRequiredMono = autoCommitEligibilityHelper.isClientMigrationRequired(pageDTO); + Mono isClientMigrationRequiredMono = + autoCommitEligibilityHelper.isClientMigrationRequiredFSOps(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(isClientMigrationRequiredMono) .assertNext(isClientMigrationRequired -> @@ -281,10 +315,18 @@ public class AutoCommitEligibilityHelperTest { @Test public void isClientMigrationRequired_whenLatestDslIsAhead_returnsTrue() { + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); + + Mockito.doReturn(Mono.just(getPageDSl(RANDOM_DSL_VERSION_NUMBER - 1))) + .when(commonGitFileUtils) + .getPageDslVersionNumber( + WORKSPACE_ID, gitArtifactMetadata, pageDTO, Boolean.TRUE, ArtifactType.APPLICATION); + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); - Mono isClientMigrationRequiredMono = autoCommitEligibilityHelper.isClientMigrationRequired(pageDTO); + Mono isClientMigrationRequiredMono = + autoCommitEligibilityHelper.isClientMigrationRequiredFSOps(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(isClientMigrationRequiredMono) .assertNext(isClientMigrationRequired -> @@ -294,7 +336,7 @@ public class AutoCommitEligibilityHelperTest { @Test public void isClientMigrationRequired_whenFeatureFlagIsFalse_returnsFalse() { - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_eligibility_enabled)) .thenReturn(Mono.just(Boolean.FALSE)); PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); @@ -308,14 +350,11 @@ public class AutoCommitEligibilityHelperTest { @Test public void isAutoCommitRequired_whenFeatureIsFlagFalse_returnsAllFalse() { - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) - .thenReturn(Mono.just(Boolean.FALSE)); - - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_eligibility_enabled)) .thenReturn(Mono.just(Boolean.FALSE)); GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); - PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); Mono autoCommitTriggerDTOMono = autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java index 8da6e36beb..0c7a44f81b 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java @@ -8,9 +8,9 @@ import com.appsmith.server.domains.AutoCommitConfig; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.GitAuth; import com.appsmith.server.domains.GitProfile; -import com.appsmith.server.dtos.AutoCommitProgressDTO; +import com.appsmith.server.dtos.AutoCommitResponseDTO; import com.appsmith.server.events.AutoCommitEvent; -import com.appsmith.server.git.AutoCommitEventHandler; +import com.appsmith.server.git.autocommit.AutoCommitEventHandler; import com.appsmith.server.git.common.CommonGitService; import com.appsmith.server.helpers.GitPrivateRepoHelper; import com.appsmith.server.helpers.RedisUtils; @@ -32,6 +32,8 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IDLE; +import static com.appsmith.server.dtos.AutoCommitResponseDTO.AutoCommitResponse.IN_PROGRESS; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -241,14 +243,14 @@ public class GitAutoCommitHelperImplTest { @Test public void getAutoCommitProgress_WhenAutoCommitRunning_ReturnsValidResponse() { - Mono progressDTOMono = redisUtils + Mono progressDTOMono = redisUtils .startAutoCommit(defaultApplicationId, branchName) .then(redisUtils.setAutoCommitProgress(defaultApplicationId, 20)) - .then(gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId)); + .then(gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId, branchName)); StepVerifier.create(progressDTOMono) .assertNext(dto -> { - assertThat(dto.getIsRunning()).isTrue(); + assertThat(dto.getAutoCommitResponse()).isEqualTo(IN_PROGRESS); assertThat(dto.getProgress()).isEqualTo(20); assertThat(dto.getBranchName()).isEqualTo(branchName); }) @@ -257,15 +259,15 @@ public class GitAutoCommitHelperImplTest { @Test public void getAutoCommitProgress_WhenNoAutoCommitFinished_ReturnsValidResponse() { - Mono progressDTOMono = redisUtils + Mono progressDTOMono = redisUtils .startAutoCommit(defaultApplicationId, branchName) .then(redisUtils.setAutoCommitProgress(defaultApplicationId, 20)) .then(redisUtils.finishAutoCommit(defaultApplicationId)) - .then(gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId)); + .then(gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId, branchName)); StepVerifier.create(progressDTOMono) .assertNext(dto -> { - assertThat(dto.getIsRunning()).isFalse(); + assertThat(dto.getAutoCommitResponse()).isEqualTo(IDLE); assertThat(dto.getProgress()).isZero(); assertThat(dto.getBranchName()).isNull(); }) @@ -274,10 +276,11 @@ public class GitAutoCommitHelperImplTest { @Test public void getAutoCommitProgress_WhenNoAutoCommitRunning_ReturnsValidResponse() { - Mono progressDTOMono = gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId); + Mono progressDTOMono = + gitAutoCommitHelper.getAutoCommitProgress(defaultApplicationId, branchName); StepVerifier.create(progressDTOMono) .assertNext(dto -> { - assertThat(dto.getIsRunning()).isFalse(); + assertThat(dto.getAutoCommitResponse()).isEqualTo(IDLE); assertThat(dto.getProgress()).isZero(); assertThat(dto.getBranchName()).isNull(); }) @@ -333,7 +336,7 @@ public class GitAutoCommitHelperImplTest { application.setGitApplicationMetadata(metaData); - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) .thenReturn(Mono.just(Boolean.FALSE)); StepVerifier.create(gitAutoCommitHelper.autoCommitServerMigration(defaultApplicationId, branchName)) @@ -358,7 +361,7 @@ public class GitAutoCommitHelperImplTest { application.setGitApplicationMetadata(metaData); - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) .thenReturn(Mono.just(Boolean.TRUE)); Mockito.when(applicationService.findById(anyString(), any(AclPermission.class))) @@ -396,7 +399,7 @@ public class GitAutoCommitHelperImplTest { application.setGitApplicationMetadata(metaData); - Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) .thenReturn(Mono.just(Boolean.TRUE)); Mockito.when(applicationService.findById(anyString(), any(AclPermission.class)))