feat: Add SeaTable as native data source plugin#41628
Closed
christophdb wants to merge 4 commits intoappsmithorg:releasefrom
Closed
feat: Add SeaTable as native data source plugin#41628christophdb wants to merge 4 commits intoappsmithorg:releasefrom
christophdb wants to merge 4 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 #41627
Contributor
WalkthroughAdds SeaTable as a new native plugin: registers the module in the plugins parent POM, adds a Maven module with shaded build, implements a SeaTable plugin executor (token exchange, command handlers, structure discovery), supplies editor/form/setting schemas, tests, plugin metadata, and a DB migration to register and install the plugin. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Appsmith UI
participant Executor as SeaTablePlugin Executor
participant WebClient as WebClient (HTTP)
participant SeaTable as SeaTable API
User->>UI: Configure datasource (Server URL, API Token)
UI->>Executor: datasourceCreate(datasourceConfig)
Executor->>WebClient: GET /api/v2.1/dtable/app-access-token/ (exchange token)
WebClient->>SeaTable: Request token exchange
SeaTable-->>WebClient: Access token + base path
WebClient-->>Executor: Token response
Executor-->>UI: datasourceCreate completed
User->>UI: Run action (e.g., LIST_ROWS)
UI->>Executor: executeParameterized(actionDTO)
Executor->>Executor: perform smart substitution (if enabled)
Executor->>WebClient: GET /api/v2/dtables/{uuid}/rows/ with Access Token
WebClient->>SeaTable: Fetch rows
SeaTable-->>WebClient: Rows JSON
WebClient-->>Executor: Parsed response
Executor-->>UI: ActionExecutionResult
UI-->>User: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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: 3
🧹 Nitpick comments (7)
app/server/appsmith-plugins/seaTablePlugin/src/main/resources/setting.json (1)
20-24: Add a positive-bound validation/default for timeout.Line 22 currently allows a numeric timeout but does not explicitly prevent
0/negative values at the config layer. Adding a minimum constraint (and a sensible default) will avoid invalid runtime timeout configs.Proposed config hardening
{ "label": "Query timeout (in milliseconds)", "subtitle": "Maximum time after which the query will return", "configProperty": "actionConfiguration.timeoutInMillisecond", "controlType": "INPUT_TEXT", - "dataType": "NUMBER" + "dataType": "NUMBER", + "placeholderText": "10000", + "validation": { + "type": "NUMBER", + "params": { + "min": 1 + } + } }🤖 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/resources/setting.json` around lines 20 - 24, The timeout field defined by configProperty "actionConfiguration.timeoutInMillisecond" with controlType "INPUT_TEXT" and dataType "NUMBER" needs a positive minimum and a sensible default; add validation properties (e.g., "default": 30000 and a "min" or "validation" rule > 0) and/or a UI-level constraint so values of 0 or negative numbers are rejected or clamped, and ensure the schema/renderer uses that validation rule for user inputs and deserialization.app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.java (1)
3-15: Consider adding a private constructor.Same utility class pattern as SeaTableErrorMessages. Adding a private constructor prevents instantiation.
♻️ Suggested fix
public class FieldName { + private FieldName() { + // Utility class - prevent instantiation + } + public static final String 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/constants/FieldName.java` around lines 3 - 15, FieldName is a pure constants utility class and should not be instantiable; add a private no-arg constructor to the FieldName class (e.g., private FieldName() { }) to prevent instantiation (optionally throw new AssertionError()) so the class mirrors the SeaTableErrorMessages pattern and enforces non-instantiability.app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTableErrorMessages.java (1)
3-19: Consider adding a private constructor.This is a utility class with only static fields. Adding a private constructor prevents accidental instantiation.
♻️ Suggested fix
public class SeaTableErrorMessages { + private SeaTableErrorMessages() { + // Utility class - prevent instantiation + } + public static final String MISSING_SERVER_URL_ERROR_MSG = "Missing SeaTable server URL.";🤖 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/exceptions/SeaTableErrorMessages.java` around lines 3 - 19, The SeaTableErrorMessages utility class should prevent instantiation: add a private constructor to the SeaTableErrorMessages class (e.g., private SeaTableErrorMessages() { throw new AssertionError("No instantiation"); } or an empty private constructor) so the class cannot be instantiated accidentally; update the SeaTableErrorMessages class to include this private constructor.app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.java (1)
55-73: Replace delegation with direct implementation to align with other plugin error classes.SeaTablePluginError is the only plugin using
BasePluginError.super.getMessage(args)delegation. All other plugins implement these methods directly—useMessageFormat.format()forgetMessage()andreplacePlaceholderWithValue()for downstream methods:♻️ Proposed fix
`@Override` public String getMessage(Object... args) { - return BasePluginError.super.getMessage(args); + return new java.text.MessageFormat(this.message).format(args); } `@Override` public String getErrorType() { return this.errorType.toString(); } `@Override` public String getDownstreamErrorMessage(Object... args) { - return BasePluginError.super.getDownstreamErrorMessage(args); + return replacePlaceholderWithValue(this.downstreamErrorMessage, args); } `@Override` public String getDownstreamErrorCode(Object... args) { - return BasePluginError.super.getDownstreamErrorCode(args); + return replacePlaceholderWithValue(this.downstreamErrorCode, args); }🤖 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/exceptions/SeaTablePluginError.java` around lines 55 - 73, The current SeaTablePluginError methods delegate to BasePluginError.super which is inconsistent with other plugins; change getMessage(Object... args) to directly format the error message using MessageFormat.format(this.message, args), implement getDownstreamErrorMessage(Object... args) and getDownstreamErrorCode(Object... args) to call replacePlaceholderWithValue(this.downstreamErrorMessage, args) and replacePlaceholderWithValue(this.downstreamErrorCode, args) respectively, and keep getErrorType() returning this.errorType.toString(); locate and update the methods getMessage, getDownstreamErrorMessage, and getDownstreamErrorCode in the SeaTablePluginError class to mirror the direct implementations used by other plugin error classes.app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java (2)
308-334: Consider adding request timeout.No timeout is configured on HTTP requests. If SeaTable becomes unresponsive, queries will hang indefinitely.
♻️ Suggested timeout configuration
private Mono<ActionExecutionResult> executeRequest(WebClient.RequestHeadersSpec<?> requestSpec) { return requestSpec .retrieve() .bodyToMono(byte[].class) + .timeout(java.time.Duration.ofSeconds(30)) .map(responseBytes -> {🤖 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 308 - 334, The executeRequest method currently calls requestSpec.retrieve().bodyToMono(...) without a timeout, causing indefinite hangs if SeaTable is unresponsive; modify executeRequest to apply a reactive timeout on the returned Mono (e.g., using .timeout(Duration.ofSeconds(...))) to the chain produced from WebClient.RequestHeadersSpec<?> requestSpec, and add handling for TimeoutException in the onErrorResume path so the ActionExecutionResult sets isExecutionSuccess=false and a clear AppsmithPluginException using SeaTablePluginError.QUERY_EXECUTION_FAILED (or a new timeout-specific error) with a message indicating a request timeout; make the timeout duration configurable (constant or config) and ensure the .subscribeOn(scheduler) remains applied.
240-276: Add timeout to token exchange request.Same as the earlier comment — this HTTP call should have a timeout to prevent indefinite hangs during datasource connection testing.
♻️ Suggested timeout
return client .get() .uri(URI.create(url)) .header("Authorization", "Token " + apiToken) .header("Accept", MediaType.APPLICATION_JSON_VALUE) .retrieve() .bodyToMono(byte[].class) + .timeout(java.time.Duration.ofSeconds(30)) .map(responseBytes -> {🤖 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 240 - 276, The token-exchange reactive chain (the Mono built from client.get().uri(...).bodyToMono(...) that maps to AccessTokenResponse) needs a request timeout to avoid hanging; add a .timeout(Duration.ofSeconds(<N>)) on the Mono (before .subscribeOn(scheduler)) and handle timeout errors in the onErrorResume by mapping TimeoutException/TimeoutException subclass to an AppsmithPluginException with SeaTablePluginError.ACCESS_TOKEN_ERROR and SeaTableErrorMessages.ACCESS_TOKEN_FETCH_FAILED_ERROR_MSG (or add a specific timeout message if you prefer); ensure you import java.time.Duration and java.util.concurrent.TimeoutException (or use reactor.util.retry.TimeoutException) as needed and keep the existing mapping for other exceptions.app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java (1)
166-171: PreferFieldNameconstants over raw formData key strings in tests.On Line 169 and repeated command payload setup blocks, literal keys (
"command","tableName","rowId","body","sql") make tests brittle if field names change centrally.Proposed refactor
@@ - setDataValueSafelyInFormData(formData, "command", command); + setDataValueSafelyInFormData(formData, FieldName.COMMAND, command); @@ - extra.put("tableName", "Contacts"); + extra.put(FieldName.TABLE_NAME, "Contacts"); @@ - extra.put("rowId", "row1"); + extra.put(FieldName.ROW_ID, "row1"); @@ - extra.put("body", "{\"Name\": \"Charlie\", \"Age\": 35}"); + extra.put(FieldName.BODY, "{\"Name\": \"Charlie\", \"Age\": 35}");Also applies to: 269-271, 297-299, 324-325, 351-353, 378-379, 427-427
🤖 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 166 - 171, The test uses raw formData keys (e.g., "command", "tableName", "rowId", "body", "sql") inside createActionConfig and other test setup blocks; replace these string literals with the centralized FieldName constants (e.g., FieldName.COMMAND, FieldName.TABLE_NAME, FieldName.ROW_ID, FieldName.BODY, FieldName.SQL) when calling setDataValueSafelyInFormData and when building formData so tests remain resilient to field-name changes; update createActionConfig and the other test payload setup locations (where setDataValueSafelyInFormData is used) to import and reference the appropriate FieldName members instead of hard-coded strings.
🤖 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 247-266: The JSON parsing in the mapping lambda inside
SeaTablePlugin.java can NPE if json.get("access_token"), "dtable_uuid" or
"dtable_server" are missing; update the lambda that constructs
AccessTokenResponse to validate that json.get(...) for those three fields is
non-null and non-empty before calling asText(), and if any are missing throw the
existing AppsmithPluginException using SeaTablePluginError.ACCESS_TOKEN_ERROR
and SeaTableErrorMessages.ACCESS_TOKEN_FETCH_FAILED_ERROR_MSG so error handling
remains consistent (preserve the catch for IOException for readTree).
- Around line 647-668: The loop that builds table/column metadata can NPE when
tableNode.get("name"), colNode.get("name") or colNode.get("type") return null;
update the parsing in SeaTablePlugin (the block iterating over tablesNode and
inner columnsNode/colNode) to defensively check for null/absent fields before
calling asText() (e.g., verify tableNode.hasNonNull("name") and
colNode.hasNonNull("name")/("type")), skip or fallback to a safe default if
missing, and log a warning when skipping malformed table/column entries so
DatasourceStructure.Table/DatasourceStructure.Column construction never receives
null values.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java`:
- Around line 228-472: The tests (e.g., testListRows, testGetRow, testCreateRow,
testUpdateRow, testDeleteRow, testListTables, testSqlQuery, testGetStructure and
testTestDatasource_invalidToken) only assert success flags from
pluginExecutor.executeParameterized / pluginExecutor.getStructure and never
verify the actual outbound HTTP request; after enqueueAccessTokenResponse() and
the action invocation call pluginExecutor.* add mockWebServer.takeRequest()
assertions to validate HTTP method, request path/query, required headers
(Authorization, Content-Type) and request body where applicable (e.g., rowId in
GET/DELETE, JSON body in CREATE/UPDATE, sql param in SQL_QUERY). Use the
MockWebServer.takeRequest() result in each test to assert expected values so the
tests assert the HTTP contract rather than only response success.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/constants/FieldName.java`:
- Around line 3-15: FieldName is a pure constants utility class and should not
be instantiable; add a private no-arg constructor to the FieldName class (e.g.,
private FieldName() { }) to prevent instantiation (optionally throw new
AssertionError()) so the class mirrors the SeaTableErrorMessages pattern and
enforces non-instantiability.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTableErrorMessages.java`:
- Around line 3-19: The SeaTableErrorMessages utility class should prevent
instantiation: add a private constructor to the SeaTableErrorMessages class
(e.g., private SeaTableErrorMessages() { throw new AssertionError("No
instantiation"); } or an empty private constructor) so the class cannot be
instantiated accidentally; update the SeaTableErrorMessages class to include
this private constructor.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.java`:
- Around line 55-73: The current SeaTablePluginError methods delegate to
BasePluginError.super which is inconsistent with other plugins; change
getMessage(Object... args) to directly format the error message using
MessageFormat.format(this.message, args), implement
getDownstreamErrorMessage(Object... args) and getDownstreamErrorCode(Object...
args) to call replacePlaceholderWithValue(this.downstreamErrorMessage, args) and
replacePlaceholderWithValue(this.downstreamErrorCode, args) respectively, and
keep getErrorType() returning this.errorType.toString(); locate and update the
methods getMessage, getDownstreamErrorMessage, and getDownstreamErrorCode in the
SeaTablePluginError class to mirror the direct implementations used by other
plugin error classes.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 308-334: The executeRequest method currently calls
requestSpec.retrieve().bodyToMono(...) without a timeout, causing indefinite
hangs if SeaTable is unresponsive; modify executeRequest to apply a reactive
timeout on the returned Mono (e.g., using .timeout(Duration.ofSeconds(...))) to
the chain produced from WebClient.RequestHeadersSpec<?> requestSpec, and add
handling for TimeoutException in the onErrorResume path so the
ActionExecutionResult sets isExecutionSuccess=false and a clear
AppsmithPluginException using SeaTablePluginError.QUERY_EXECUTION_FAILED (or a
new timeout-specific error) with a message indicating a request timeout; make
the timeout duration configurable (constant or config) and ensure the
.subscribeOn(scheduler) remains applied.
- Around line 240-276: The token-exchange reactive chain (the Mono built from
client.get().uri(...).bodyToMono(...) that maps to AccessTokenResponse) needs a
request timeout to avoid hanging; add a .timeout(Duration.ofSeconds(<N>)) on the
Mono (before .subscribeOn(scheduler)) and handle timeout errors in the
onErrorResume by mapping TimeoutException/TimeoutException subclass to an
AppsmithPluginException with SeaTablePluginError.ACCESS_TOKEN_ERROR and
SeaTableErrorMessages.ACCESS_TOKEN_FETCH_FAILED_ERROR_MSG (or add a specific
timeout message if you prefer); ensure you import java.time.Duration and
java.util.concurrent.TimeoutException (or use
reactor.util.retry.TimeoutException) as needed and keep the existing mapping for
other exceptions.
In `@app/server/appsmith-plugins/seaTablePlugin/src/main/resources/setting.json`:
- Around line 20-24: The timeout field defined by configProperty
"actionConfiguration.timeoutInMillisecond" with controlType "INPUT_TEXT" and
dataType "NUMBER" needs a positive minimum and a sensible default; add
validation properties (e.g., "default": 30000 and a "min" or "validation" rule >
0) and/or a UI-level constraint so values of 0 or negative numbers are rejected
or clamped, and ensure the schema/renderer uses that validation rule for user
inputs and deserialization.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java`:
- Around line 166-171: The test uses raw formData keys (e.g., "command",
"tableName", "rowId", "body", "sql") inside createActionConfig and other test
setup blocks; replace these string literals with the centralized FieldName
constants (e.g., FieldName.COMMAND, FieldName.TABLE_NAME, FieldName.ROW_ID,
FieldName.BODY, FieldName.SQL) when calling setDataValueSafelyInFormData and
when building formData so tests remain resilient to field-name changes; update
createActionConfig and the other test payload setup locations (where
setDataValueSafelyInFormData is used) to import and reference the appropriate
FieldName members instead of hard-coded strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c728e53-23a5-4676-9278-b940f8a7b3fb
📒 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
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Show resolved
Hide resolved
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Show resolved
Hide resolved
...r/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java
Show resolved
Hide resolved
- 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()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 9-76: SeaTablePluginError is missing the
BasePluginError#getErrorAction() implementation; add a getErrorAction() method
to the enum that returns a valid default error action to satisfy the interface.
Update SeaTablePluginError by adding a private final ErrorAction errorAction
field (or similar type expected by BasePluginError), initialize it in the
existing constructor to a sensible default (e.g., ErrorAction.NO_ACTION or the
API's none/default value), and implement public getErrorAction() to return that
field (or its string form if the interface expects String). Ensure method name
exactly matches getErrorAction() from BasePluginError so the enum compiles.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java`:
- Around line 204-221: The code calls fetchAccessToken(...) before validating
command-specific form inputs, causing unnecessary auth/network calls; move or
add pre-checks that validate required fields based on the command string (e.g.,
ensure tableName is present for LIST_ROWS, LIST_TABLES, CREATE_ROW, UPDATE_ROW,
DELETE_ROW; ensure rowId for GET_ROW, UPDATE_ROW, DELETE_ROW; ensure sql for
SQL_QUERY) and return Mono.error(...) with the appropriate
AppsmithPluginException before calling fetchAccessToken; update the dispatch in
the method that currently uses fetchAccessToken(...).flatMap(...) so it first
performs these validations (refer to the command variable and methods
executeListRows, executeGetRow, executeCreateRow, executeUpdateRow,
executeDeleteRow, executeListTables, executeSqlQuery) and only calls
fetchAccessToken when validations pass.
- Around line 694-705: The DatasourceStructure returned by the code path in
SeaTablePlugin (variable structure of type DatasourceStructure) can have a null
tables field when metadata or tables are missing; ensure structure.tables is
initialized to an empty List before any early returns—e.g., create and call
structure.setTables(new ArrayList<>()) (or construct DatasourceStructure with an
empty list) prior to the checks that return on missing metadata/tables so
downstream consumers never see a null tables value.
- Around line 156-170: smartSubstitutionOfBindings can NPE when
executeActionDTO.getParams() is null; before calling smartSubstitutionOfBindings
(in the BODY handling block in SeaTablePlugin.java), guard the evaluated params
by replacing a null executeActionDTO.getParams() with an empty list (or
Collections.emptyList()) so the method never receives null, or alternatively
make smartSubstitutionOfBindings accept/handle null safely; reference symbols:
smartSubstitutionOfBindings, executeActionDTO.getParams(), BODY,
getDataValueSafelyFromFormData.
In
`@app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java`:
- Around line 37-41: The MockWebServer instance and serverUrl must be reset per
test to avoid leaking recorded requests across tests; change the static fields
mockWebServer and serverUrl to instance fields and initialize a fresh
MockWebServer in a `@BeforeEach` method (start it and set serverUrl =
mockWebServer.url("/").toString()), and shut it down in an `@AfterEach` method
(mockWebServer.shutdown()) so each test gets an isolated request queue; keep
SeaTablePlugin.SeaTablePluginExecutor pluginExecutor and ObjectMapper
objectMapper as needed but update tests to use the instance serverUrl and
mockWebServer rather than relying on static shared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ec421c9-aad7-4a7f-a399-f549147aa5f7
📒 Files selected for processing (6)
app/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/setting.jsonapp/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/seaTablePlugin/src/main/resources/setting.json
...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
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Show resolved
Hide resolved
...erver/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.java
Show resolved
Hide resolved
...r/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java
Outdated
Show resolved
Hide resolved
- 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 244-247: In fetchAccessToken, guard against null or malformed
datasourceConfiguration and authentication before dereferencing: check
datasourceConfiguration != null and its getUrl() != null before calling
getUrl().trim(), and ensure datasourceConfiguration.getAuthentication() is
non-null and an instance of DBAuth before casting to DBAuth and accessing
getPassword(); if any check fails, return a Mono.error(...) using the plugin's
structured error handling (preserve existing error type/message format) instead
of letting an NPE/CCE propagate.
- Around line 214-230: The code currently calls
fetchAccessToken(datasourceConfiguration) before checking the command, causing
unknown commands to surface as auth/network errors; update the logic in the
method containing the switch so you validate the command and return a Mono.error
for unknown commands immediately (before invoking fetchAccessToken), e.g.,
perform the switch or a command-lookup first and only call fetchAccessToken when
the command maps to one of the handlers (executeListRows, executeGetRow,
executeCreateRow, executeUpdateRow, executeDeleteRow, executeListTables,
executeSqlQuery); ensure the default branch returns the AppsmithPluginException
without triggering fetchAccessToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f055280-cb26-42a9-9423-c2097550c23e
📒 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-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-plugins/seaTablePlugin/src/test/java/com/external/plugins/SeaTablePluginTest.java
- app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/exceptions/SeaTablePluginError.java
Comment on lines
+214
to
+230
| return 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)); | ||
| }; |
Contributor
There was a problem hiding this comment.
Fail fast for unsupported commands before token fetch.
Line 214 still performs fetchAccessToken(...) before rejecting unknown commands, so invalid command input can surface as auth/network failure instead of a local argument error.
Suggested fix
// Validate required fields before making any network calls
Mono<Void> validation = validateCommandInputs(command, formData);
if (validation != null) {
return validation.then(Mono.empty());
}
+
+ Set<String> supportedCommands =
+ Set.of("LIST_ROWS", "GET_ROW", "CREATE_ROW", "UPDATE_ROW", "DELETE_ROW", "LIST_TABLES", "SQL_QUERY");
+ if (!supportedCommands.contains(command)) {
+ return Mono.error(new AppsmithPluginException(
+ AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR,
+ "Unknown command: " + command));
+ }
return fetchAccessToken(datasourceConfiguration)
.flatMap(tokenResponse -> {🤖 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 214 - 230, The code currently calls
fetchAccessToken(datasourceConfiguration) before checking the command, causing
unknown commands to surface as auth/network errors; update the logic in the
method containing the switch so you validate the command and return a Mono.error
for unknown commands immediately (before invoking fetchAccessToken), e.g.,
perform the switch or a command-lookup first and only call fetchAccessToken when
the command maps to one of the handlers (executeListRows, executeGetRow,
executeCreateRow, executeUpdateRow, executeDeleteRow, executeListTables,
executeSqlQuery); ensure the default branch returns the AppsmithPluginException
without triggering fetchAccessToken.
Comment on lines
+244
to
+247
| private Mono<AccessTokenResponse> fetchAccessToken(DatasourceConfiguration datasourceConfiguration) { | ||
| String serverUrl = datasourceConfiguration.getUrl().trim(); | ||
| DBAuth auth = (DBAuth) datasourceConfiguration.getAuthentication(); | ||
| String apiToken = auth.getPassword(); |
Contributor
There was a problem hiding this comment.
Guard datasource/auth before dereferencing in token flow.
Line 245 and Line 247 can throw NPE/CCE (getUrl().trim(), cast to DBAuth) when config is malformed, bypassing your structured plugin error handling.
Suggested fix
private Mono<AccessTokenResponse> fetchAccessToken(DatasourceConfiguration datasourceConfiguration) {
+ if (datasourceConfiguration == null || StringUtils.isBlank(datasourceConfiguration.getUrl())) {
+ return Mono.error(new AppsmithPluginException(
+ SeaTablePluginError.ACCESS_TOKEN_ERROR,
+ SeaTableErrorMessages.MISSING_SERVER_URL_ERROR_MSG));
+ }
+ if (!(datasourceConfiguration.getAuthentication() instanceof DBAuth)
+ || StringUtils.isBlank(((DBAuth) datasourceConfiguration.getAuthentication()).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 244 - 247, In fetchAccessToken, guard against null or malformed
datasourceConfiguration and authentication before dereferencing: check
datasourceConfiguration != null and its getUrl() != null before calling
getUrl().trim(), and ensure datasourceConfiguration.getAuthentication() is
non-null and an instance of DBAuth before casting to DBAuth and accessing
getPassword(); if any check fails, return a Mono.error(...) using the plugin's
structured error handling (preserve existing error type/message format) instead
of letting an NPE/CCE propagate.
SeaTable should appear under "SaaS Integrations" alongside Airtable and Google Sheets, not under "APIs". Also fix documentationLink URL.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java`:
- Around line 680-686: The catch for DuplicateKeyException around
mongoTemplate.insert(plugin) in DatabaseChangelog2 currently still calls
installPluginToAllWorkspaces(mongoTemplate, plugin.getId()), but plugin.getId()
may be null; update the handler to either re-fetch the existing plugin by its
package name (using mongoTemplate.findOne/query by packageName) and pass that
plugin's id to installPluginToAllWorkspaces, or add a null-guard that skips the
install call when plugin.getId() is null; reference the insert call, the catch
block, plugin.getPackageName(), and installPluginToAllWorkspaces to locate and
implement the fix.
- Around line 673-676: The SeaTable plugin registration is missing a datasource
component mapping, so update the Plugin object where
plugin.setType(PluginType.SAAS), plugin.setPackageName("seatable-plugin"),
plugin.setUiComponent("UQIDbEditorForm"), and
plugin.setResponseType(Plugin.ResponseType.JSON) are set to also call
plugin.setDatasourceComponent(...) to wire the datasource/auth form; use a
component consistent with other API-token SAAS plugins (e.g., "AutoForm" or the
project’s OAuth/API token datasource form component) so the datasource UI
(server URL + API token) is shown correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da4db7fa-9a8b-4fc3-a914-98452d1e8833
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
Comment on lines
+673
to
+676
| plugin.setType(PluginType.SAAS); | ||
| plugin.setPackageName("seatable-plugin"); | ||
| plugin.setUiComponent("UQIDbEditorForm"); | ||
| plugin.setResponseType(Plugin.ResponseType.JSON); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify SAAS plugin registration patterns and component wiring in migrations.
rg -n -C4 'setType\(PluginType\.SAAS\)|setUiComponent\(|setDatasourceComponent\(' \
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog1.java \
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
# Verify SeaTable plugin resource files that imply datasource form expectations.
fd -a 'form.json|setting.json|plugin.properties|root.json' app/server/appsmith-plugins/seaTablePlugin | sortRepository: appsmithorg/appsmith
Length of output: 34587
🏁 Script executed:
cat app/server/appsmith-plugins/seaTablePlugin/src/main/resources/setting.jsonRepository: appsmithorg/appsmith
Length of output: 1099
🏁 Script executed:
head -50 app/server/appsmith-plugins/seaTablePlugin/src/main/resources/form.jsonRepository: appsmithorg/appsmith
Length of output: 831
🏁 Script executed:
cat app/server/appsmith-plugins/seaTablePlugin/src/main/resources/plugin.propertiesRepository: appsmithorg/appsmith
Length of output: 221
🏁 Script executed:
# Search for SeaTable plugin references to see what datasourceComponent might be expected
rg -n 'seatable|SeaTable' --type java | grep -i 'datasourceComponent\|form\|component' | head -20Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check if there are any recent changes or additional SeaTable plugin configuration
fd seaTable -t f | head -30Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check what datasourceComponent values are used for API/SAAS plugins with token auth
rg -n 'setDatasourceComponent' --type java -A1 -B1 | head -40Repository: appsmithorg/appsmith
Length of output: 4980
🏁 Script executed:
# Search more broadly for SeaTable in all files
find . -type f \( -name '*.java' -o -name '*.json' -o -name '*.ts' -o -name '*.tsx' \) -exec grep -l -i 'seatable' {} \; 2>/dev/null | head -20Repository: appsmithorg/appsmith
Length of output: 758
🏁 Script executed:
# Check SeaTablePlugin.java to see if it references a specific datasource form component
head -100 app/server/appsmith-plugins/seaTablePlugin/src/main/java/com/external/plugins/SeaTablePlugin.javaRepository: appsmithorg/appsmith
Length of output: 4479
🏁 Script executed:
# Check if there are any datasource form references in the sqlQuery.json editor config
cat app/server/appsmith-plugins/seaTablePlugin/src/main/resources/editor/sqlQuery.json | head -40Repository: appsmithorg/appsmith
Length of output: 830
Add datasourceComponent mapping for SeaTable SAAS plugin.
SeaTable's form.json defines datasource-level configuration (Server URL and API Token authentication), yet the plugin registration at lines 673-676 lacks setDatasourceComponent(). All comparable SAAS and API plugins in the codebase include explicit datasource component mappings (Google Sheets uses OAuth2DatasourceForm, GraphQL uses RestAPIDatasourceForm). Add a datasourceComponent mapping to ensure the datasource/auth form UI is properly wired. Based on other API token-based plugins, AutoForm or a custom form component should be specified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java`
around lines 673 - 676, The SeaTable plugin registration is missing a datasource
component mapping, so update the Plugin object where
plugin.setType(PluginType.SAAS), plugin.setPackageName("seatable-plugin"),
plugin.setUiComponent("UQIDbEditorForm"), and
plugin.setResponseType(Plugin.ResponseType.JSON) are set to also call
plugin.setDatasourceComponent(...) to wire the datasource/auth form; use a
component consistent with other API-token SAAS plugins (e.g., "AutoForm" or the
project’s OAuth/API token datasource form component) so the datasource UI
(server URL + API token) is shown correctly.
Comment on lines
+680
to
+686
| try { | ||
| mongoTemplate.insert(plugin); | ||
| } catch (DuplicateKeyException e) { | ||
| log.warn(plugin.getPackageName() + " already present in database."); | ||
| } | ||
| installPluginToAllWorkspaces(mongoTemplate, plugin.getId()); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard workspace installation when insert fails.
If Line 681 throws DuplicateKeyException, plugin.getId() can be null, but Line 685 still runs. Re-fetch existing plugin (by package name) or null-guard before installing to workspaces.
Proposed fix
try {
mongoTemplate.insert(plugin);
} catch (DuplicateKeyException e) {
log.warn(plugin.getPackageName() + " already present in database.");
+ plugin = mongoTemplate.findOne(
+ query(where(Plugin.Fields.packageName).is(plugin.getPackageName())),
+ Plugin.class);
}
- installPluginToAllWorkspaces(mongoTemplate, plugin.getId());
+ if (plugin != null && plugin.getId() != null) {
+ installPluginToAllWorkspaces(mongoTemplate, plugin.getId());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java`
around lines 680 - 686, The catch for DuplicateKeyException around
mongoTemplate.insert(plugin) in DatabaseChangelog2 currently still calls
installPluginToAllWorkspaces(mongoTemplate, plugin.getId()), but plugin.getId()
may be null; update the handler to either re-fetch the existing plugin by its
package name (using mongoTemplate.findOne/query by packageName) and pass that
plugin's id to installPluginToAllWorkspaces, or add a null-guard that skips the
install call when plugin.getId() is null; reference the insert call, the catch
block, plugin.getPackageName(), and installPluginToAllWorkspaces to locate and
implement the fix.
Author
|
Reopened as #41629 — the previous PR was automatically closed when the source fork was accidentally deleted. |
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.
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 endpointTest plan
References
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores