codeant-ai-for-open-source[bot] commented on code in PR #36880:
URL: https://github.com/apache/superset/pull/36880#discussion_r3474436909
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -940,7 +967,15 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps>
= ({
}
const errors = await getValidation(dbToUpdate, true);
- if (!isEmpty(validationErrors) || errors?.length) {
+ // ``getValidation`` returns ``[]`` on success, a field-keyed object
+ // for blocking errors (e.g. the duplicate ``database_name`` check),
+ // and ``null`` for stale or unexpected responses. During save we
+ // cannot proceed without a usable result, so treat ``null`` as
+ // blocking too — only ``[]`` is a clean pass.
+ const hasReturnedErrors =
+ errors === null ||
+ (Array.isArray(errors) ? errors.length > 0 : !isEmpty(errors));
+ if (!isEmpty(validationErrors) || hasReturnedErrors) {
Review Comment:
**Suggestion:** The save guard mixes the fresh `getValidation` result with
the stale `validationErrors` React state. Because state updates are async, a
previously set error object can still be present right after a successful
validation call, causing valid submissions to be rejected with a false
"Connection failed" toast. Base the decision on the returned validation result
from this call (or await state reconciliation) instead of reading possibly
stale state in the same tick. [stale reference]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Edit-database saves can fail despite passing server validation.
- ⚠️ Users see generic failure toast without actionable field errors.
- ⚠️ Dynamic-form database configuration updates may be blocked.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an existing dynamically-configured database in edit mode so it uses
`DatabaseModal` in edit path
(`superset-frontend/src/features/databases/DatabaseModal/index.tsx:635` with
`isEditMode`
true and `configuration_method === ConfigurationMethod.DynamicForm`).
2. Trigger a validation error via the validate-parameters API (for example,
by causing a
duplicate `database_name` so `/api/v1/database/validate_parameters/` returns
an
`INVALID_PAYLOAD_SCHEMA_ERROR`), which is parsed and stored into
`validationErrors` by
`useDatabaseValidation()` in
`superset-frontend/src/views/CRUD/hooks.ts:816-943`.
3. Without blurring the offending field in a way that clears
`validationErrors` (e.g. edit
the `Display Name` field, which uses `displayField` in
`CommonParameters.tsx:270-292`
wired to `changeMethods.onChange`, and in the modal this is mapped to
`handleTextChange`
that *does not* call `handleClearValidationErrors`), click the primary
"Save" button in
the edit modal (`renderEditModalFooter` in `index.tsx:1436-1457`, wired
through `Modal`'s
`onHandledPrimaryAction={onSave}` at `index.tsx:31-33`).
4. In `onSave` (`index.tsx:937-1138`), the code clones `dbToUpdate`, then
calls `const
errors = await getValidation(dbToUpdate, true);` at `index.tsx:969`, which
hits
`/api/v1/database/validate_parameters/` via `getValidation` in
`hooks.ts:824-843`. If the
updated config is now valid, `getValidation` returns `[]`, but inside the
same render the
closure's `validationErrors` variable still holds the *previous* non-empty
error object.
`hasReturnedErrors` becomes `false`, yet `!isEmpty(validationErrors)` is
`true`, so the
guard `if (!isEmpty(validationErrors) || hasReturnedErrors) { ... }` at
`index.tsx:975-979` fires, the user sees "Connection failed, please check
your connection
settings." and the save is aborted even though the latest validation call
succeeded.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d42a00be72744bf1978dd1f8e08992b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d42a00be72744bf1978dd1f8e08992b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/index.tsx
**Line:** 975:978
**Comment:**
*Stale Reference: The save guard mixes the fresh `getValidation` result
with the stale `validationErrors` React state. Because state updates are async,
a previously set error object can still be present right after a successful
validation call, causing valid submissions to be rejected with a false
"Connection failed" toast. Base the decision on the returned validation result
from this call (or await state reconciliation) instead of reading possibly
stale state in the same tick.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36880&comment_hash=e25205e7e7146e25523abed7566e7fb2b18f2de04e2f6ba653de4ef767bd6192&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36880&comment_hash=e25205e7e7146e25523abed7566e7fb2b18f2de04e2f6ba653de4ef767bd6192&reaction=dislike'>👎</a>
##########
superset/databases/schemas.py:
##########
@@ -443,6 +443,24 @@ class Meta: # pylint: disable=too-few-public-methods
required=True,
metadata={"description": configuration_method_description},
)
+ ssh_tunnel = fields.Nested("DatabaseSSHTunnelValidation", allow_none=True)
+
+
+class DatabaseSSHTunnelValidation(Schema):
+ """SSH Tunnel schema for validation.
+
+ Allows partial data without strict authentication requirements.
+ """
+
+ id = fields.Integer(
+ allow_none=True, metadata={"description": "SSH Tunnel ID (for
updates)"}
+ )
+ server_address = fields.String(allow_none=True)
+ server_port = fields.Integer(allow_none=True)
+ username = fields.String(allow_none=True)
+ password = fields.String(required=False, allow_none=True)
+ private_key = fields.String(required=False, allow_none=True)
+ private_key_password = fields.String(required=False, allow_none=True)
Review Comment:
**Suggestion:** The new validation schema for `ssh_tunnel` is missing the
existing `server_host_key` field, so validation requests for databases that
already have host-key verification configured will fail with an unknown nested
field error before command-level validation runs. This breaks edit/validate
flows for those databases and can also drop the host-key value from validation
payload handling. Add `server_host_key` to `DatabaseSSHTunnelValidation` (and
keep nested unknown-handling aligned with expected payloads) so the validate
API remains compatible with existing SSH tunnel data. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ /api/v1/database/validate_parameters fails for SSH tunnels.
- ⚠️ Database edit modal validation fails with host-key field.
- ⚠️ Host-key value ignored in validation-time payload handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a database with an SSH tunnel that includes a host key, so
`server_host_key`
is stored via the model at `superset/databases/ssh_tunnel/models.py:16-20`
and exposed in
API responses by the `data` property at
`superset/databases/ssh_tunnel/models.py:37-7`,
which adds `"server_host_key"` to the `ssh_tunnel` JSON when present.
2. Load the database edit modal (or any client using that API), which calls
the database
detail endpoint and receives an `ssh_tunnel` object containing
`server_host_key` shaped
according to `DatabaseSSHTunnel` in `superset/databases/schemas.py:75-80`
and its
`server_host_key` field at `superset/databases/schemas.py:502-509`.
3. Trigger progressive validation by calling the validate endpoint
`/api/v1/database/validate_parameters` (implemented in
`superset/databases/api.py:2045-2051`), sending a payload that includes
`engine`,
`configuration_method`, and an `ssh_tunnel` object copied from the existing
database JSON,
including `ssh_tunnel.server_host_key`.
4. The request body is deserialized by `DatabaseValidateParametersSchema` at
`superset/databases/schemas.py:392-55`, whose `ssh_tunnel` field at line 446
nests
`DatabaseSSHTunnelValidation` (lines 58-72); since
`DatabaseSSHTunnelValidation` defines
only `id`, `server_address`, `server_port`, `username`, `password`,
`private_key`, and
`private_key_password` and omits `server_host_key`, Marshmallow treats
`ssh_tunnel.server_host_key` as an unknown nested field, raises a
`ValidationError` before
`ValidateDatabaseParametersCommand.run()` in
`superset/commands/database/validate.py:6-8`
is reached, and `databases/api.py:11-23` converts this into a 422
`INVALID_PAYLOAD_SCHEMA_ERROR`, breaking validate/edit flows for
SSH-tunneled databases
with host-key verification configured.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2575b27803c64186bcd9a519c66ec277&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2575b27803c64186bcd9a519c66ec277&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/schemas.py
**Line:** 446:463
**Comment:**
*Api Mismatch: The new validation schema for `ssh_tunnel` is missing
the existing `server_host_key` field, so validation requests for databases that
already have host-key verification configured will fail with an unknown nested
field error before command-level validation runs. This breaks edit/validate
flows for those databases and can also drop the host-key value from validation
payload handling. Add `server_host_key` to `DatabaseSSHTunnelValidation` (and
keep nested unknown-handling aligned with expected payloads) so the validate
API remains compatible with existing SSH tunnel data.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36880&comment_hash=15e5a3047ebe093f2635aaf4574c6e3ffa41270c5e35dd785d9659e1c5b65908&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36880&comment_hash=15e5a3047ebe093f2635aaf4574c6e3ffa41270c5e35dd785d9659e1c5b65908&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
