chore: Google sheet shared drive support added behind a flag #37776
Conversation
WalkthroughThe changes in this pull request enhance the Google Sheets integration by introducing feature flag support for shared drive access. Key modifications include renaming and updating methods in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12045677715. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (1)
42-45: Consider pagination for large result setsWith the expanded search scope across all drives, the API might return a large number of spreadsheets. Consider implementing pagination to handle large result sets efficiently.
Consider adding a pageSize parameter and handling nextPageToken in the response:
UriComponentsBuilder uriBuilder = getBaseUriBuilder( this.BASE_DRIVE_API_URL, - "?includeItemsFromAllDrives=true&supportsAllDrives=true&corpora=allDrives&orderBy=name&q=mimeType%3D'application%2Fvnd.google-apps.spreadsheet'%20and%20trashed%3Dfalse", + "?includeItemsFromAllDrives=true&supportsAllDrives=true&corpora=allDrives&orderBy=name&pageSize=100&q=mimeType%3D'application%2Fvnd.google-apps.spreadsheet'%20and%20trashed%3Dfalse", true);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java(1 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json(1 hunks)
🔇 Additional comments (2)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json (1)
41-52: Well-structured permission levels with clear labeling.
The new permission options follow a clear progression from most restrictive to least restrictive access, with descriptive labels that help users make informed decisions.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (1)
43-43: Verify permissions and performance impact of all-drives search
The addition of includeItemsFromAllDrives=true&supportsAllDrives=true&corpora=allDrives significantly expands the search scope. This change:
- May require additional Google Workspace API permissions
- Could impact response times when dealing with large organizations
- Might return more results than before
✅ Verification successful
Change is safe with existing permission model and test coverage
The expanded search scope is already covered by the existing permission model:
- The form.json shows two permission levels: file-specific and all-sheets access
- The test suite demonstrates proper handling of both permission scopes through
userAuthorizedSheetIds - Response transformation is well-tested for both all-sheets and specific-sheets scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the permissions are properly documented in form.json
rg -A 10 "scope" app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json
# Check if there are any related performance tests
rg -l "FileListMethod" app/server/appsmith-plugins/googleSheetsPlugin/src/test/java
Length of output: 802
Script:
#!/bin/bash
# Check the test file to understand the test coverage for all-drives functionality
cat app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/FileListMethodTest.java
# Check if there are any existing performance-related configurations or timeouts
rg -A 3 "timeout|performance" app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java
Length of output: 10187
app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json
Outdated
Show resolved
Hide resolved
Failed server tests
|
github-actions
bot
added
Bug
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12052747236. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
17-17: Add documentation for the feature flag.Consider adding a brief comment explaining the flag's purpose and its temporary nature. This will help with future maintenance.
+ // Controls access to shared drive functionality in Google Sheets integration + // Note: Temporary flag for testing - can be removed once feature is fully tested release_google_sheets_shared_drive_support_enabled,app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/TriggerMethod.java (1)
25-27: Consider documenting the default null returnThe default implementation returning null is a good backward compatibility approach, but it would be helpful to document why it returns null.
+ /** + * Default implementation returns null. Implementations should override this + * or use getTriggerClientWithFlags for feature flag support. + */ default WebClient.RequestHeadersSpec<?> getTriggerClient(WebClient webClient, MethodConfig methodConfig) { return null; }app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (1)
42-51: Consider defining feature flag key as a constantWhile the implementation is solid, consider extracting the feature flag key to a constant to prevent typos and enable easier refactoring.
+ private static final String SHARED_DRIVE_FEATURE_FLAG = + FeatureFlagEnum.release_google_sheets_shared_drive_support_enabled.toString(); Boolean isSharedDriveSupportEnabled = featureFlagMap.getOrDefault( - FeatureFlagEnum.release_google_sheets_shared_drive_support_enabled.toString(), + SHARED_DRIVE_FEATURE_FLAG, Boolean.FALSE);app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/ExecutionMethod.java (1)
108-111: Consider adding documentation for feature flagsThe new method provides good backward compatibility, but lacks documentation about expected feature flags and their effects.
Add Javadoc explaining:
- Available feature flags
- Their impact on execution
- Example usage
+ /** + * Executes the client request with feature flag support. + * @param featureFlagMap Map of feature flags including: + * - release_google_sheets_shared_drive_support_enabled + * @return RequestHeadersSpec for the execution + */ default WebClient.RequestHeadersSpec<?> getExecutionClientWithFlags( WebClient webClient, MethodConfig methodConfig, Map<String, Boolean> featureFlagMap) { return getExecutionClient(webClient, methodConfig); }app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
109-111: Track technical debt removalThe comment indicates this is temporary, but there's no tracking mechanism.
Consider adding a TODO with a JIRA/issue reference for removing this feature flag once testing is complete.
- // Flags are needed here for google sheets integration to support shared drive behind a flag - // Once thoroughly tested, this flag can be removed + // TODO(ISSUE-XXX): Remove feature flag for Google Sheets shared drive support once testing is completeapp/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (1)
Line range hint
1-391: Add test cases for feature flag scenariosGiven that this PR introduces feature flag support for shared drive functionality, we should add test cases to verify the behavior when the feature flag is enabled/disabled.
Consider adding tests like:
@Test public void testExecuteAction_whenSharedDriveFeatureFlagEnabled_shouldAllowAccess() { // Setup doReturn(true) .when(featureFlagService) .isFeatureEnabled(FieldName.GOOGLE_SHEETS_SHARED_DRIVE_SUPPORT); // Test implementation } @Test public void testExecuteAction_whenSharedDriveFeatureFlagDisabled_shouldRestrictAccess() { // Setup doReturn(false) .when(featureFlagService) .isFeatureEnabled(FieldName.GOOGLE_SHEETS_SHARED_DRIVE_SUPPORT); // Test implementation }app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (4)
250-261: Fix typo in documentationThere's a typo in the word "thorogh" which should be "thorough".
-// Once thorogh testing of shared drive support is done, we can remove this tech debt of passing feature flags like +// Once thorough testing of shared drive support is done, we can remove this tech debt of passing feature flags like
262-269: Consider adding null check for featureFlagMapThe method should handle cases where featureFlagMap is null to prevent potential NullPointerException in implementing classes.
default Mono<ActionExecutionResult> executeParameterizedWithFlags( C connection, ExecuteActionDTO executeActionDTO, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration, Map<String, Boolean> featureFlagMap) { + if (featureFlagMap == null) { + featureFlagMap = Map.of(); + } return this.executeParameterized(connection, executeActionDTO, datasourceConfiguration, actionConfiguration); }
285-291: Consider adding null check for featureFlagMapSimilar to executeParameterizedWithFlags, add null safety for the feature flag map.
default Mono<TriggerResultDTO> triggerWithFlags( C connection, DatasourceConfiguration datasourceConfiguration, TriggerRequestDTO request, Map<String, Boolean> featureFlagMap) { + if (featureFlagMap == null) { + featureFlagMap = Map.of(); + } return this.trigger(connection, datasourceConfiguration, request); }
250-291: Technical Debt: Consider timeline for refactoringWhile the current implementation is a valid temporary solution, consider creating a timeline for moving the feature flag service to the shared interface module to prevent this pattern from becoming permanent.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (1)
83-84: Consider handling potential exceptions when fetching feature flagsWhen retrieving feature flags using
featureFlagService.getCachedTenantFeatureFlags().getFeatures(), ensure that the service handles any possible exceptions or null values to prevent runtime errors.app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (3)
79-84: Add@OverrideAnnotation toexecuteParameterizedWithFlagsMethodSince
executeParameterizedWithFlagsoverrides a method from thePluginExecutorinterface, it's good practice to annotate it with@Overrideto enhance readability and catch potential errors at compile time.Apply this diff to add the
@Overrideannotation:+ @Override public Mono<ActionExecutionResult> executeParameterizedWithFlags( Void connection, ExecuteActionDTO executeActionDTO, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration, Map<String, Boolean> featureFlagMap) {
143-144: Add@OverrideAnnotation toexecuteCommonMethodIf
executeCommonoverrides a method from a superclass or interface, consider adding the@Overrideannotation for better clarity and error checking.Apply this diff if applicable:
+ @Override public Mono<ActionExecutionResult> executeCommon( Void connection, DatasourceConfiguration datasourceConfiguration, ActionConfiguration actionConfiguration, Map<String, Boolean> featureFlagMap) {
324-328: Add@OverrideAnnotation totriggerWithFlagsMethodSince
triggerWithFlagslikely overrides a method from thePluginExecutorinterface, adding the@Overrideannotation improves code clarity and helps prevent mistakes.Apply this diff to include the
@Overrideannotation:+ @Override public Mono<TriggerResultDTO> triggerWithFlags( Void connection, DatasourceConfiguration datasourceConfiguration, TriggerRequestDTO request, Map<String, Boolean> featureFlagMap) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(1 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/ExecutionMethod.java(2 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java(4 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/TriggerMethod.java(2 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/DatasourceTriggerSolutionImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java(3 hunks)
🔇 Additional comments (23)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
17-17: LGTM! Feature flag follows naming conventions and is correctly placed.
The flag is appropriately positioned in the product features section and follows the established naming pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionImpl.java (2)
6-6: LGTM! Clean implementation of feature flag service integration.
The addition of FeatureFlagService follows proper dependency injection patterns and maintains consistent code style.
Also applies to: 18-26
20-26: Verify feature flag handling in parent class.
Let's confirm the parent class properly handles the feature flag service for Google Sheets shared drive support.
✅ Verification successful
Let me search for more specific information about the feature flag usage in the parent class.
Let me check one more thing to verify how the feature flags are actually used in the Google Sheets plugin.
Feature flag handling is properly implemented
The parent class PluginTriggerSolutionCEImpl correctly injects and stores the FeatureFlagService. The Google Sheets shared drive feature flag is defined in FeatureFlagEnum and is used appropriately in FileListMethod. The feature flag service is accessible through the inheritance chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check parent class implementation for feature flag handling
# Expected: Find feature flag usage in parent class related to Google Sheets
# Search for the parent class implementation
ast-grep --pattern 'class PluginTriggerSolutionCEImpl {
$$$
}'
# Search for Google Sheets related feature flag usage
rg -A 5 "release_google_sheets_shared_drive_support_enabled"
Length of output: 1941
Script:
#!/bin/bash
# Search for the parent class implementation and its constructor
ast-grep --pattern 'class PluginTriggerSolutionCEImpl {
$$$
PluginTriggerSolutionCEImpl($$$) {
$$$
}
$$$
}'
# Search for methods in parent class that might use feature flags
rg -A 10 "featureFlagService" app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java
Length of output: 2120
Script:
#!/bin/bash
# Search for the complete implementation of FileListMethod to understand feature flag usage
ast-grep --pattern 'class FileListMethod {
$$$
}'
# Search for any other Google Sheets related feature flag usage
rg -A 5 "googleSheetsPlugin.*FeatureFlagService"
Length of output: 104
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/TriggerMethod.java (2)
6-7: LGTM: Clean import additions
The new imports for Map and Set are necessary for the feature flag implementation.
29-35: LGTM: Well-designed feature flag integration
The new method follows good interface evolution practices:
- Maintains backward compatibility
- Provides sensible default implementation
- Well-documented
- Properly typed parameters
Let's verify if all implementations properly override this method:
✅ Verification successful
The previous ast-grep pattern might not have matched due to syntax variations. Let's try using ripgrep to find the implementations of the TriggerMethod interface.
Let's verify if any of the implementations override the default method. This will help us confirm if the interface evolution is being properly handled.
Default implementation is sufficient, no overrides needed
The interface provides a default implementation that delegates to the existing getTriggerClient method, making it backward compatible. Since none of the implementing classes (RowsGetMethod, FileListMethod, GetStructureMethod, FileInfoMethod) override this method, they correctly inherit the default behavior. This is the intended design pattern for interface evolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of TriggerMethod to ensure they're updated
ast-grep --pattern 'class $_ implements $$$TriggerMethod$$$'
Length of output: 62
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find the interface file
fd "TriggerMethod.java" --type f
# Then use ripgrep to find implementations
rg "implements.*TriggerMethod" --type java -l
Length of output: 587
Script:
#!/bin/bash
# Let's check if any of these implementations override the new method
rg "getTriggerClientWithFlags.*WebClient.*MethodConfig.*featureFlagMap" --type java -A 2 -B 2
Length of output: 95
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/DatasourceTriggerSolutionImpl.java (2)
10-10: LGTM! Clean implementation of feature flag service injection.
The addition of FeatureFlagService follows Spring DI best practices and maintains consistency with existing code structure.
Also applies to: 32-33, 45-46
Line range hint 32-46: Verify feature flag handling in parent class.
Let's ensure the parent class properly utilizes the injected feature flag service.
✅ Verification successful
Let me search for the parent class implementation to verify feature flag usage.
Let me try a different search pattern to find the feature flag usage.
Feature flag service is properly utilized in the parent class
The parent class DatasourceTriggerSolutionCEImpl correctly uses the feature flag service to get tenant features and pass them to the plugin executor during trigger execution. The implementation follows the same pattern as other solution classes like ActionExecutionSolutionCEImpl.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for feature flag usage in the parent class
ast-grep --pattern 'class DatasourceTriggerSolutionCEImpl {
$$$
featureFlagService.$$$
$$$
}'
# Backup: Search using ripgrep if parent class is not in the current state
rg -A 5 "featureFlagService" --type java
Length of output: 63274
Script:
#!/bin/bash
# Search for DatasourceTriggerSolutionCEImpl class implementation
ast-grep --pattern 'class DatasourceTriggerSolutionCEImpl {
$$$
featureFlagService.$$$
$$$
}'
# Search for specific feature flag usage in the parent class
rg -A 5 "DatasourceTriggerSolutionCEImpl.*featureFlagService" --type java
Length of output: 177
Script:
#!/bin/bash
# Search for the parent class implementation and feature flag usage
rg -l "class DatasourceTriggerSolutionCEImpl" --type java | xargs rg -A 10 "featureFlagService"
# Backup: Search for any feature flag usage in datasource trigger related files
rg -A 5 "getCachedTenantFeatureFlags|getTenantFeatures" --type java app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/
Length of output: 3804
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java (2)
16-16: LGTM! Clean implementation of feature flag service.
The addition of FeatureFlagService follows proper dependency injection patterns and maintains consistent parameter ordering.
Also applies to: 46-47, 68-69
46-47: Verify ActionExecutionSolutionImpl instantiation.
Since we've added a new required constructor parameter, we should verify all instantiations of this class are updated.
✅ Verification successful
Constructor change is safe - Spring handles dependency injection
The class is annotated with @Service and is component-scanned via the root @ComponentScan({"com.appsmith"}) configuration. Spring's dependency injection will automatically handle the new constructor parameter, making this change safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ActionExecutionSolutionImpl instantiations
rg -l "new ActionExecutionSolutionImpl\(" | xargs rg -A 10 "new ActionExecutionSolutionImpl\("
# Search for Spring @Autowired usages
ast-grep --pattern 'class $_ {
$$$
@Autowired
$$$ActionExecutionSolutionImpl$$$
$$$
}'
Length of output: 193
Script:
#!/bin/bash
# Search for ActionExecutionSolutionImpl instantiations with more context
rg "ActionExecutionSolutionImpl" -B 2 -A 2
# Search for Spring configuration files that might wire this bean
fd "configuration" -e java -e xml | xargs rg "ActionExecutionSolutionImpl"
# Search for Spring component scan configurations
fd -e java | xargs rg "@ComponentScan|@Configuration.*ActionExecutionSolution"
# Search for any test configurations
fd "test" -e java | xargs rg "ActionExecutionSolutionImpl"
Length of output: 1351
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (2)
29-30: LGTM: Well-defined constant for shared drive parameters
The constant effectively encapsulates the query parameters required for shared drive support.
85-87: LGTM: Clean delegation pattern
The trigger client correctly delegates to the execution client, maintaining consistency in feature flag handling.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
20-20: LGTM: Clean dependency injection setup
The FeatureFlagService integration follows proper dependency injection patterns.
Also applies to: 57-57
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (2)
27-27: LGTM: FeatureFlagService integration
The addition of FeatureFlagService as a SpyBean is correctly implemented, allowing for proper testing of feature flag behavior.
Also applies to: 140-142
174-175: LGTM: Constructor dependency injection
The featureFlagService is properly injected into the constructor, maintaining consistent ordering with other dependencies.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (1)
271-283: LGTM!
The implementation correctly combines metrics observation with feature flags while maintaining the existing metrics pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (2)
39-40: Constructor updated with FeatureFlagService injection
95-96: Verify that PluginExecutor implementations handle null parameters
Passing null for datasourceContext and datasourceConfiguration in triggerWithFlags is acceptable here. Please confirm that all PluginExecutor implementations can handle null values without issues.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (3)
137-137: Code Change Looks Good
Passing featureFlagMap to executeCommon ensures that feature flags are consistently handled throughout the execution flow.
190-190: Ensure Implementation of getExecutionClientWithFlags
Verify that getExecutionClientWithFlags is properly implemented to handle feature flags as intended. This is crucial for the correct functioning of feature flag-dependent logic.
351-351: Validate getTriggerClientWithFlags Method
Ensure that getTriggerClientWithFlags correctly incorporates feature flags and that its behavior aligns with the expected functionality, especially regarding shared drive support.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (3)
48-48: Properly imported and declared FeatureFlagService.
The FeatureFlagService is correctly imported and declared as a final class member for dependency injection.
Also applies to: 126-126
Line range hint 155-177: Constructor updated to include FeatureFlagService.
The constructor parameters and assignment correctly include the new FeatureFlagService dependency.
Line range hint 733-749: Ensure all PluginExecutor implementations support the new method.
Since the new method executeParameterizedWithMetricsAndFlags is introduced, all implementations of the PluginExecutor interface need to be updated to include this method to prevent potential runtime issues.
Run the following script to verify that all implementations of PluginExecutor have the new method:
...er/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java
Outdated
Show resolved
Hide resolved
...r/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/ExecutionMethod.java
Show resolved
Hide resolved
...h-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java
Show resolved
Hide resolved
...h-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java
Outdated
Show resolved
Hide resolved
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12063169922. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
110-114: Consider using Optional for cleaner null handlingThe null check could be more elegant using Optional.
- Map<String, Boolean> featureFlagMap = featureFlagService.getCachedTenantFeatureFlags() != null - ? featureFlagService.getCachedTenantFeatureFlags().getFeatures() - : Collections.emptyMap(); + Map<String, Boolean> featureFlagMap = Optional.ofNullable(featureFlagService.getCachedTenantFeatureFlags()) + .map(flags -> flags.getFeatures()) + .orElse(Collections.emptyMap());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(4 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (3)
20-20: LGTM: Clean dependency injection
The FeatureFlagService integration follows proper dependency injection patterns.
Also applies to: 58-58
110-114: Consider using reactive feature flag retrieval
The synchronous getCachedTenantFeatureFlags() call could impact performance in this reactive flow.
123-127: Add error handling for feature flags
The feature flag integration still lacks error handling.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (2)
48-48: LGTM: Clean dependency injection implementation
The FeatureFlagService integration follows the standard dependency injection pattern.
Also applies to: 127-127, 156-157, 178-178
734-747: Track technical debt: Feature flag for Google Sheets shared drive
The comment indicates this is a temporary solution. Let's ensure this technical debt is tracked.
Let's verify the PluginExecutor interface changes:
✅ Verification successful
Feature flag integration is properly implemented
The code follows the correct pattern for feature flag integration:
- The
executeParameterizedWithMetricsAndFlagsmethod is properly defined in thePluginExecutorinterface - The Google Sheets plugin correctly implements the interface and handles the feature flags parameter
- The implementation includes proper null checks and fallback to empty map
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if executeParameterizedWithMetricsAndFlags is properly defined in the interface
ast-grep --pattern 'interface PluginExecutor {
$$$
executeParameterizedWithMetricsAndFlags($$$) {
$$$
}
$$$
}'
Length of output: 124
Script:
#!/bin/bash
# Let's search for the PluginExecutor interface definition
rg -t java "interface PluginExecutor" -A 10
# Also search for executeParameterizedWithMetricsAndFlags method references
rg -t java "executeParameterizedWithMetricsAndFlags"
Length of output: 2483
Script:
#!/bin/bash
# Let's check the implementation details of executeParameterizedWithMetricsAndFlags
rg -t java -A 15 "default Mono<ActionExecutionResult> executeParameterizedWithMetricsAndFlags"
# Also check for any Google Sheets specific implementations
rg -t java "class GoogleSheetsPlugin" -A 20
Length of output: 7680
Script:
#!/bin/bash
# Let's check the implementation of executeParameterizedWithFlags in GoogleSheetsPlugin
rg -t java -A 10 "executeParameterizedWithFlags.*GoogleSheetsPlugin"
# Also check for any shared drive related code
rg -t java "sharedDrive" -B 2 -A 2
Length of output: 105
|
Deploy-Preview-URL: https://ce-37776.dp.appsmith.com |
Failed server tests
|
...er/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/TriggerMethod.java
Show resolved
Hide resolved
...psmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java
Show resolved
Hide resolved
...-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java (6)
330-331: Consolidate mock configurationsBoth
executeParameterizedWithMetricsandexecuteParameterizedWithMetricsAndFlagsare mocked to returnmockResult. If only one of these methods is actually used in this test, consider removing the unnecessary mock to keep the test clean.
532-533: Remove redundant exception mocksYou're mocking both
executeParameterizedWithMetricsandexecuteParameterizedWithMetricsAndFlagsto throwpluginException. If the old method is deprecated or unused here, removing its mock can enhance readability.
591-592: Avoid duplicating mocksMocking both methods to throw the same
pluginExceptionmay be unnecessary. Verify if both mocks are required for this test case, and eliminate any redundant ones.
644-646: Simplify mocked exceptionsBoth methods are set up to throw
StaleConnectionException(). If the test only requires one method to simulate this exception, consider removing the extra mock for clarity.
698-699: Streamline mock delaysIn this timeout test, both methods are mocked to introduce a delay. If only one method is under test, it might be clearer to mock only that method.
730-732: Clarify exception handling in mocksBoth
executeParameterizedWithMetricsandexecuteParameterizedWithMetricsAndFlagsare throwingStaleConnectionException(). Confirm if it's necessary to mock exceptions for both methods in this test, and remove any that are not needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java(6 hunks)
Can I add test cases for this in a separate PR? As this is a release blocker I would like to get this merged ASAP, WDYT? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (1)
44-51: Add tracking issue reference to TODO commentThe implementation looks good, but the TODO comment should reference a tracking issue for future cleanup.
- // TODO: Flags are needed here for google sheets integration to support shared drive behind a flag - // Once thoroughly tested, this flag can be removed + // TODO: #ISSUE_NUMBER Remove feature flag once shared drive support is thoroughly testedapp/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (2)
83-84: Consider caching feature flags at instance levelThe feature flags are retrieved on every trigger call. Consider caching the flags at the instance level and implementing a refresh mechanism to improve performance.
95-96: LGTM: Consider enhancing type safetyThe implementation is correct, but consider using a generic type parameter for better type safety instead of casting to
PluginExecutor<Object>.-((PluginExecutor<Object>) pluginExecutor) +pluginExecutorapp/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
110-111: Track technical debt: TODO comment needs follow-upThe TODO comment indicates this is temporary. Consider creating a tracking issue for removing the feature flag once testing is complete.
Would you like me to create a GitHub issue to track the removal of this feature flag?
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (2)
263-270: Add JavaDoc for the new method.Please add proper JavaDoc documentation including:
+ /** + * Executes an action with feature flags support. + * @since 1.0.0 + * @param connection Connection object + * @param executeActionDTO Action execution data + * @param datasourceConfiguration Datasource configuration + * @param actionConfiguration Action configuration + * @param featureFlagMap Map of feature flag names to their enabled status + * @return Action execution result + */
286-292: Add JavaDoc for consistency.Please add JavaDoc documentation to maintain consistency with other interface methods.
+ /** + * Triggers an action with feature flags support. + * @since 1.0.0 + * @param connection Connection object + * @param datasourceConfiguration Datasource configuration + * @param request Trigger request data + * @param featureFlagMap Map of feature flag names to their enabled status + * @return Trigger result + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(1 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
🔇 Additional comments (10)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java (2)
3-3: LGTM! Good encapsulation of shared drive parameters.
The constant effectively isolates the shared drive query parameters, making the code more maintainable.
Also applies to: 29-30
87-87: LGTM! Good reuse of logic.
Appropriate delegation ensures consistent feature flag handling between trigger and execution paths.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (2)
12-12: LGTM: Clean dependency injection implementation
The FeatureFlagService integration follows proper dependency injection patterns and immutability principles.
Also applies to: 32-32, 39-40, 46-46
81-82: Enhance TODO comment with specific removal criteria
The TODO comment should include specific criteria for when this technical debt can be removed, such as test coverage thresholds or specific test scenarios that need to pass.
-// TODO: Flags are needed here for google sheets integration to support shared drive behind a flag
-// Once thoroughly tested, this flag can be removed
+// TODO: Remove feature flag once shared drive support is thoroughly tested with:
+// 1. 100% test coverage for shared drive scenarios
+// 2. Successful QA validation in staging environment
+// 3. No reported issues after 2 weeks in production
+// Tracking issue: #37714app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (4)
20-20: LGTM: Clean dependency injection
The FeatureFlagService integration follows proper dependency injection patterns.
Also applies to: 58-58
110-114: Consider using reactive feature flag retrieval
The synchronous getCachedTenantFeatureFlags() call breaks the reactive pattern.
123-127: Add error handling for feature flags
The feature flag integration lacks error handling for the triggerWithFlags call.
123-127: Verify plugin executor compatibility
Ensure all plugin executors implement the new triggerWithFlags method correctly.
✅ Verification successful
Plugin executors are compatible with triggerWithFlags
The triggerWithFlags method is properly defined in the PluginExecutor interface with a default implementation that falls back to the regular trigger method. The GoogleSheetsPlugin has already implemented this method, and other plugins will automatically use the default implementation, ensuring backward compatibility.
- The interface provides a default implementation in
PluginExecutor.java GoogleSheetsPlugin.javahas a custom implementation- Other plugins will use the default implementation that calls
trigger()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of triggerWithFlags across plugin executors
ast-grep --pattern 'class $_ implements PluginExecutor {
$$$
triggerWithFlags($$$) {
$$$
}
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Let's try a different approach to find triggerWithFlags implementations
# First, let's find all files containing "triggerWithFlags"
rg "triggerWithFlags" -l
# Then let's see the actual implementations with context
rg "triggerWithFlags" -A 5 -B 2
# Also search for the interface definition
rg "interface.*PluginExecutor" -A 10
Length of output: 12623
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (2)
250-262: Well-documented technical debt!
The TODO comment clearly explains the rationale, limitations, and future plans for handling feature flags in plugin modules.
272-284: LGTM!
The implementation correctly combines metrics and feature flags support while maintaining the existing metrics observation pattern.
...er/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/config/FileListMethod.java
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
This PR hides shared drive support for Google Sheets Integration behind a feature flag as we need to do thorough testing for it. The feature flag implementation done in this PR involves passing feature flags from server to google sheets plugin module. This temporary feature flagging solution tech debt can be removed once the testing is done and once we would release this feature to all. **Why feature flags are only available at server module?** Because they have a dependency on UserIdentifierService, SessionUserService, TenantService etc which exists at server module. Supporting feature flags out of the box at plugin module level would require significant efforts to migrate these dependencies. Fixes #37714 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ /ok-to-test tags="@tag.Authentication, @tag.Datasource" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12063930619> > Commit: 47f08f9 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12063930619&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Authentication, @tag.Datasource` > Spec: > <hr>Thu, 28 Nov 2024 07:38:58 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced support for Google Sheets shared drive through feature flags. - Added new methods to handle feature flags in plugin execution and triggering processes. - Enhanced action execution and triggering logic to utilize feature flags for dynamic behavior. - **Bug Fixes** - Improved error handling for plugin execution processes. - **Tests** - Integrated `FeatureFlagService` into the testing framework for enhanced test coverage. - Expanded test scenarios to include various response types and error conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
## Description Failing server unit test fixed, This test started failing due to changes in this PR: #37776, for some reason it did not show up on this Pr in failed test cases 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.IconButton" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12066311357> > Commit: f915b4f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12066311357&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IconButton` > Spec: > <hr>Thu, 28 Nov 2024 09:59:28 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling for stale connections during action execution, allowing the system to recover and continue executing actions after an initial failure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: “sneha122” <“[email protected]”>
Failing server unit test fixed, This test started failing due to changes in this PR: #37776, for some reason it did not show up on this Pr in failed test cases 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._ /ok-to-test tags="@tag.IconButton" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12066311357> > Commit: f915b4f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12066311357&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IconButton` > Spec: > <hr>Thu, 28 Nov 2024 09:59:28 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **Bug Fixes** - Improved error handling for stale connections during action execution, allowing the system to recover and continue executing actions after an initial failure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: “sneha122” <“[email protected]”>
…horg#37776) ## Description This PR hides shared drive support for Google Sheets Integration behind a feature flag as we need to do thorough testing for it. The feature flag implementation done in this PR involves passing feature flags from server to google sheets plugin module. This temporary feature flagging solution tech debt can be removed once the testing is done and once we would release this feature to all. **Why feature flags are only available at server module?** Because they have a dependency on UserIdentifierService, SessionUserService, TenantService etc which exists at server module. Supporting feature flags out of the box at plugin module level would require significant efforts to migrate these dependencies. Fixes appsmithorg#37714 _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.Authentication, @tag.Datasource" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12063930619> > Commit: 47f08f9 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12063930619&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Authentication, @tag.Datasource` > Spec: > <hr>Thu, 28 Nov 2024 07:38:58 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced support for Google Sheets shared drive through feature flags. - Added new methods to handle feature flags in plugin execution and triggering processes. - Enhanced action execution and triggering logic to utilize feature flags for dynamic behavior. - **Bug Fixes** - Improved error handling for plugin execution processes. - **Tests** - Integrated `FeatureFlagService` into the testing framework for enhanced test coverage. - Expanded test scenarios to include various response types and error conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
## Description Failing server unit test fixed, This test started failing due to changes in this PR: appsmithorg#37776, for some reason it did not show up on this Pr in failed test cases 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.IconButton" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12066311357> > Commit: f915b4f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12066311357&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IconButton` > Spec: > <hr>Thu, 28 Nov 2024 09:59:28 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling for stale connections during action execution, allowing the system to recover and continue executing actions after an initial failure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: “sneha122” <“[email protected]”>
Description
This PR hides shared drive support for Google Sheets Integration behind a feature flag as we need to do thorough testing for it.
The feature flag implementation done in this PR involves passing feature flags from server to google sheets plugin module. This temporary feature flagging solution tech debt can be removed once the testing is done and once we would release this feature to all.
Why feature flags are only available at server module?
Because they have a dependency on UserIdentifierService, SessionUserService, TenantService etc which exists at server module. Supporting feature flags out of the box at plugin module level would require significant efforts to migrate these dependencies.
Fixes #37714
or
Fixes
Issue URLWarning
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.Authentication, @tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12063930619
Commit: 47f08f9
Cypress dashboard.
Tags:
@tag.Authentication, @tag.DatasourceSpec:
Thu, 28 Nov 2024 07:38:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
FeatureFlagServiceinto the testing framework for enhanced test coverage.