feat: Add SeaTable as native data source plugin#41629
Open
christophdb wants to merge 7 commits intoappsmithorg:releasefrom
Open
feat: Add SeaTable as native data source plugin#41629christophdb wants to merge 7 commits intoappsmithorg:releasefrom
christophdb wants to merge 7 commits intoappsmithorg:releasefrom
Conversation
Add a native Appsmith data source plugin for SeaTable, an open-source database platform (spreadsheet-database hybrid). Supported operations: - List Rows (with filtering, sorting, pagination) - Get Row (by ID) - Create Row - Update Row - Delete Row - List Tables (metadata/schema discovery) - SQL Query Authentication uses SeaTable's API Token, which is automatically exchanged for a base access token. The plugin is stateless (HTTP via WebClient) and follows the UQI editor pattern (like Firestore). All API endpoints verified against the SeaTable OpenAPI specification and tested against a live SeaTable instance. Closes appsmithorg#41627
- Add private constructors to FieldName and SeaTableErrorMessages utility classes - Use MessageFormat.format() in SeaTablePluginError instead of super delegation - Add 30s timeout to all HTTP requests (fetchAccessToken, executeRequest, getStructure) - Add null-checks for access token response fields - Add defensive null-checks for metadata parsing (table name, column name/type) - Add Javadoc to all public and significant private methods - Add timeout validation (min: 1) and placeholder to setting.json - Use FieldName constants instead of string literals in tests - Add HTTP request assertions (method, path, headers, body) to all tests via MockWebServer.takeRequest()
- Add getErrorAction() and AppsmithErrorAction to SeaTablePluginError (match BasePluginError interface fully, follow FirestorePluginError pattern) - Validate required form fields before fetching access token (avoid unnecessary network calls) - Guard against null executeActionDTO.getParams() in smart substitution - Initialize structure.setTables() before early returns in getStructure() - Switch tests from @BeforeAll/@afterall to @BeforeEach/@AfterEach for per-test MockWebServer isolation
SeaTable should appear under "SaaS Integrations" alongside Airtable and Google Sheets, not under "APIs". Also fix documentationLink URL.
Contributor
WalkthroughAdds a new SeaTable native data source plugin: module registration, plugin implementation with token exchange and CRUD/SQL commands, editor UIs and settings, unit tests, and a DB migration to register and provision the plugin. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Plugin as SeaTable Plugin
participant SeaTableAPI as SeaTable API
participant AppsmithDB as Appsmith DB
Client->>Plugin: executeParameterized(actionConfig)
activate Plugin
Plugin->>SeaTableAPI: GET /api/v2.1/dtable/app-access-token/ (API token)
activate SeaTableAPI
SeaTableAPI-->>Plugin: {access_token,...}
deactivate SeaTableAPI
Plugin->>Plugin: route command (LIST_ROWS/GET_ROW/CREATE/UPDATE/DELETE/LIST_TABLES/SQL)
alt row/table/sql operations
Plugin->>SeaTableAPI: GET/POST/PUT/DELETE /api/v2/dtables/{uuid}/...
activate SeaTableAPI
SeaTableAPI-->>Plugin: response payload
deactivate SeaTableAPI
end
Plugin-->>Client: ActionExecutionResult
deactivate Plugin
Note over AppsmithDB: Migration registers plugin package and provisions to workspaces
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
4 tasks
![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: 4
🧹 Nitpick comments (4)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.java (1)
3-6: Consider marking the class asfinal.Since this is a utility class with a private constructor, adding
finalexplicitly prevents subclassing and signals intent.♻️ Suggested change
-public class FieldName { +public final class FieldName { private FieldName() { // Utility class - prevent instantiation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.java` around lines 3 - 6, The FieldName utility class is intended to be non-instantiable and non-subclassable; make the intent explicit by declaring the class final (i.e., mark FieldName as final) while leaving the private constructor intact to prevent instantiation and inheritance.app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java (1)
218-252: Please pin the defensive token/metadata branches with tests.The suite only locks in happy-path
access_tokenandmetadata.tablesresponses. Since this PR added explicit null/defensive handling around both, one negative test for a missing token and one for empty/malformed metadata would keep those guards from regressing.Also applies to: 481-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java` around lines 218 - 252, Add two negative tests to pin the defensive branches introduced for missing token and malformed/empty metadata: ensure pluginExecutor.testDatasource is exercised when the token endpoint returns no token (e.g., 401 or a body missing access_token) and when the metadata/tables endpoint returns empty or malformed JSON (e.g., {} or invalid structure). Use the same test scaffolding helpers like enqueueAccessTokenResponse / mockWebServer.enqueue and createDatasourceConfig to set up responses, then assert that DatasourceTestResult.getInvalids() is non-empty for each case; reference the existing testTestDatasource_success and testTestDatasource_invalidToken to mirror structure so these guards won’t regress.app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (2)
209-212: Non-idiomatic validation pattern.The
validateCommandInputsmethod returnsnullfor success instead ofMono.empty(). This pattern is functional but unusual in reactive code. The.then(Mono.empty())is also redundant since the error propagates anyway.Consider returning
Mono.empty()for success and always usingflatMap:♻️ Suggested refactor
- Mono<Void> validation = validateCommandInputs(command, formData); - if (validation != null) { - return validation.then(Mono.empty()); - } + return validateCommandInputs(command, formData) + .then(fetchAccessToken(datasourceConfiguration)) + .flatMap(tokenResponse -> {And update
validateCommandInputsto returnMono.empty()on success instead ofnull:- return null; + return Mono.empty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 209 - 212, The validateCommandInputs method currently returns null on success which forces the caller to null-check and call .then(Mono.empty()), which is non-idiomatic; change validateCommandInputs to always return Mono<Void> and emit Mono.empty() on success (errors remain as error signals), then replace the null-checking call site in SeaTablePlugin (the block that calls validateCommandInputs(command, formData)) with a reactive composition (e.g., flatMap) that chains the returned Mono directly and remove the redundant .then(Mono.empty()) so validation flows as a normal Mono<Void>.
382-422: Duplicate validation across command methods.
validateCommandInputs(lines 382-422) validates required fields before network calls, but each command method (executeListRows,executeGetRow, etc.) repeats the same validations. This is defensive but adds maintenance burden.Since
validateCommandInputsis called before any command executes, the duplicate checks in individual methods are unreachable code under normal flow.♻️ Consider removing duplicate validations from command methods
If you trust
validateCommandInputsto always run first (which it does viaexecuteQuery), you could remove the redundant checks from individual command methods. However, if you prefer keeping them for defense-in-depth, that's also reasonable—just documenting the intent would help future maintainers.Also applies to: 430-438, 475-490, 504-514, 540-556, 588-603, 637-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 382 - 422, The command-specific methods (executeListRows, executeGetRow, executeCreateRow, executeUpdateRow, executeDeleteRow, executeSqlQuery) repeat the same presence checks already performed by validateCommandInputs (which is invoked from executeQuery); remove the redundant StringUtils.isBlank checks and corresponding Mono.error returns from those command methods to avoid unreachable/duplicated validation logic, or alternatively replace them with a single short comment referencing validateCommandInputs for defensive-intent; ensure validateCommandInputs remains the single source of truth for TABLE_NAME/ROW_ID/SQL validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.java`:
- Around line 66-69: The SeaTablePluginError.getMessage method should follow the
PostgresPluginError pattern by using Java's MessageFormat to format the
top-level message: replace the current use of
replacePlaceholderWithValue(this.message, args) in getMessage with new
MessageFormat(this.message).format(args); retain replacePlaceholderWithValue for
any downstream field-specific formatting only. Update
SeaTablePluginError.getMessage accordingly to call MessageFormat.format on
this.message and leave replacePlaceholderWithValue untouched for other helper
uses.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 244-251: fetchAccessToken currently dereferences
datasourceConfiguration.getUrl() and datasourceConfiguration.getAuthentication()
(DBAuth) blindly and can NPE during testDatasource/getStructure; add defensive
null/blank checks for serverUrl and auth/apiToken at the start of
fetchAccessToken and return a failed Mono (e.g., Mono.error with a clear
IllegalArgumentException or custom error) when datasourceConfiguration.getUrl()
is null/blank or authentication is null/missing password, and ensure you
trim/normalize serverUrl only after the null check; reference fetchAccessToken,
DatasourceConfiguration, DBAuth, serverUrl, and apiToken to locate and implement
the checks.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java`:
- Line 677: Replace the third-party icon URL used for the plugin by setting a
local Appsmith-hosted icon instead of "https://seatable.com/favicon.svg"; update
the call to plugin.setIconLocation(...) in DatabaseChangelog2 (the plugin
variable in this migration) to point to the appropriate Appsmith asset (use the
same Appsmith-hosted icon pattern used by the other plugin migrations) so the
migration no longer depends on an external host.
- Around line 680-685: The insert into Mongo via mongoTemplate.insert(plugin)
can throw DuplicateKeyException leaving `plugin` not persisted, so avoid passing
a null/transient id into installPluginToAllWorkspaces; in the catch block for
DuplicateKeyException catch the existing persisted plugin by querying for the
same package name (or unique key) using mongoTemplate.findOne(...) to retrieve
its real id and then call installPluginToAllWorkspaces(mongoTemplate,
persistedPlugin.getId()) (or skip installation if not found), ensuring the
change set is idempotent and uses the persisted document's id rather than
plugin.getId().
---
Nitpick comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.java`:
- Around line 3-6: The FieldName utility class is intended to be
non-instantiable and non-subclassable; make the intent explicit by declaring the
class final (i.e., mark FieldName as final) while leaving the private
constructor intact to prevent instantiation and inheritance.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 209-212: The validateCommandInputs method currently returns null
on success which forces the caller to null-check and call .then(Mono.empty()),
which is non-idiomatic; change validateCommandInputs to always return Mono<Void>
and emit Mono.empty() on success (errors remain as error signals), then replace
the null-checking call site in SeaTablePlugin (the block that calls
validateCommandInputs(command, formData)) with a reactive composition (e.g.,
flatMap) that chains the returned Mono directly and remove the redundant
.then(Mono.empty()) so validation flows as a normal Mono<Void>.
- Around line 382-422: The command-specific methods (executeListRows,
executeGetRow, executeCreateRow, executeUpdateRow, executeDeleteRow,
executeSqlQuery) repeat the same presence checks already performed by
validateCommandInputs (which is invoked from executeQuery); remove the redundant
StringUtils.isBlank checks and corresponding Mono.error returns from those
command methods to avoid unreachable/duplicated validation logic, or
alternatively replace them with a single short comment referencing
validateCommandInputs for defensive-intent; ensure validateCommandInputs remains
the single source of truth for TABLE_NAME/ROW_ID/SQL validation.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java`:
- Around line 218-252: Add two negative tests to pin the defensive branches
introduced for missing token and malformed/empty metadata: ensure
pluginExecutor.testDatasource is exercised when the token endpoint returns no
token (e.g., 401 or a body missing access_token) and when the metadata/tables
endpoint returns empty or malformed JSON (e.g., {} or invalid structure). Use
the same test scaffolding helpers like enqueueAccessTokenResponse /
mockWebServer.enqueue and createDatasourceConfig to set up responses, then
assert that DatasourceTestResult.getInvalids() is non-empty for each case;
reference the existing testTestDatasource_success and
testTestDatasource_invalidToken to mirror structure so these guards won’t
regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba53fb09-196b-456b-bb80-d0dfc0ad010e
📒 Files selected for processing (19)
app/server/appsmith-plugins/pom.xmlapp/server/appsmith-plugins/seaTablePlugin/pom.xmlapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTableErrorMessages.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/createRow.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/deleteRow.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/getRow.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/listRows.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/listTables.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/root.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/sqlQuery.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/updateRow.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/form.jsonapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/plugin.propertiesapp/server/appsmith-plugins/seaTablePlugin/src/main/resources/setting.jsonapp/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
...lugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.java
Show resolved
Hide resolved
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
Show resolved
Hide resolved
- Use MessageFormat in SeaTablePluginError.getMessage() for consistency with PostgresPluginError pattern - Add defensive null checks in fetchAccessToken() to prevent NPE when URL or authentication is missing - Use Appsmith-hosted icon URL instead of external seatable.com favicon - Make migration changeset idempotent by fetching persisted plugin on duplicate key
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (2)
442-450: Redundant validation in command methods.
validateCommandInputsat line 209 already validatestableNameforLIST_ROWS, butexecuteListRowsre-validates at lines 446-450. Same pattern repeats in other command methods. Consider removing the duplicate checks in command methods sincevalidateCommandInputsis called first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 442 - 450, The executeListRows method redundantly re-checks tableName even though validateCommandInputs already validates TABLE_NAME for the LIST_ROWS command; remove the duplicate validation block in executeListRows (the String tableName = getDataValueSafelyFromFormData(...) plus the StringUtils.isBlank check and Mono.error) and rely on the previously-run validateCommandInputs to enforce required inputs; ensure executeListRows continues to use getDataValueSafelyFromFormData(TABLE_NAME, STRING_TYPE) to read the value but without throwing the same AppsmithPluginException there.
746-760: Consider reusingbuildRequestfor consistency.
getStructurecreates a WebClient inline instead of using the existingbuildRequesthelper, which already handles URL construction and headers. This duplicates code and could drift over time.♻️ Suggested refactor
return fetchAccessToken(datasourceConfiguration) .flatMap(tokenResponse -> { - WebClient client = WebClientUtils.builder() - .exchangeStrategies(EXCHANGE_STRATEGIES) - .build(); - - return client - .get() - .uri(URI.create(tokenResponse.basePath() + "/metadata/")) - .header("Authorization", "Token " + tokenResponse.accessToken()) - .header("Accept", MediaType.APPLICATION_JSON_VALUE) - .retrieve() - .bodyToMono(byte[].class) - .timeout(REQUEST_TIMEOUT); + return buildRequest( + tokenResponse.basePath(), + tokenResponse.accessToken(), + HttpMethod.GET, + "/metadata/") + .retrieve() + .bodyToMono(byte[].class) + .timeout(REQUEST_TIMEOUT); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 746 - 760, The code in getStructure creates a WebClient inline and duplicates header/URL handling; replace the inline WebClient construction with the existing buildRequest helper so URL construction and headers (Authorization/Accept) are consistent: call buildRequest(datasourceConfiguration, tokenResponse) (or the equivalent buildRequest overload that accepts the datasourceConfiguration and tokenResponse) and then perform .get().uri("/metadata/") (or append "/metadata/" to the base path via the helper) followed by .retrieve().bodyToMono(byte[].class).timeout(REQUEST_TIMEOUT) so behavior remains identical but reuses buildRequest for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 250-259: The pattern-matching variable auth introduced in the
instanceof check is out of scope after the if block; move to a separate
declaration so auth is available when calling auth.getPassword(). After
validating datasourceConfiguration and that authentication is a DBAuth, cast or
assign the authentication to a local DBAuth variable (e.g., DBAuth auth =
(DBAuth) datasourceConfiguration.getAuthentication()) before reading
auth.getPassword(), ensuring the validation still returns the Mono.error(...)
when authentication is missing or password is blank; update references around
SeaTablePlugin where serverUrl and apiToken are set.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 442-450: The executeListRows method redundantly re-checks
tableName even though validateCommandInputs already validates TABLE_NAME for the
LIST_ROWS command; remove the duplicate validation block in executeListRows (the
String tableName = getDataValueSafelyFromFormData(...) plus the
StringUtils.isBlank check and Mono.error) and rely on the previously-run
validateCommandInputs to enforce required inputs; ensure executeListRows
continues to use getDataValueSafelyFromFormData(TABLE_NAME, STRING_TYPE) to read
the value but without throwing the same AppsmithPluginException there.
- Around line 746-760: The code in getStructure creates a WebClient inline and
duplicates header/URL handling; replace the inline WebClient construction with
the existing buildRequest helper so URL construction and headers
(Authorization/Accept) are consistent: call
buildRequest(datasourceConfiguration, tokenResponse) (or the equivalent
buildRequest overload that accepts the datasourceConfiguration and
tokenResponse) and then perform .get().uri("/metadata/") (or append "/metadata/"
to the base path via the helper) followed by
.retrieve().bodyToMono(byte[].class).timeout(REQUEST_TIMEOUT) so behavior
remains identical but reuses buildRequest for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62b45ff9-1932-4d33-872f-ea5ec457854d
📒 Files selected for processing (3)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
Comment on lines
+250
to
+259
| if (datasourceConfiguration.getAuthentication() == null | ||
| || !(datasourceConfiguration.getAuthentication() instanceof DBAuth auth) | ||
| || StringUtils.isBlank(auth.getPassword())) { | ||
| return Mono.error(new AppsmithPluginException( | ||
| SeaTablePluginError.ACCESS_TOKEN_ERROR, | ||
| SeaTableErrorMessages.MISSING_API_TOKEN_ERROR_MSG)); | ||
| } | ||
|
|
||
| String serverUrl = datasourceConfiguration.getUrl().trim(); | ||
| String apiToken = auth.getPassword(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if this code compiles - check for pattern variable scope
cd app/server/appsmith-plugins/seaTablePlugin
cat src/main/java/com/external/plugins/SeaTablePlugin.java | grep -A 15 "fetchAccessToken"Repository: appsmithorg/appsmith
Length of output: 3940
🏁 Script executed:
# Get the complete fetchAccessToken method and verify the exact line numbers
cd app/server/appsmith-plugins/seaTablePlugin
# First, find the exact line numbers of the method
grep -n "fetchAccessToken" src/main/java/com/external/plugins/SeaTablePlugin.java | head -5Repository: appsmithorg/appsmith
Length of output: 372
🏁 Script executed:
# Check Java version in pom.xml
cd app/server/appsmith-plugins/seaTablePlugin
grep -A 2 -B 2 "source\|target\|maven.compiler" pom.xml | head -20Repository: appsmithorg/appsmith
Length of output: 604
🏁 Script executed:
# Get exact lines 244-270 from the file
cd app/server/appsmith-plugins/seaTablePlugin
sed -n '244,270p' src/main/java/com/external/plugins/SeaTablePlugin.java | cat -nRepository: appsmithorg/appsmith
Length of output: 1620
🏁 Script executed:
# Find Java compiler version configuration
cd app/server/appsmith-plugins/seaTablePlugin
find . -name "pom.xml" -exec grep -l "source\|target" {} \; | head -3
# Then check the actual values
grep -E "<source>|<target>" pom.xml 2>/dev/null || grep -E "maven.compiler" pom.xmlRepository: appsmithorg/appsmith
Length of output: 73
🏁 Script executed:
# Check for parent pom or Java version config
cd app/server/appsmith-plugins/seaTablePlugin
cat pom.xml | grep -A 5 -B 5 "parent\|properties" | head -30Repository: appsmithorg/appsmith
Length of output: 1123
🏁 Script executed:
# Try to find any .java-version or build config
cd app/server/appsmith-plugins/seaTablePlugin
find . -name ".java-version" -o -name ".sdkmanrc" -o -name "gradle.properties" 2>/dev/nullRepository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check if parent pom defines Java version
cd app/server/appsmith-plugins
cat pom.xml | grep -A 3 -B 3 "source\|target" | head -20Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check the actual code to see if auth is declared elsewhere
cd app/server/appsmith-plugins/seaTablePlugin
sed -n '244,290p' src/main/java/com/external/plugins/SeaTablePlugin.java | cat -nRepository: appsmithorg/appsmith
Length of output: 2900
Pattern variable auth is out of scope on line 259.
The auth variable is introduced by the pattern-matching instanceof on line 251, but Java scoping rules restrict its availability to branches where the pattern is guaranteed to match. Line 259 uses auth.getPassword() outside the if block, where the variable is no longer in scope—this will cause a compilation error.
Declare auth separately after the validation block:
Fix
if (datasourceConfiguration.getAuthentication() == null
|| !(datasourceConfiguration.getAuthentication() instanceof DBAuth auth)
|| StringUtils.isBlank(auth.getPassword())) {
return Mono.error(new AppsmithPluginException(
SeaTablePluginError.ACCESS_TOKEN_ERROR,
SeaTableErrorMessages.MISSING_API_TOKEN_ERROR_MSG));
}
String serverUrl = datasourceConfiguration.getUrl().trim();
+ DBAuth auth = (DBAuth) datasourceConfiguration.getAuthentication();
String apiToken = auth.getPassword();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (datasourceConfiguration.getAuthentication() == null | |
| || !(datasourceConfiguration.getAuthentication() instanceof DBAuth auth) | |
| || StringUtils.isBlank(auth.getPassword())) { | |
| return Mono.error(new AppsmithPluginException( | |
| SeaTablePluginError.ACCESS_TOKEN_ERROR, | |
| SeaTableErrorMessages.MISSING_API_TOKEN_ERROR_MSG)); | |
| } | |
| String serverUrl = datasourceConfiguration.getUrl().trim(); | |
| String apiToken = auth.getPassword(); | |
| if (datasourceConfiguration.getAuthentication() == null | |
| || !(datasourceConfiguration.getAuthentication() instanceof DBAuth auth) | |
| || StringUtils.isBlank(auth.getPassword())) { | |
| return Mono.error(new AppsmithPluginException( | |
| SeaTablePluginError.ACCESS_TOKEN_ERROR, | |
| SeaTableErrorMessages.MISSING_API_TOKEN_ERROR_MSG)); | |
| } | |
| String serverUrl = datasourceConfiguration.getUrl().trim(); | |
| DBAuth auth = (DBAuth) datasourceConfiguration.getAuthentication(); | |
| String apiToken = auth.getPassword(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`
around lines 250 - 259, The pattern-matching variable auth introduced in the
instanceof check is out of scope after the if block; move to a separate
declaration so auth is available when calling auth.getPassword(). After
validating datasourceConfiguration and that authentication is a DBAuth, cast or
assign the authentication to a local DBAuth variable (e.g., DBAuth auth =
(DBAuth) datasourceConfiguration.getAuthentication()) before reading
auth.getPassword(), ensuring the validation still returns the Mono.error(...)
when authentication is missing or password is blank; update references around
SeaTablePlugin where serverUrl and apiToken are set.
- Mark FieldName as final (utility class pattern) - Remove duplicate validation in command methods (already handled by validateCommandInputs) - Reuse buildRequest helper in getStructure instead of inline WebClient
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (1)
753-755:⚠️ Potential issue | 🟠 MajorDo not silently return an empty schema when metadata parsing fails.
Current behavior logs and continues with an empty structure, which hides real API/format failures from users.
Proposed fix
- } catch (IOException e) { - log.error("Failed to parse SeaTable metadata", e); - } + } catch (IOException e) { + throw Exceptions.propagate(new AppsmithPluginException( + SeaTablePluginError.QUERY_EXECUTION_FAILED, + String.format( + SeaTableErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, + "Failed to parse SeaTable metadata response: " + e.getMessage()))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 753 - 755, The catch block in SeaTablePlugin that currently swallows parsing failures ("catch (IOException e) { log.error(\"Failed to parse SeaTable metadata\", e); }") should not return an empty schema silently; instead propagate the failure to the caller by rethrowing a runtime or plugin-specific exception (e.g., wrap and throw an AppsmithException or IllegalStateException with a clear message and the original IOException), or return an explicit error result that callers can surface to users; update the method that parses metadata (the method containing the above catch) to remove silent swallowing and ensure its callers handle or bubble up the thrown exception so API/format failures are visible.
🧹 Nitpick comments (1)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (1)
209-212: PreferMono.empty()overnullfromvalidateCommandInputs.Returning
nullfrom a reactive validator makes control flow harder to follow and easier to break in future edits.Refactor sketch
- Mono<Void> validation = validateCommandInputs(command, formData); - if (validation != null) { - return validation.then(Mono.empty()); - } - - return fetchAccessToken(datasourceConfiguration) + return validateCommandInputs(command, formData) + .then(fetchAccessToken(datasourceConfiguration) .flatMap(tokenResponse -> { String basePath = tokenResponse.basePath(); String accessToken = tokenResponse.accessToken(); return switch (command) { case "LIST_ROWS" -> executeListRows(basePath, accessToken, formData); case "GET_ROW" -> executeGetRow(basePath, accessToken, formData); case "CREATE_ROW" -> executeCreateRow(basePath, accessToken, formData); case "UPDATE_ROW" -> executeUpdateRow(basePath, accessToken, formData); case "DELETE_ROW" -> executeDeleteRow(basePath, accessToken, formData); case "LIST_TABLES" -> executeListTables(basePath, accessToken); case "SQL_QUERY" -> executeSqlQuery(basePath, accessToken, formData); default -> Mono.error(new AppsmithPluginException( AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, "Unknown command: " + command)); }; - }); + })); ... - * Returns a Mono.error if validation fails, or null if validation passes. + * Returns a Mono.error if validation fails, or Mono.empty() if validation passes. */ private Mono<Void> validateCommandInputs(String command, Map<String, Object> formData) { ... - return null; + return Mono.empty(); }Also applies to: 394-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 209 - 212, The validator method validateCommandInputs currently returns null to indicate "no validation error", which is error-prone; change validateCommandInputs to always return a Mono (use Mono.empty() for the no-error case) and update call sites (e.g., the usages inside the blocks that assign Mono<Void> validation and check "if (validation != null)") to stop checking for null and instead chain on the returned Mono (e.g., return validation.then(Mono.empty()) when the Mono is non-empty or simply return validation) — apply the same change to the other usages mentioned around the 394-434 region so all control flow uses Mono.empty() instead of null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 214-231: The code currently calls fetchAccessToken(...) before
validating the incoming command, causing unnecessary network calls for
unsupported commands; update the execute path so you validate/switch on the
command string first and immediately return Mono.error(new
AppsmithPluginException(...)) for unknown commands, and only call
fetchAccessToken(...) when the command matches one of the supported operations
(e.g., LIST_ROWS, GET_ROW, CREATE_ROW, UPDATE_ROW, DELETE_ROW, LIST_TABLES,
SQL_QUERY) so that executeListRows/executeGetRow/... are invoked after obtaining
the token.
---
Duplicate comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 753-755: The catch block in SeaTablePlugin that currently swallows
parsing failures ("catch (IOException e) { log.error(\"Failed to parse SeaTable
metadata\", e); }") should not return an empty schema silently; instead
propagate the failure to the caller by rethrowing a runtime or plugin-specific
exception (e.g., wrap and throw an AppsmithException or IllegalStateException
with a clear message and the original IOException), or return an explicit error
result that callers can surface to users; update the method that parses metadata
(the method containing the above catch) to remove silent swallowing and ensure
its callers handle or bubble up the thrown exception so API/format failures are
visible.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 209-212: The validator method validateCommandInputs currently
returns null to indicate "no validation error", which is error-prone; change
validateCommandInputs to always return a Mono (use Mono.empty() for the no-error
case) and update call sites (e.g., the usages inside the blocks that assign
Mono<Void> validation and check "if (validation != null)") to stop checking for
null and instead chain on the returned Mono (e.g., return
validation.then(Mono.empty()) when the Mono is non-empty or simply return
validation) — apply the same change to the other usages mentioned around the
394-434 region so all control flow uses Mono.empty() instead of null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05fad9d7-f907-4a1c-8b1c-7c013c20c237
📒 Files selected for processing (2)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.javaapp/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Outdated
Show resolved
Hide resolved
- Fail fast on unsupported commands before token exchange - Use Mono.empty() instead of null in validateCommandInputs - Propagate metadata parsing errors instead of silently returning empty schema
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (1)
209-236: Centralize command definitions to avoid drift.
LIST_ROWS/GET_ROW/...are duplicated across the supported set and switch cases (Line 209–212 and Line 225–236). This is easy to desync when commands evolve.Refactor sketch
+ private static final Set<String> SUPPORTED_COMMANDS = Set.of( + "LIST_ROWS", "GET_ROW", "CREATE_ROW", "UPDATE_ROW", + "DELETE_ROW", "LIST_TABLES", "SQL_QUERY"); - Set<String> supportedCommands = Set.of( - "LIST_ROWS", "GET_ROW", "CREATE_ROW", "UPDATE_ROW", - "DELETE_ROW", "LIST_TABLES", "SQL_QUERY"); - if (!supportedCommands.contains(command)) { + if (!SUPPORTED_COMMANDS.contains(command)) { return Mono.error(new AppsmithPluginException( AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, "Unknown command: " + command)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java` around lines 209 - 236, The command strings are duplicated between the supportedCommands Set and the switch in the execute flow causing potential drift; centralize these definitions by creating a single source of truth (e.g., an enum or a static unmodifiable list) and use it both to build supportedCommands and in the command dispatch (replace literal strings in the Set.of(...) and the switch cases with references to the enum/constants). Update validateCommandInputs, the contains check, and the switch (the block that calls executeListRows, executeGetRow, executeCreateRow, executeUpdateRow, executeDeleteRow, executeListTables, executeSqlQuery) to reference the centralized command identifiers so additions/removals only change in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 719-725: The code in SeaTablePlugin currently treats
missing/invalid metadata.tables as a successful empty schema by returning
structure; instead, update the handling in the method inside class
SeaTablePlugin (where metadata is read — e.g., the method that builds/returns
`structure`) to surface the error: if `metadata == null` or
`metadata.get("tables")` is missing or not an array, log a clear error including
the metadata contents and throw an appropriate runtime/checked exception (e.g.,
IllegalStateException or a Plugin-specific exception) rather than returning
`structure`, so upstream callers see schema discovery failures instead of silent
empty schemas.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 209-236: The command strings are duplicated between the
supportedCommands Set and the switch in the execute flow causing potential
drift; centralize these definitions by creating a single source of truth (e.g.,
an enum or a static unmodifiable list) and use it both to build
supportedCommands and in the command dispatch (replace literal strings in the
Set.of(...) and the switch cases with references to the enum/constants). Update
validateCommandInputs, the contains check, and the switch (the block that calls
executeListRows, executeGetRow, executeCreateRow, executeUpdateRow,
executeDeleteRow, executeListTables, executeSqlQuery) to reference the
centralized command identifiers so additions/removals only change in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d096474-aaac-4765-ae9b-3d27f4785d0a
📒 Files selected for processing (1)
app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Comment on lines
+719
to
+725
| if (metadata == null) { | ||
| return structure; | ||
| } | ||
| JsonNode tablesNode = metadata.get("tables"); | ||
| if (tablesNode == null || !tablesNode.isArray()) { | ||
| return structure; | ||
| } |
Contributor
There was a problem hiding this comment.
Malformed metadata is silently treated as “empty schema”.
At Line 719 and Line 723, returning an empty structure for missing/invalid metadata.tables hides upstream contract/API issues and makes schema discovery failures look like success.
Proposed fix
- JsonNode metadata = json.get("metadata");
- if (metadata == null) {
- return structure;
- }
- JsonNode tablesNode = metadata.get("tables");
- if (tablesNode == null || !tablesNode.isArray()) {
- return structure;
- }
+ JsonNode metadata = json.get("metadata");
+ if (metadata == null) {
+ throw Exceptions.propagate(new AppsmithPluginException(
+ SeaTablePluginError.QUERY_EXECUTION_FAILED,
+ String.format(
+ SeaTableErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG,
+ "Missing `metadata` in SeaTable metadata response")));
+ }
+ JsonNode tablesNode = metadata.get("tables");
+ if (tablesNode == null || !tablesNode.isArray()) {
+ throw Exceptions.propagate(new AppsmithPluginException(
+ SeaTablePluginError.QUERY_EXECUTION_FAILED,
+ String.format(
+ SeaTableErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG,
+ "Invalid `metadata.tables` in SeaTable metadata response")));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`
around lines 719 - 725, The code in SeaTablePlugin currently treats
missing/invalid metadata.tables as a successful empty schema by returning
structure; instead, update the handling in the method inside class
SeaTablePlugin (where metadata is read — e.g., the method that builds/returns
`structure`) to surface the error: if `metadata == null` or
`metadata.get("tables")` is missing or not an array, log a clear error including
the metadata contents and throw an appropriate runtime/checked exception (e.g.,
IllegalStateException or a Plugin-specific exception) rather than returning
`structure`, so upstream callers see schema discovery failures instead of silent
empty schemas.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
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
1 participant
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.
Summary
Features
/api/v2/dtables/{uuid}/rows//api/v2/dtables/{uuid}/rows/{row_id}//api/v2/dtables/{uuid}/rows//api/v2/dtables/{uuid}/rows//api/v2/dtables/{uuid}/rows//api/v2/dtables/{uuid}/metadata//api/v2/dtables/{uuid}/sql/Implementation Details
PluginExecutor<Void>(stateless HTTP, no persistent connection)getStructure()via metadata endpointVerified against live SeaTable instance
All API operations have been tested and verified against a live SeaTable server:
dtable_serveranddtable_uuidreturned correctlyconvert_keys=truereturns column names, pagination vialimit/startworksconvert_keys=truerow_idsandfirst_rowreturned{"success": true}confirmedconvert_keys=truein body works, results and metadata returnedTest plan
dtable_serverURL handling verifiedReferences
Summary by CodeRabbit