fix: updated the regex expression to correctly identify the table nam… #35361
Conversation
WalkthroughThe changes implemented in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PostgresPlugin
participant Database
Client->>PostgresPlugin: Request table structure
PostgresPlugin->>Database: Query for table structure
Database-->>PostgresPlugin: Return table structure
PostgresPlugin-->>Client: Return quoted table name and structure
sequenceDiagram
participant TestRunner
participant PostgresPluginTest
participant Database
TestRunner->>PostgresPluginTest: Run setup
PostgresPluginTest->>Database: Create new test tables
Database-->>PostgresPluginTest: Confirm table creation
TestRunner->>PostgresPluginTest: Execute test cases for various naming conventions
PostgresPluginTest->>Database: Validate table structures
Database-->>PostgresPluginTest: Return structure data
PostgresPluginTest-->>TestRunner: Return test results
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (3 hunks)
Additional comments not posted (3)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1)
887-887: Verify the updated regex pattern for correctness.The regex pattern has been updated to
\\.(.+)to capture any character sequence following the period. This change should correctly handle table names with hyphens. Ensure that this pattern does not introduce any unexpected behavior with other table name formats.app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (2)
715-781: LGTM! Ensure comprehensive test coverage.The new test method
testStructure_containing_hyphencorrectly verifies the structure of thetesting-table-datatable.However, ensure that the test coverage is comprehensive and includes edge cases.
195-202: LGTM! Ensure the new table creation is verified.The SQL command for creating the
testing-table-datatable appears correct.However, ensure that the new table creation is verified in subsequent tests.
|
Hii @rohan-arthur @ajinkyakulkarni ,Could you please assign the reviewer for it. |
...erver/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java
Show resolved
Hide resolved
|
Hii @IAmAnubhavSaini , I have updated with proper regex to identify the table names with hypens .I have also cross checked with , Its working as expected.Please review it. thank you. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1 hunks)
Additional comments not posted (1)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1)
887-887: LGTM! Verify the impact of the regex change across the codebase.The regex pattern has been correctly updated to handle table names with hyphens. Ensure that this change does not introduce any unintended side effects in other parts of the codebase that might rely on the previous pattern.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Nikhil-Nandagopal
requested review from
NilanshBansal
and removed request for
IAmAnubhavSaini
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10494572254. |
|
Deploy-Preview-URL: https://ce-35361.dp.appsmith.com |
...erver/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java
Outdated
Show resolved
Hide resolved
|
@Naveen-Goud this regex does not work if the table name has dots in it. |
|
@Naveen-Goud this regex may work out better for the table names containing dots as well.
|
|
@Naveen-Goud the server-unit-test |
|
Hii @NilanshBansal , I have updated the regex in latest commit because we want to accept the hypes(-) only , when we are using dot(.) in regex , its accepting all the special characters in table names . if you want me to use the prev regex ,it allows all the special chars like $,#,@,*,& in table names , If you are okay with this ,I will updated it to prev regex. My thought was to not allow special characters other than hypens(-),underscore(_) also we cannot have mutliple dot in table names, it leads nested structure if we use mutliple dots in table names. |
@Naveen-Goud yes we should update the regex to this to allow special characters also as supabase also supports these naming for their tables. 
Please make the relevant changes along with the suggestions in comment 1, comment 2 and comment 3 Let me know once you have done the changes will re-review the PR |
… the test cases for this scenarios.
|
Hii @NilanshBansal , I have updated the regex and the test case as you mentioned in the above comments.Can review now. thank you. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (5 hunks)
Additional comments not posted (4)
app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (1)
887-887: Evaluate the implications of the updated regex pattern.The regex pattern in
quotedTableNamewas changed from\\.(\\w+)to\\.(.+), which broadens the matching criteria. While this change allows for more complex table names, it may also permit unintended characters. Ensure that this change does not introduce security vulnerabilities or incorrect behavior by allowing special characters that should be restricted.Consider adding unit tests to cover edge cases with special characters and validate the expected behavior.
app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (3)
352-414: LGTM! Comprehensive test for special characters in table names.The test case effectively verifies the structure and templates for a table with special characters.
416-478: LGTM! Thorough test for underscores and hyphens in table names.The test case effectively verifies the structure and templates for a table with underscores and hyphens.
480-542: LGTM! Comprehensive test for dots in table names.The test case effectively verifies the structure and templates for a table with dots.
...r/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java
Outdated
Show resolved
Hide resolved
|
Thanks @Naveen-Goud for the changes. I have pulled the latest changes and triggered tests on the shadow PR here: #35832 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (5 hunks)
Additional comments not posted (4)
app/server/appsmith-plugins/postgresPlugin/src/test/java/com/external/plugins/PostgresPluginTest.java (4)
352-414: Comprehensive test for special characters in table names.The test method
testStructure_containing_special_charseffectively verifies the structure of a table with special characters in its name. The assertions cover the table's existence, columns, primary keys, and SQL templates.
416-478: Comprehensive test for underscores and hyphens in table names.The test method
testStructure_containing_underscore_and_hypheneffectively verifies the structure of a table with underscores and hyphens in its name. The assertions cover the table's existence, columns, primary keys, and SQL templates.
480-542: Comprehensive test for dots in table names.The test method
testStructure_containing_dotseffectively verifies the structure of a table with dots in its name. The assertions cover the table's existence, columns, primary keys, and SQL templates.
941-1006: Comprehensive test for hyphens in table names.The test method
testStructure_containing_hypheneffectively verifies the structure of a table with hyphens in its name. The assertions cover the table's existence, columns, primary keys, and SQL templates.
|
@Naveen-Goud tests are failing on the shadow PR, please check and fix. |
|
Hii @NilanshBansal , The test failing are related to cypress which are for frontend. My changes are in backend only and added Junit tests. thank you. |
|
@Naveen-Goud the backend changes can also trigger failure for certain cypress tests. Can you please confirm once if these are related to your PR or not? |
|
@NilanshBansal , those tests are not related to my PR. |
|
@Naveen-Goud thanks for confirming, I have re-triggered tests on the shadow PR |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10576066486. |
|
Deploy-Preview-URL: https://ce-35361.dp.appsmith.com |
|
@Naveen-Goud thank you for your patience, this PR has been merged! We appreciate your contribution towards making Appsmith better ❤️ |
Description
When the user tries to add a postgres datasource and add any query, if the table name has hyphens in it, then quotes get assigned to first word after the schema and the dot. For example, public."counter"-with-db
The quotes should be applied to the whole table name. For example: public."counter-with-db"
Fixes #10631
Fixes #30692
changes in PR:
1.updated the regec expression to idetify the table name properly with table name consists of hypens.
2.added a test case for this scenario.
snapshots:
before:
After:
Hi @ajinkyakulkarni @rohan-arthur @Nikhil-Nandagopal ,Please review this PR.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests