From 9f8e1e96d06eb2107db7d99378b27f3861ee7280 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Thu, 27 Jun 2024 13:47:32 +0530 Subject: [PATCH 1/7] fix: Disable custom widgets for airgapped environments (#34540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Since custom widgets rely on react delievered from `https://cdn.jsdelivr.net`, it can't work in airgapped edition. 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.Widget" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: bf3b40a5c3c51a2271347967516a53655721f7a2 > Cypress dashboard. > Tags: `@tag.Widget` ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Custom widgets will now hide specific cards if the environment is air-gapped. - **Tests** - Updated test tags for custom widgets to exclude tests in air-gapped environments. --- .../Widgets/Custom/CustomWidgetDefaultComponent_spec.ts | 3 +-- .../Widgets/Custom/CustomWidgetEditorPropertyPane_spec.ts | 2 +- app/client/src/widgets/CustomWidget/widget/index.tsx | 2 ++ 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetDefaultComponent_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetDefaultComponent_spec.ts index 4affed344f..94b7402645 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetDefaultComponent_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetDefaultComponent_spec.ts @@ -6,14 +6,13 @@ import { propPane, } from "../../../../../support/Objects/ObjectsCore"; -import { featureFlagIntercept } from "../../../../../support/Objects/FeatureFlags"; import EditorNavigation, { EntityType, } from "../../../../../support/Pages/EditorNavigation"; describe( "Custom widget Tests", - { tags: ["@tag.Widget", "@tag.Custom"] }, + { tags: ["@tag.Widget", "@tag.excludeForAirgap"] }, function () { before(() => { entityExplorer.DragDropWidgetNVerify("customwidget", 550, 100); diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetEditorPropertyPane_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetEditorPropertyPane_spec.ts index fb8c2636ce..6805ad3288 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetEditorPropertyPane_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Custom/CustomWidgetEditorPropertyPane_spec.ts @@ -7,7 +7,7 @@ import { describe( "Custom widget Tests", - { tags: ["@tag.Widget", "@tag.Custom"] }, + { tags: ["@tag.Widget", "@tag.excludeForAirgap"] }, function () { before(() => { agHelper.AddDsl("customWidget"); diff --git a/app/client/src/widgets/CustomWidget/widget/index.tsx b/app/client/src/widgets/CustomWidget/widget/index.tsx index 58f7a3320a..86657aae51 100644 --- a/app/client/src/widgets/CustomWidget/widget/index.tsx +++ b/app/client/src/widgets/CustomWidget/widget/index.tsx @@ -34,6 +34,7 @@ import { ReduxActionTypes } from "@appsmith/constants/ReduxActionConstants"; import { Colors } from "constants/Colors"; import AnalyticsUtil from "@appsmith/utils/AnalyticsUtil"; import { DynamicHeight, type WidgetFeatures } from "utils/WidgetFeatures"; +import { isAirgapped } from "@appsmith/utils/airgapHelpers"; const StyledLink = styled(Link)` display: inline-block; @@ -55,6 +56,7 @@ class CustomWidget extends BaseWidget { tags: [WIDGET_TAGS.DISPLAY], searchTags: ["external"], isSearchWildcard: true, + hideCard: isAirgapped(), }; } From 1a457591a252d2e67fec3d144c2b14b3f5261756 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Wed, 3 Jul 2024 19:11:41 +0530 Subject: [PATCH 2/7] chore: Remove strict payload parsing (#34680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverting https://github.com/appsmithorg/appsmith/pull/33724 temporarily, until we gain more confidence. [Slack conversation](https://theappsmith.slack.com/archives/C040LHZN03V/p1719997989677959). ## Summary by CodeRabbit - **Refactor** - Improved server configuration by removing unnecessary custom object mapper settings. - Optimized data source handling by cleaning up authentication object manipulations. - **New Features** - Introduced a new method for future strict type checking on the server. --------- Co-authored-by: β€œsneha122” <β€œsneha@appsmith.com”> --- app/client/src/api/DatasourcesApi.ts | 10 +--------- .../appsmith/server/configurations/CommonConfig.java | 12 ------------ 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/app/client/src/api/DatasourcesApi.ts b/app/client/src/api/DatasourcesApi.ts index 3a1b1ef7f6..1ba5810ba2 100644 --- a/app/client/src/api/DatasourcesApi.ts +++ b/app/client/src/api/DatasourcesApi.ts @@ -50,9 +50,6 @@ class DatasourcesApi extends API { datasourceConfiguration: { ...storage.datasourceConfiguration, isValid: undefined, - authentication: DatasourcesApi.cleanAuthenticationObject( - storage.datasourceConfiguration.authentication, - ), connection: storage.datasourceConfiguration.connection && { ...storage.datasourceConfiguration.connection, ssl: { @@ -70,6 +67,7 @@ class DatasourcesApi extends API { return API.post(DatasourcesApi.url, datasourceConfig); } + // Need for when we add strict type checking back on server static cleanAuthenticationObject(authentication: any): any { if (!authentication) { return undefined; @@ -144,9 +142,6 @@ class DatasourcesApi extends API { toastMessage: undefined, datasourceConfiguration: datasourceConfig.datasourceConfiguration && { ...datasourceConfig.datasourceConfiguration, - authentication: DatasourcesApi.cleanAuthenticationObject( - datasourceConfig.datasourceConfiguration.authentication, - ), connection: datasourceConfig.datasourceConfiguration.connection && { ...datasourceConfig.datasourceConfiguration.connection, ssl: { @@ -179,9 +174,6 @@ class DatasourcesApi extends API { toastMessage: undefined, datasourceConfiguration: datasourceStorage.datasourceConfiguration && { ...datasourceStorage.datasourceConfiguration, - authentication: DatasourcesApi.cleanAuthenticationObject( - datasourceStorage.datasourceConfiguration.authentication, - ), connection: datasourceStorage.datasourceConfiguration.connection && { ...datasourceStorage.datasourceConfiguration.connection, ssl: { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java index 1767baaf12..72545773fb 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java @@ -3,7 +3,6 @@ package com.appsmith.server.configurations; import com.appsmith.util.JSONPrettyPrinter; import com.appsmith.util.SerializationUtils; import com.fasterxml.jackson.core.PrettyPrinter; -import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -18,7 +17,6 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; import org.springframework.util.StringUtils; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; @@ -103,16 +101,6 @@ public class CommonConfig { return SerializationUtils.getDefaultObjectMapper(null); } - @Bean - public MappingJackson2HttpMessageConverter mappingJackson2HttpMessageConverter() { - final MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter(); - - converter.setObjectMapper(objectMapper() - .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, "CE".equals(ProjectProperties.EDITION))); - - return converter; - } - @Bean public Gson gsonInstance() { GsonBuilder gsonBuilder = new GsonBuilder(); From 1fc98c645cfaa39b8645c6ae9e9772c62e1e28ee Mon Sep 17 00:00:00 2001 From: sneha122 Date: Fri, 5 Jul 2024 15:09:39 +0530 Subject: [PATCH 3/7] fix: snowflake imports issue fixed (#34745) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR fixes the intermittent issue in snowflake, where if we import the app containing snowflake and if we open the datasource it in edit mode, password field was not there. Root cause: When we are on slow 3g and when we don’t wait for datasources to load in reconnect dialog and quickly go to application, the authentication object does not get set, which causes snowflake password conditions to break and it does not show up. We tried by simulating slow 3g on all environments and were able to reproduce it Steps to test: - We can try importing following app json and simulating slow 3g in network tab - Once app is imported, when we see reconnect modal, quickly click on go to application before the datasources are loaded in the modal [Snow issue_aparna.json](https://github.com/user-attachments/files/16107584/Snow.issue_aparna.json) 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: 9868be55568774565f9c675d8cec92b12eb55d70 > Cypress dashboard. > Tags: `@tag.Sanity` >
Fri, 05 Jul 2024 09:38:19 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **New Features** - Introduced a new comparison operation `DEFINED_AND_NOT_EQUALS` for form controls. - **Bug Fixes** - Updated the Snowflake plugin to use `DEFINED_AND_NOT_EQUALS` for better validation of authentication type configurations. - **Improvements** - Enhanced form control logic to support more precise comparison checks. Co-authored-by: β€œsneha122” <β€œsneha@appsmith.com”> --- app/client/src/components/formControls/BaseControl.tsx | 3 ++- app/client/src/components/formControls/utils.ts | 2 ++ .../snowflakePlugin/src/main/resources/form.json | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/client/src/components/formControls/BaseControl.tsx b/app/client/src/components/formControls/BaseControl.tsx index 7f2e7c1241..6bf3ad21ae 100644 --- a/app/client/src/components/formControls/BaseControl.tsx +++ b/app/client/src/components/formControls/BaseControl.tsx @@ -21,7 +21,8 @@ export type ComparisonOperations = | "IN" | "NOT_IN" | "FEATURE_FLAG" - | "VIEW_MODE"; + | "VIEW_MODE" + | "DEFINED_AND_NOT_EQUALS"; export enum ComparisonOperationsEnum { VIEW_MODE = "VIEW_MODE", diff --git a/app/client/src/components/formControls/utils.ts b/app/client/src/components/formControls/utils.ts index b5e78233df..4ec2bd0e7c 100644 --- a/app/client/src/components/formControls/utils.ts +++ b/app/client/src/components/formControls/utils.ts @@ -255,6 +255,8 @@ export const caculateIsHidden = ( case "VIEW_MODE": // This can be used to decide which form controls to show in view mode or edit mode depending on the value. return viewMode === value; + case "DEFINED_AND_NOT_EQUALS": + return !!valueAtPath && valueAtPath !== value; default: return true; } diff --git a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/form.json b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/form.json index 8053ec58f2..265cb6df3f 100644 --- a/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/form.json +++ b/app/server/appsmith-plugins/snowflakePlugin/src/main/resources/form.json @@ -95,7 +95,7 @@ }, { "path": "datasourceConfiguration.authentication.authenticationType", - "comparison": "NOT_EQUALS", + "comparison": "DEFINED_AND_NOT_EQUALS", "value": "dbAuth" } ] From c9b0462c46c0790dab01f6f4f260fa518df304b2 Mon Sep 17 00:00:00 2001 From: Jacques Ikot Date: Thu, 11 Jul 2024 10:38:53 +0100 Subject: [PATCH 4/7] revert: change value to label in onItemSelect inside SelectCell of table widget (#34872) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts appsmithorg/appsmith#34743 ## Automation /ok-to-test tags="@tag.Table" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: eec317589c3e8ec7fa68a85811f0fb4e7e632582 > Cypress dashboard. > Tags: `@tag.Table` > Spec: >
Thu, 11 Jul 2024 09:38:04 UTC ## Summary by CodeRabbit - **Refactor** - Improved test cases for select options in the table widget for better clarity and organization. - Updated `SelectCell` component to use option values instead of labels for item selection. --- .../TableV2/columnTypes/Select1_spec.ts | 40 ++----------------- .../component/cellComponents/SelectCell.tsx | 2 +- 2 files changed, 5 insertions(+), 37 deletions(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts index 000f373fe1..ba126c6e70 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts @@ -174,39 +174,7 @@ describe( cy.discardTableRow(4, 0); }); - it("6. should check that on option select uses label as value in select cell (#34743)", () => { - cy.updateCodeInput( - ".t--property-control-options", - ` - [ - { - "label": "#1label", - "value": "#1value" - }, - { - "label": "#2label", - "value": "#2value" - }, - { - "label": "#3label", - "value": "#3value" - } - ] - `, - ); - cy.editTableSelectCell(0, 0); - cy.get(".menu-item-link").contains("#3label").click(); - - _.agHelper.ValidateToastMessage("#3label"); - - cy.get(".menu-virtual-list").should("not.exist"); - cy.readTableV2data(0, 0).then((val) => { - expect(val).to.equal("#3label"); - }); - cy.discardTableRow(4, 0); - }); - - it("7. should check that currentRow is accessible in the select options", () => { + it("6. should check that currentRow is accessible in the select options", () => { cy.updateCodeInput( ".t--property-control-options", ` @@ -231,7 +199,7 @@ describe( cy.get(".menu-item-text").contains("#1").should("not.exist"); }); - it("8. should check that 'same select option in new row' property is working", () => { + it("7. should check that 'same select option in new row' property is working", () => { _.propPane.NavigateBackToPropertyPane(); const checkSameOptionsInNewRowWhileEditing = () => { @@ -297,7 +265,7 @@ describe( checkSameOptionsWhileAddingNewRow(); }); - it("9. should check that 'new row select options' is working", () => { + it("8. should check that 'new row select options' is working", () => { const checkNewRowOptions = () => { // New row select options should be visible when "Same options in new row" is turned off _.propPane.TogglePropertyState("Same options in new row", "Off"); @@ -362,7 +330,7 @@ describe( checkNoOptionState(); }); - it("10. should check that server side filering is working", () => { + it("9. should check that server side filering is working", () => { _.dataSources.CreateDataSource("Postgres"); _.dataSources.CreateQueryAfterDSSaved( "SELECT * FROM public.astronauts {{this.params.filterText ? `WHERE name LIKE '%${this.params.filterText}%'` : ''}} LIMIT 10;", diff --git a/app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx b/app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx index a0648a18a7..89c9a1fe15 100644 --- a/app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx +++ b/app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx @@ -149,7 +149,7 @@ export const SelectCell = (props: SelectProps) => { const onSelect = useCallback( (option: DropdownOption) => { onItemSelect( - option.label || "", + option.value || "", rowIndex, alias, onOptionSelectActionString, From 4916b817a7aa5bc5daa108e1944fc70777d3b91f Mon Sep 17 00:00:00 2001 From: Nilesh Sarupriya Date: Thu, 25 Jul 2024 19:47:37 +0530 Subject: [PATCH 5/7] chore: Override OAuth2AuthenticationException to differentiate the errors thrown by Appsmith (#35160) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description > Extend OAuth2AuthenticationException so that we can differentiate between AppsmithException and exceptions thrown by Spring Library. > There is not going to be any change to the Authentication flows here, as the we are just inheriting the OAuth2AuthenticationException. 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.All" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: bc2f204a6516fd527775daafb4829254d19251eb > Cypress dashboard. > Tags: `@tag.All` > Spec: >
Thu, 25 Jul 2024 13:13:00 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **New Features** - Introduced a new custom exception for improved handling of OAuth 2.0 authentication errors, enhancing the clarity and robustness of the authentication process. - **Bug Fixes** - Enhanced error categorization in the authentication process by refining the error handling logic, allowing for better management of exceptions related to OAuth 2.0. --------- Co-authored-by: Nilesh Sarupriya <20905988+nsarupr@users.noreply.github.com> --- .../ce/CustomOAuth2UserServiceCEImpl.java | 11 ++++++- .../ce/CustomOidcUserServiceCEImpl.java | 5 +++- ...AppsmithOAuth2AuthenticationException.java | 29 +++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithOAuth2AuthenticationException.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java index 4697263182..33cced2fd3 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java @@ -3,6 +3,8 @@ package com.appsmith.server.authentication.handlers.ce; import com.appsmith.server.domains.LoginSource; import com.appsmith.server.domains.User; import com.appsmith.server.domains.UserState; +import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.exceptions.AppsmithOAuth2AuthenticationException; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.services.UserService; import lombok.extern.slf4j.Slf4j; @@ -10,6 +12,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.oauth2.client.userinfo.DefaultReactiveOAuth2UserService; import org.springframework.security.oauth2.client.userinfo.OAuth2UserRequest; import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.user.OAuth2User; import reactor.core.publisher.Mono; @@ -65,6 +68,12 @@ public class CustomOAuth2UserServiceCEImpl extends DefaultReactiveOAuth2UserServ return repository.save(user); } return Mono.just(user); - }); + }) + .onErrorMap( + AppsmithException.class, + // Throwing an AppsmithOAuth2AuthenticationException in case of an AppsmithException + // This is to differentiate between Appsmith exceptions and OAuth2 exceptions + error -> new AppsmithOAuth2AuthenticationException( + new OAuth2Error(error.getAppErrorCode().toString(), error.getMessage(), ""))); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java index 5d3326036d..cbfc782c5e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java @@ -4,6 +4,7 @@ import com.appsmith.server.domains.LoginSource; import com.appsmith.server.domains.User; import com.appsmith.server.domains.UserState; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.exceptions.AppsmithOAuth2AuthenticationException; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.services.UserService; import lombok.extern.slf4j.Slf4j; @@ -76,7 +77,9 @@ public class CustomOidcUserServiceCEImpl extends OidcReactiveOAuth2UserService { }) .onErrorMap( AppsmithException.class, - error -> new OAuth2AuthenticationException( + // Throwing an AppsmithOAuth2AuthenticationException in case of an AppsmithException + // This is to differentiate between Appsmith exceptions and OAuth2 exceptions + error -> new AppsmithOAuth2AuthenticationException( new OAuth2Error(error.getAppErrorCode().toString(), error.getMessage(), ""))); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithOAuth2AuthenticationException.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithOAuth2AuthenticationException.java new file mode 100644 index 0000000000..9641864a02 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithOAuth2AuthenticationException.java @@ -0,0 +1,29 @@ +package com.appsmith.server.exceptions; + +import lombok.Getter; +import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; + +@Getter +public class AppsmithOAuth2AuthenticationException extends OAuth2AuthenticationException { + + private final OAuth2Error error; + /** + * Constructs an {@code AppsmithOAuth2AuthenticationException} using the provided parameters. + * @param error the {@link OAuth2Error OAuth 2.0 Error} + */ + public AppsmithOAuth2AuthenticationException(OAuth2Error error) { + this(error, error.getDescription(), null); + } + + /** + * Constructs an {@code AppsmithOAuth2AuthenticationException} using the provided parameters. + * @param error the {@link OAuth2Error OAuth 2.0 Error} + * @param message the detail message + * @param cause the root cause + */ + public AppsmithOAuth2AuthenticationException(OAuth2Error error, String message, Throwable cause) { + super(error, message, cause); + this.error = error; + } +} From 9f4d55997cd31f01aae081154f8e7a209bc47bef Mon Sep 17 00:00:00 2001 From: srix Date: Fri, 26 Jul 2024 12:52:49 +0530 Subject: [PATCH 6/7] fix: cleanup stale postgres postmaster.pid (#35171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description issue of Postgres not coming up with error`Β Previous Postgres was not shutdown cleanly. Please start and stop Postgres 14 properly with 'supervisorctl' only.Β ` https://www.notion.so/appsmith/Closed-Beta-Customer-issues-45a274a9eb8e4762a72cbff74cd3bad5?pvs=4#ecca04d205414f25a289884cebdc0f9b Fixes https://app.zenhub.com/workspaces/workflows-pod-652fff131a95920b9bf2bc7e/issues/zh/226_ ## Automation /ok-to-test tags="@tag.Sanity" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: 357bf5d2ff0c1f65b4cbf46b0f3ee823ac192614 > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Fri, 26 Jul 2024 07:15:19 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced PostgreSQL upgrade process with improved error handling and robust management of old server instances. - **Bug Fixes** - Reinstituted logic for checking and managing the `postmaster.pid` file, ensuring proper startup and shutdown of old PostgreSQL servers. - **Refactor** - Improved formatting and readability of shell scripts without altering their functionality. --- deploy/docker/fs/opt/appsmith/entrypoint.sh | 17 +++++++--- deploy/docker/fs/opt/appsmith/pg-upgrade.sh | 37 +++++++++++++-------- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/deploy/docker/fs/opt/appsmith/entrypoint.sh b/deploy/docker/fs/opt/appsmith/entrypoint.sh index c0338a67e4..17eaf3a72d 100644 --- a/deploy/docker/fs/opt/appsmith/entrypoint.sh +++ b/deploy/docker/fs/opt/appsmith/entrypoint.sh @@ -437,10 +437,19 @@ init_postgres() { } -safe_init_postgres(){ -runEmbeddedPostgres=1 -# fail safe to prevent entrypoint from exiting, and prevent postgres from starting -init_postgres || runEmbeddedPostgres=0 +safe_init_postgres() { + runEmbeddedPostgres=1 + # fail safe to prevent entrypoint from exiting, and prevent postgres from starting + # when runEmbeddedPostgres=0 , postgres conf file for supervisord will not be copied + # so postgres will not be started by supervisor. Explicit message helps us to know upgrade script failed. + + if init_postgres; then + tlog "init_postgres succeeded." + else + local exit_status=$? + tlog "init_postgres failed with exit status $exit_status." + runEmbeddedPostgres=0 + fi } setup_caddy() { diff --git a/deploy/docker/fs/opt/appsmith/pg-upgrade.sh b/deploy/docker/fs/opt/appsmith/pg-upgrade.sh index e646276c0c..132e7ddf7b 100644 --- a/deploy/docker/fs/opt/appsmith/pg-upgrade.sh +++ b/deploy/docker/fs/opt/appsmith/pg-upgrade.sh @@ -37,11 +37,6 @@ if [[ -z "$old_version" ]]; then exit fi -if [[ -f "$pg_data_dir/postmaster.pid" ]]; then - tlog "Previous Postgres was not shutdown cleanly. Please start and stop Postgres $old_version properly with 'supervisorctl' only." >&2 - exit 1 -fi - top_available_version="$(postgres --version | grep -o '[[:digit:]]\+' | head -1)" declare -a to_uninstall @@ -55,14 +50,30 @@ if [[ "$old_version" == 13 && "$top_available_version" > "$old_version" ]]; then to_uninstall+=("postgresql-$old_version") fi - new_version="$((old_version + 1))" - new_data_dir="$pg_data_dir-$new_version" + if [[ -f "$pg_data_dir/postmaster.pid" ]]; then + # Start old PostgreSQL using pg_ctl + tlog "Stale postmaster.pid found. Starting old PostgreSQL $old_version using pg_ctl to cleanup." + su postgres -c "$postgres_path/$old_version/bin/pg_ctl start -D '$pg_data_dir' " - # `pg_upgrade` writes log to current folder. So change to a temp folder first. - rm -rf "$TMP/pg_upgrade" "$new_data_dir" - mkdir -p "$TMP/pg_upgrade" "$new_data_dir" - chown -R postgres "$TMP/pg_upgrade" "$new_data_dir" - cd "$TMP/pg_upgrade" + # Wait for old PostgreSQL to be ready + until su postgres -c "$postgres_path/$old_version/bin/pg_isready"; do + tlog "Waiting for PostgreSQL $old_version to start..." + sleep 1 + done + + # Shut down PostgreSQL gracefully using pg_ctl + su postgres -c "$postgres_path/$old_version/bin/pg_ctl stop -D '$pg_data_dir' -m smart" + tlog "PostgreSQL $old_version has been shut down." + fi + + new_version="$((old_version + 1))" + new_data_dir="$pg_data_dir-$new_version" + + # `pg_upgrade` writes log to current folder. So change to a temp folder first. + rm -rf "$TMP/pg_upgrade" "$new_data_dir" + mkdir -p "$TMP/pg_upgrade" "$new_data_dir" + chown -R postgres "$TMP/pg_upgrade" "$new_data_dir" + cd "$TMP/pg_upgrade" # Required by the temporary Postgres server started by `pg_upgrade`. chown postgres /etc/ssl/private/ssl-cert-snakeoil.key @@ -88,7 +99,7 @@ if [[ "$old_version" == 13 && "$top_available_version" > "$old_version" ]]; then fi if [[ -n "${#to_uninstall[@]}" ]]; then - apt-get purge "${to_uninstall[@]}" + DEBIAN_FRONTEND=noninteractive apt-get purge --yes "${to_uninstall[@]}" apt-get clean fi From e7a074fd5a53bfa64d04968ea543279a0b3aa901 Mon Sep 17 00:00:00 2001 From: Diljit Date: Tue, 30 Jul 2024 08:47:21 +0530 Subject: [PATCH 7/7] chore: Add page load trace with resource, paint and navigation spans (#34957) --- .../UITelemetry/PageLoadInstrumentation.ts | 361 ++++++++++++++++++ app/client/src/UITelemetry/auto-otel-web.ts | 21 +- app/client/src/UITelemetry/generateTraces.ts | 10 +- app/client/src/index.tsx | 15 +- 4 files changed, 400 insertions(+), 7 deletions(-) create mode 100644 app/client/src/UITelemetry/PageLoadInstrumentation.ts diff --git a/app/client/src/UITelemetry/PageLoadInstrumentation.ts b/app/client/src/UITelemetry/PageLoadInstrumentation.ts new file mode 100644 index 0000000000..917baece3b --- /dev/null +++ b/app/client/src/UITelemetry/PageLoadInstrumentation.ts @@ -0,0 +1,361 @@ +import type { Span } from "@opentelemetry/api"; +import { InstrumentationBase } from "@opentelemetry/instrumentation"; +import { startRootSpan, startNestedSpan } from "./generateTraces"; +import { onLCP, onFCP } from "web-vitals/attribution"; +import type { + LCPMetricWithAttribution, + FCPMetricWithAttribution, + NavigationTimingPolyfillEntry, +} from "web-vitals"; + +export class PageLoadInstrumentation extends InstrumentationBase { + // PerformanceObserver to observe resource timings + resourceTimingObserver: PerformanceObserver | null = null; + // Root span for the page load instrumentation + rootSpan: Span; + // List of resource URLs to ignore + ignoreResourceUrls: string[] = []; + // Timestamp when the page was last hidden + pageLastHiddenAt: number = 0; + // Duration the page was hidden for + pageHiddenFor: number = 0; + // Flag to check if navigation entry was pushed + wasNavigationEntryPushed: boolean = false; + // Set to keep track of resource entries + resourceEntriesSet: Set = new Set(); + // Timeout for polling resource entries + resourceEntryPollTimeout: number | null = null; + + constructor({ ignoreResourceUrls = [] }: { ignoreResourceUrls?: string[] }) { + // Initialize the base instrumentation with the name and version + super("appsmith-page-load-instrumentation", "1.0.0", { + enabled: true, + }); + this.ignoreResourceUrls = ignoreResourceUrls; + // Start the root span for the page load + this.rootSpan = startRootSpan("PAGE_LOAD", {}, 0); + } + + init() { + // init method is present in the base class and needs to be implemented + // This is method is never called by the OpenTelemetry SDK + // Leaving it empty as it is done by other OpenTelemetry instrumentation classes + } + + enable(): void { + this.addVisibilityChangeListener(); + + // Listen for LCP and FCP events + // reportAllChanges: true will report all LCP and FCP events + // binding the context to the class to access class properties + onLCP(this.onLCPReport.bind(this), { reportAllChanges: true }); + onFCP(this.onFCPReport.bind(this), { reportAllChanges: true }); + + // Check if PerformanceObserver is available + if (PerformanceObserver) { + this.observeResourceTimings(); + } else { + // If PerformanceObserver is not available, fallback to polling + this.pollResourceTimingEntries(); + } + } + + private addVisibilityChangeListener() { + // Listen for page visibility changes to track time spent on hidden page + document.addEventListener("visibilitychange", () => { + if (document.visibilityState === "hidden") { + this.pageLastHiddenAt = performance.now(); + } else { + const endTime = performance.now(); + this.pageHiddenFor = endTime - this.pageLastHiddenAt; + } + }); + } + + // Handler for LCP report + private onLCPReport(metric: LCPMetricWithAttribution) { + const { + attribution: { lcpEntry }, + } = metric; + + if (lcpEntry) { + this.pushLcpTimingToSpan(lcpEntry); + } + } + + // Handler for FCP report + private onFCPReport(metric: FCPMetricWithAttribution) { + const { + attribution: { fcpEntry, navigationEntry }, + } = metric; + + // Push navigation entry only once + // This is to avoid pushing multiple navigation entries + if (navigationEntry && !this.wasNavigationEntryPushed) { + this.pushNavigationTimingToSpan(navigationEntry); + this.wasNavigationEntryPushed = true; + } + + if (fcpEntry) { + this.pushPaintTimingToSpan(fcpEntry); + } + } + + private getElementName(element?: Element | null, depth = 0): string { + // Limit the depth to 3 to avoid long element names + if (!element || depth > 3) { + return ""; + } + + const elementTestId = element.getAttribute("data-testid"); + const className = element.className + ? "." + element.className.split(" ").join(".") + : ""; + const elementId = element.id ? `#${element.id}` : ""; + + const elementName = `${element.tagName}${elementId}${className}:${elementTestId}`; + + // Recursively get the parent element names + const parentElementName = this.getElementName( + element.parentElement, + depth + 1, + ); + + return `${parentElementName} > ${elementName}`; + } + + // Convert kebab-case to SCREAMING_SNAKE_CASE + private kebabToScreamingSnakeCase(str: string) { + return str.replace(/-/g, "_").toUpperCase(); + } + + // Push paint timing to span + private pushPaintTimingToSpan(entry: PerformanceEntry) { + const paintSpan = startNestedSpan( + this.kebabToScreamingSnakeCase(entry.name), + this.rootSpan, + {}, + 0, + ); + + paintSpan.end(entry.startTime); + } + + // Push LCP timing to span + private pushLcpTimingToSpan(entry: LargestContentfulPaint) { + const { element, entryType, loadTime, renderTime, startTime, url } = entry; + + const lcpSpan = startNestedSpan( + this.kebabToScreamingSnakeCase(entryType), + this.rootSpan, + { + url, + renderTime, + element: this.getElementName(element), + entryType, + loadTime, + pageHiddenFor: this.pageHiddenFor, + }, + 0, + ); + + lcpSpan.end(startTime); + } + + // Push navigation timing to span + private pushNavigationTimingToSpan( + entry: PerformanceNavigationTiming | NavigationTimingPolyfillEntry, + ) { + const { + connectEnd, + connectStart, + domainLookupEnd, + domainLookupStart, + domComplete, + domContentLoadedEventEnd, + domContentLoadedEventStart, + domInteractive, + entryType, + fetchStart, + loadEventEnd, + loadEventStart, + name: url, + redirectEnd, + redirectStart, + requestStart, + responseEnd, + responseStart, + secureConnectionStart, + startTime: navigationStartTime, + type: navigationType, + unloadEventEnd, + unloadEventStart, + workerStart, + } = entry; + + this.rootSpan.setAttributes({ + connectEnd, + connectStart, + decodedBodySize: + (entry as PerformanceNavigationTiming).decodedBodySize || 0, + domComplete, + domContentLoadedEventEnd, + domContentLoadedEventStart, + domInteractive, + domainLookupEnd, + domainLookupStart, + encodedBodySize: + (entry as PerformanceNavigationTiming).encodedBodySize || 0, + entryType, + fetchStart, + initiatorType: + (entry as PerformanceNavigationTiming).initiatorType || "navigation", + loadEventEnd, + loadEventStart, + nextHopProtocol: + (entry as PerformanceNavigationTiming).nextHopProtocol || "", + redirectCount: (entry as PerformanceNavigationTiming).redirectCount || 0, + redirectEnd, + redirectStart, + requestStart, + responseEnd, + responseStart, + secureConnectionStart, + navigationStartTime, + transferSize: (entry as PerformanceNavigationTiming).transferSize || 0, + navigationType, + url, + unloadEventEnd, + unloadEventStart, + workerStart, + }); + + this.rootSpan?.end(entry.domContentLoadedEventEnd); + } + + // Observe resource timings using PerformanceObserver + private observeResourceTimings() { + this.resourceTimingObserver = new PerformanceObserver((list) => { + const entries = list.getEntries() as PerformanceResourceTiming[]; + const resources = this.getResourcesToTrack(entries); + resources.forEach((entry) => { + this.pushResourceTimingToSpan(entry); + }); + }); + + this.resourceTimingObserver.observe({ + type: "resource", + buffered: true, + }); + } + + // Filter out resources to track based on ignoreResourceUrls + private getResourcesToTrack(resources: PerformanceResourceTiming[]) { + return resources.filter(({ name }) => { + return !this.ignoreResourceUrls.some((ignoreUrl) => + name.includes(ignoreUrl), + ); + }); + } + + // Push resource timing to span + private pushResourceTimingToSpan(entry: PerformanceResourceTiming) { + const { + connectEnd, + connectStart, + decodedBodySize, + domainLookupEnd, + domainLookupStart, + duration: resourceDuration, + encodedBodySize, + entryType, + fetchStart, + initiatorType, + name: url, + nextHopProtocol, + redirectEnd, + redirectStart, + requestStart, + responseEnd, + responseStart, + secureConnectionStart, + transferSize, + workerStart, + } = entry; + + const resourceSpan = startNestedSpan( + entry.name, + this.rootSpan, + { + connectEnd, + connectStart, + decodedBodySize, + domainLookupEnd, + domainLookupStart, + encodedBodySize, + entryType, + fetchStart, + firstInterimResponseStart: (entry as any).firstInterimResponseStart, + initiatorType, + nextHopProtocol, + redirectEnd, + redirectStart, + requestStart, + responseEnd, + responseStart, + resourceDuration, + secureConnectionStart, + transferSize, + url, + workerStart, + renderBlockingStatus: (entry as any).renderBlockingStatus, + }, + entry.startTime, + ); + + resourceSpan.end(entry.startTime + entry.responseEnd); + } + + // Get unique key for a resource entry + private getResourceEntryKey(entry: PerformanceResourceTiming) { + return `${entry.name}:${entry.startTime}:${entry.entryType}`; + } + + // Poll resource timing entries periodically + private pollResourceTimingEntries() { + // Clear the previous timeout + if (this.resourceEntryPollTimeout) { + clearInterval(this.resourceEntryPollTimeout); + } + + const resources = performance.getEntriesByType( + "resource", + ) as PerformanceResourceTiming[]; + + const filteredResources = this.getResourcesToTrack(resources); + + filteredResources.forEach((entry) => { + const key = this.getResourceEntryKey(entry); + if (!this.resourceEntriesSet.has(key)) { + this.pushResourceTimingToSpan(entry); + this.resourceEntriesSet.add(key); + } + }); + + // Poll every 5 seconds + this.resourceEntryPollTimeout = setTimeout( + this.pollResourceTimingEntries, + 5000, + ); + } + + disable(): void { + if (this.resourceTimingObserver) { + this.resourceTimingObserver.disconnect(); + } + + if (this.rootSpan) { + this.rootSpan.end(); + } + } +} diff --git a/app/client/src/UITelemetry/auto-otel-web.ts b/app/client/src/UITelemetry/auto-otel-web.ts index 389e5e04fd..fca494e321 100644 --- a/app/client/src/UITelemetry/auto-otel-web.ts +++ b/app/client/src/UITelemetry/auto-otel-web.ts @@ -20,14 +20,21 @@ import { } from "@opentelemetry/exporter-metrics-otlp-http"; import type { Context, TextMapSetter } from "@opentelemetry/api"; import { metrics } from "@opentelemetry/api"; +import { registerInstrumentations } from "@opentelemetry/instrumentation"; +import { PageLoadInstrumentation } from "./PageLoadInstrumentation"; enum CompressionAlgorithm { NONE = "none", GZIP = "gzip", } const { newRelic } = getAppsmithConfigs(); -const { applicationId, otlpEndpoint, otlpLicenseKey, otlpServiceName } = - newRelic; +const { + applicationId, + browserAgentEndpoint, + otlpEndpoint, + otlpLicenseKey, + otlpServiceName, +} = newRelic; const tracerProvider = new WebTracerProvider({ resource: new Resource({ @@ -112,3 +119,13 @@ const meterProvider = new MeterProvider({ // Register the MeterProvider globally metrics.setGlobalMeterProvider(meterProvider); + +registerInstrumentations({ + tracerProvider, + meterProvider, + instrumentations: [ + new PageLoadInstrumentation({ + ignoreResourceUrls: [browserAgentEndpoint, otlpEndpoint], + }), + ], +}); diff --git a/app/client/src/UITelemetry/generateTraces.ts b/app/client/src/UITelemetry/generateTraces.ts index 776d132aec..1f26128489 100644 --- a/app/client/src/UITelemetry/generateTraces.ts +++ b/app/client/src/UITelemetry/generateTraces.ts @@ -7,10 +7,10 @@ import type { import { SpanKind } from "@opentelemetry/api"; import { context } from "@opentelemetry/api"; import { trace } from "@opentelemetry/api"; -import { deviceType } from "react-device-detect"; - +import { deviceType, browserName, browserVersion } from "react-device-detect"; import { APP_MODE } from "entities/App"; import { matchBuilderPath, matchViewerPath } from "constants/routes"; +import nanoid from "nanoid"; import memoizeOne from "memoize-one"; const GENERATOR_TRACE = "generator-tracer"; @@ -18,6 +18,8 @@ const GENERATOR_TRACE = "generator-tracer"; export type OtlpSpan = Span; export type SpanAttributes = Attributes; +const OTLP_SESSION_ID = nanoid(); + const getAppMode = memoizeOne((pathname: string) => { const isEditorUrl = matchBuilderPath(pathname); const isViewerUrl = matchViewerPath(pathname); @@ -29,6 +31,7 @@ const getAppMode = memoizeOne((pathname: string) => { : ""; return appMode; }); + const getCommonTelemetryAttributes = () => { const pathname = window.location.pathname; const appMode = getAppMode(pathname); @@ -36,6 +39,9 @@ const getCommonTelemetryAttributes = () => { return { appMode, deviceType, + browserName, + browserVersion, + otlpSessionId: OTLP_SESSION_ID, }; }; diff --git a/app/client/src/index.tsx b/app/client/src/index.tsx index 30891d5c3f..28cc0dbbed 100755 --- a/app/client/src/index.tsx +++ b/app/client/src/index.tsx @@ -24,7 +24,9 @@ import { setAutoFreeze } from "immer"; import AppErrorBoundary from "./AppErrorBoundry"; import log from "loglevel"; import { getAppsmithConfigs } from "@appsmith/configs"; -import { BrowserAgent } from "@newrelic/browser-agent/loaders/browser-agent"; +import { PageViewTiming } from "@newrelic/browser-agent/features/page_view_timing"; +import { PageViewEvent } from "@newrelic/browser-agent/features/page_view_event"; +import { Agent } from "@newrelic/browser-agent/loaders/agent"; const { newRelic } = getAppsmithConfigs(); const { enableNewRelic } = newRelic; @@ -33,7 +35,6 @@ const newRelicBrowserAgentConfig = { init: { distributed_tracing: { enabled: true }, privacy: { cookies_enabled: true }, - ajax: { deny_list: [newRelic.browserAgentEndpoint] }, }, info: { beacon: newRelic.browserAgentEndpoint, @@ -53,7 +54,15 @@ const newRelicBrowserAgentConfig = { // The agent loader code executes immediately on instantiation. if (enableNewRelic) { - new BrowserAgent(newRelicBrowserAgentConfig); + new Agent( + { + ...newRelicBrowserAgentConfig, + features: [PageViewTiming, PageViewEvent], + }, + // The second argument agentIdentifier is not marked as optional in its type definition. + // Passing a null value throws an error as well. So we pass undefined. + undefined, + ); } const shouldAutoFreeze = process.env.NODE_ENV === "development";