fix: google sheets query getting executed even after changing sheet#37006
fix: google sheets query getting executed even after changing sheet#37006
Conversation
…d sheet issue fixed
github-actions
bot
added
Bug
WalkthroughThe pull request introduces several changes to the Google Sheets plugin, enhancing error handling and user authorization processes. A new error message constant is added for missing spreadsheet URLs, and the method for retrieving authorized sheet IDs has been renamed to include validation logic. This ensures that only selected sheets are accessible based on user authorization. Corresponding test cases have been updated to reflect these changes, ensuring robust validation and error handling. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java (1)
42-44: LGTM. Consider minor improvement.The new error message constant is well-defined and aligns with the PR objectives. It provides clear guidance to users when encountering issues with spreadsheet authorization.
Consider wrapping the long string for better readability:
- public static final String MISSING_SPREADSHEET_URL_SELECTED_SHEETS_ERROR_MSG = - "Missing required field 'Spreadsheet Url', Check if your datasource is authorised to use this spreadsheet or not"; + public static final String MISSING_SPREADSHEET_URL_SELECTED_SHEETS_ERROR_MSG = + "Missing required field 'Spreadsheet Url'. " + + "Check if your datasource is authorized to use this spreadsheet.";This change also corrects the spelling of "authorised" to "authorized" and improves the message structure.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (1)
342-343: Trigger method updated to use enhanced validation.The
validateAndGetUserAuthorizedSheetIdsmethod is now used in the trigger functionality, consistent with the changes inexecuteCommon. This ensures consistent validation across different entry points.Consider updating the relevant test cases to cover this new validation process in the trigger method.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java (1)
44-49: Refine code comments for clarity and readabilityThe comments can be improved for better clarity. Consider rephrasing and consolidating them into a coherent block.
Revised comment:
// Added specifically for selected Google Sheets: // When authorization changes from one sheet to another, we throw an error. // This is because using the 'drive.file' scope grants access to all selected sheets across datasources. // We aim to constrain access to the sheet selected during datasource authorization.app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (1)
27-28: UseassertNullfor null checksThe
assertNullmethod provides a more direct and readable way to check fornullvalues.Apply this diff to improve the assertion:
- assertEquals(result, null); + assertNull(result);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java (1 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (3 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java (3 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (3)
54-54: Import statement updated to include validation.The change from
getUserAuthorizedSheetIdstovalidateAndGetUserAuthorizedSheetIdssuggests an enhanced validation process for authorized sheet IDs. This is a good practice for improving data integrity and security.
54-54: Summary of changes: Enhanced validation for authorized sheet IDsThe changes consistently implement a new validation process for authorized sheet IDs across the plugin. This addresses the issue of unauthorized access to Google Sheets data after changing the selected sheet, as described in the PR objectives.
Key points:
- Import statement updated to reflect the new validation method.
- Both
executeCommonandtriggermethods now use the enhanced validation.- The validation process now considers the method configuration.
These changes should effectively prevent the execution of queries on unauthorized sheets, improving the security and data integrity of the Google Sheets integration.
To ensure the effectiveness of these changes:
- Verify that all relevant test cases have been updated to cover the new validation process.
- Conduct end-to-end testing to confirm that queries are only executed on authorized sheets, especially after changing the selected sheet.
#!/bin/bash # Search for test files related to GoogleSheetsPlugin rg "class.*Test.*GoogleSheets" --type javaAlso applies to: 178-179, 342-343
178-179: Method call updated to include validation and method configuration.The
validateAndGetUserAuthorizedSheetIdsmethod now takesmethodConfigas an additional parameter. This change aligns with the updated import and suggests a more context-aware validation process.To ensure this change doesn't introduce any unintended side effects, please verify that all calls to this method across the codebase have been updated accordingly.
✅ Verification successful
Verification Successful: No remaining calls to
getUserAuthorizedSheetIdsfound.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls to getUserAuthorizedSheetIds rg "getUserAuthorizedSheetIds" --type javaLength of output: 124
Script:
#!/bin/bash # Search for any remaining calls to getUserAuthorizedSheetIds rg "getUserAuthorizedSheetIds" --type javaLength of output: 42
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java (1)
24-25: Ensure all references to the renamed method are updatedThe method
getUserAuthorizedSheetIdshas been renamed tovalidateAndGetUserAuthorizedSheetIdsand its signature has changed. Please verify that all usages of this method in the codebase have been updated accordingly.✅ Verification successful
All method references have been successfully updated
- No references to
getUserAuthorizedSheetIdsfound.- All usages now correctly reference
validateAndGetUserAuthorizedSheetIds.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find references to the old method name in the codebase. # Test: Search for usages of 'getUserAuthorizedSheetIds' in Java files. rg 'getUserAuthorizedSheetIds' --type java --no-heading --line-numberLength of output: 69
Script:
#!/bin/bash # Description: Verify all references to both old and new method names in Java files. # Search for the old method name 'getUserAuthorizedSheetIds' echo "Checking for old method name 'getUserAuthorizedSheetIds':" rg 'getUserAuthorizedSheetIds' --type java --no-heading --line-number # Search for the new method name 'validateAndGetUserAuthorizedSheetIds' echo "Checking for new method name 'validateAndGetUserAuthorizedSheetIds':" rg 'validateAndGetUserAuthorizedSheetIds' --type java --no-heading --line-numberLength of output: 2197
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java
Show resolved
Hide resolved
...er/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java
Show resolved
Hide resolved
...er/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11460865686. |
|
Deploy-Preview-URL: https://ce-37006.dp.appsmith.com |
.../appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java
Show resolved
Hide resolved
app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (1)
Line range hint
32-53: LGTM with a minor suggestion.The test method has been correctly updated to match the new
validateAndGetUserAuthorizedSheetIdssignature. The assertion checks the size of the result set as expected.Consider adding an additional assertion to verify the content of the result set:
assertTrue(result.contains("id1"));This would ensure not only the correct size but also the correct content of the returned set.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java (1 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/constants/ErrorMessages.java
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (1)
Learnt from: sneha122 PR: appsmithorg/appsmith#37006 File: app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/utils/SheetsUtil.java:27-27 Timestamp: 2024-10-22T14:16:16.702Z Learning: In the `validateAndGetUserAuthorizedSheetIds` method in `SheetsUtil.java`, initializing `userAuthorisedSheetIds` to null is intentional for subsequent null-checks in the codebase.
🔇 Additional comments (5)
app/server/appsmith-plugins/googleSheetsPlugin/src/test/java/com/external/config/SheetsUtilTest.java (5)
19-28: LGTM: Test case updated correctly.The test method has been updated to match the new
validateAndGetUserAuthorizedSheetIdssignature and behavior. The assertion correctly checks for a null result when all sheets are selected.
55-89: Excellent addition of error case test.This new test case effectively verifies the behavior when an invalid specific sheet is provided. The use of
assertThrowsand the verification of the error message are both well-implemented.
91-123: Well-crafted test for the happy path scenario.This new test case thoroughly verifies the behavior when a valid specific sheet is provided. The assertions checking both the size and content of the result set provide comprehensive coverage.
125-138: Good coverage for the all-sheets scenario.This new test case effectively verifies the behavior when no specific sheets are selected. The use of
assertNullis appropriate, and this test complements the earlier all-sheets test, providing more specific coverage.
Line range hint
1-138: Comprehensive test coverage and well-structured test class.The updates to this test class have significantly improved its coverage and structure:
- All relevant scenarios are now covered: all sheets, specific sheets, invalid sheets, and edge cases.
- Test method names are descriptive and follow a consistent pattern.
- There's a good balance between positive and negative test cases.
- The new imports and constant usage enhance maintainability.
These changes should ensure robust testing of the
SheetsUtilclass'svalidateAndGetUserAuthorizedSheetIdsmethod.
sneha122
added
ok-to-test
|
waiting to run the specific gsheets tests workflow to ensure no expensive breakages in later stages |
|
Triggered the workflow here: https://github.com/appsmithorg/appsmith/actions/runs/11482943399 |
|
All google sheets test cases have passed in the workflow, we should be good to merge the PR |
…ppsmithorg#37006) ## Description This PR fixes the issue with google sheets -> selected sheets option. Following are the steps to reproduce the issue on cloud/release platform: **Steps:** 1. Create a Google sheets datasource with selected sheets option 2. Select "spreadsheet1" from the file picker. 3. Create a query for this datasource and attach it to table widget 4. Now edit the datasource and change authorisation from "spreadsheet1" to "spreadsheet2" 5. Refresh the page and check the table data This issue has been fixed, where we check if the correct spreadsheetId is present in authorisedSheetIds, if it's not throwing an exception so we don't execute the query. Fixes appsmithorg#36747 _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" ### 🔍 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/11473463879> > Commit: e0f725a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11473463879&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 23 Oct 2024 05:43:59 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 - **New Features** - Introduced a new error message for missing spreadsheet URLs to enhance user guidance. - Updated method for retrieving authorized sheet IDs to include validation, improving error handling. - **Bug Fixes** - Enhanced error handling for cases where the spreadsheet URL is invalid or missing. - **Tests** - Added new test cases to validate the updated functionality for authorized sheet IDs, ensuring robust error handling and correct returns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
3 participants
Description
This PR fixes the issue with google sheets -> selected sheets option. Following are the steps to reproduce the issue on cloud/release platform:
Steps:
This issue has been fixed, where we check if the correct spreadsheetId is present in authorisedSheetIds, if it's not throwing an exception so we don't execute the query.
Fixes #36747
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11473463879
Commit: e0f725a
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 23 Oct 2024 05:43:59 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests