fix(plugins): parameterize query execution for Redshift, Snowflake and Databricks#41967
Draft
wyattwalter wants to merge 3 commits into
Draft
fix(plugins): parameterize query execution for Redshift, Snowflake and Databricks#41967wyattwalter wants to merge 3 commits into
wyattwalter wants to merge 3 commits into
Conversation
…d Databricks Adds prepared-statement support to the Redshift, Snowflake and Databricks plugins so query execution is parameterized, bringing them in line with the other SQL plugins (Postgres, MySQL, MSSQL). - Bind dynamic values as prepared-statement parameters instead of interpolating them into the raw SQL string. - Add a "Use prepared statement" toggle in the plugin editor/settings, with prepared statements enabled by default and an opt-out matching the Postgres/MySQL/MSSQL behavior. When the setting is absent or not a recognized value, execution defaults to prepared; only an explicit opt-out selects raw execution. - Reject array-typed parameters with a clear error rather than binding them incorrectly, and ensure prepared statements are closed on binding failure. - Add the plugins' dependency.json so the editor links the query body to the prepared-statement toggle. Adds unit tests covering the prepared path, the default-on behavior for absent/malformed settings, the explicit opt-out, and resource cleanup for all three plugins.
Contributor
WalkthroughDatabricks, Redshift, and Snowflake plugins add prepared-statement execution paths, typed parameter binding, UI toggles, dependency wiring, execution metadata, and regression tests. ChangesDatabricks Prepared Statement Support
Redshift Prepared Statement Support
Snowflake Prepared Statement Support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DatabricksPluginExecutor
participant PreparedStatement
Client->>DatabricksPluginExecutor: executeParameterized(...)
DatabricksPluginExecutor->>DatabricksPluginExecutor: read prepared-statement toggle
DatabricksPluginExecutor->>DatabricksPluginExecutor: rewrite mustache bindings to ?
DatabricksPluginExecutor->>PreparedStatement: prepareStatement(rewrittenSQL)
DatabricksPluginExecutor->>PreparedStatement: bind values and execute
PreparedStatement-->>DatabricksPluginExecutor: rows or update count
DatabricksPluginExecutor-->>Client: ActionExecutionResult
sequenceDiagram
participant Client
participant RedshiftPluginExecutor
participant PreparedStatement
Client->>RedshiftPluginExecutor: executeParameterized(...)
RedshiftPluginExecutor->>RedshiftPluginExecutor: read prepared-statement toggle
RedshiftPluginExecutor->>RedshiftPluginExecutor: rewrite mustache bindings to ?
RedshiftPluginExecutor->>PreparedStatement: prepareStatement(transformedSQL)
RedshiftPluginExecutor->>PreparedStatement: bind values and execute
PreparedStatement-->>RedshiftPluginExecutor: rows or update count
RedshiftPluginExecutor-->>Client: ActionExecutionResult
sequenceDiagram
participant Client
participant SnowflakePluginExecutor
participant ExecutionUtils
participant PreparedStatement
Client->>SnowflakePluginExecutor: executeParameterized(...)
SnowflakePluginExecutor->>SnowflakePluginExecutor: read prepared-statement toggle
SnowflakePluginExecutor->>SnowflakePluginExecutor: rewrite mustache bindings to ?
SnowflakePluginExecutor->>PreparedStatement: prepareStatement(preparedSQL)
SnowflakePluginExecutor->>PreparedStatement: bind values and execute
SnowflakePluginExecutor->>ExecutionUtils: getRowsFromPreparedStatement(...)
ExecutionUtils-->>SnowflakePluginExecutor: rows list
SnowflakePluginExecutor-->>Client: ActionExecutionResult
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (2)
307-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
StandardCharsets.UTF_8over the string variant.
value.getBytes("UTF-8")throws a checkedUnsupportedEncodingExceptionthat can never fire. UsingStandardCharsets.UTF_8is the idiomatic Java 17 approach and eliminates the deadIOExceptioncatch path.♻️ Proposed fix
case BYTES: { - preparedStatement.setBytes(index, value.getBytes("UTF-8")); + preparedStatement.setBytes(index, value.getBytes(StandardCharsets.UTF_8)); break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java` around lines 307 - 313, In SnowflakePlugin’s prepared-statement value handling, the BYTES branch is using the string-based UTF-8 lookup, which adds an unnecessary checked-exception path. Update the BYTES case in the switch that calls preparedStatement.setBytes to use StandardCharsets.UTF_8 instead of the string literal, and remove any now-dead IOException/UnsupportedEncodingException handling around this conversion if it becomes unused.
357-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
defaultcase silently skips parameter binding.An unrecognized
DataTypewill leave the?placeholder unbound whileinsertedParamsstill records the entry, producing a confusing JDBC "parameter not set" error at execution time. Add a log warning or throw to fail fast.♻️ Proposed fix
case ARRAY: case NULL_ARRAY: throw new AppsmithPluginException( AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, String.format(SnowflakeErrorMessages.ARRAY_PARAMETER_NOT_SUPPORTED_ERROR_MSG, binding)); default: - break; + log.warn("Unrecognized data type {} for binding {}, skipping parameter binding", valueType, binding); + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java` around lines 357 - 358, The default branch in SnowflakePlugin’s parameter binding switch is silently ignoring unknown DataType values, which leaves placeholders unbound while insertedParams still tracks them. Update the binding logic in the relevant parameter-processing method to fail fast on unsupported types, either by logging a clear warning and stopping or by throwing an explicit exception, so the issue is caught before JDBC execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java`:
- Around line 411-412: The catch block in DatabricksPlugin should guard against
a null exception message before calling contains on e.getMessage(). Update the
SQLException branch in the exception handler to safely check for a non-null
message before matching "The column index is out of range:", so the logic still
routes through the intended AppsmithPluginException path instead of throwing an
NPE.
In
`@app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java`:
- Around line 577-578: The SQLException message check in RedshiftPlugin’s
exception handling is not null-safe because e.getMessage() may be null before
calling contains(). Update the catch block in RedshiftPlugin to guard the
message lookup before checking for "The column index is out of range:", keeping
the existing SQLException/IllegalArgumentException/java.io.IOException flow
intact while avoiding an NPE.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java`:
- Around line 307-313: In SnowflakePlugin’s prepared-statement value handling,
the BYTES branch is using the string-based UTF-8 lookup, which adds an
unnecessary checked-exception path. Update the BYTES case in the switch that
calls preparedStatement.setBytes to use StandardCharsets.UTF_8 instead of the
string literal, and remove any now-dead IOException/UnsupportedEncodingException
handling around this conversion if it becomes unused.
- Around line 357-358: The default branch in SnowflakePlugin’s parameter binding
switch is silently ignoring unknown DataType values, which leaves placeholders
unbound while insertedParams still tracks them. Update the binding logic in the
relevant parameter-processing method to fail fast on unsupported types, either
by logging a clear warning and stopping or by throwing an explicit exception, so
the issue is caught before JDBC execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 215dcebe-0403-499a-83f9-a443d16e56e9
📒 Files selected for processing (19)
app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.javaapp/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/exceptions/DatabricksErrorMessages.javaapp/server/appsmith-plugins/databricksPlugin/src/main/resources/dependency.jsonapp/server/appsmith-plugins/databricksPlugin/src/main/resources/editor/root.jsonapp/server/appsmith-plugins/databricksPlugin/src/main/resources/setting.jsonapp/server/appsmith-plugins/databricksPlugin/src/test/java/com/external/plugins/DatabricksPluginTest.javaapp/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.javaapp/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/exceptions/RedshiftErrorMessages.javaapp/server/appsmith-plugins/redshiftPlugin/src/main/resources/dependency.jsonapp/server/appsmith-plugins/redshiftPlugin/src/main/resources/editor.jsonapp/server/appsmith-plugins/redshiftPlugin/src/main/resources/setting.jsonapp/server/appsmith-plugins/redshiftPlugin/src/test/java/com/external/plugins/RedshiftPluginTest.javaapp/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaapp/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.javaapp/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/ExecutionUtils.javaapp/server/appsmith-plugins/snowflakePlugin/src/main/resources/dependency.jsonapp/server/appsmith-plugins/snowflakePlugin/src/main/resources/editor.jsonapp/server/appsmith-plugins/snowflakePlugin/src/main/resources/setting.jsonapp/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
…nowflake and Databricks - Guard against a null exception message when classifying JDBC binding errors so a null message no longer triggers a NullPointerException. - Log a warning when an unrecognized data type is encountered during parameter binding, instead of silently leaving the placeholder unbound and surfacing a confusing driver error later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28978657747. |
|
Deploy-Preview-URL: https://ce-41967.dp.appsmith.com |
subrata71
reviewed
Jul 16, 2026
wyattwalter
marked this pull request as draft
July 16, 2026 14:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear: APP-15378
Summary
The Redshift, Snowflake and Databricks plugins previously executed queries by substituting dynamic bindings directly into the SQL text. This PR adds prepared-statement support to all three so query execution is parameterized, bringing them in line with the other SQL plugins (Postgres, MySQL, MSSQL).
Changes
{{...}}) are bound asPreparedStatementparameters instead of being substituted into the SQL string.dependency.json— added for each plugin so the editor links the query body to the prepared-statement toggle (matching the existing SQL plugins).Tests
Adds unit tests for all three plugins covering:
prepareStatementused, values bound,createStatementnever called),Focused module suites are green:
redshiftPlugin19,snowflakePlugin16,databricksPlugin8 — 0 failures.spotless:checkpasses for all three modules.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Warning
Tests have not run on the HEAD 2da5573 yet
Wed, 08 Jul 2026 21:35:06 UTC