codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4432568216
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `3608687`.
Endpoint and scope defaults now use nullish coalescing (`??`), so an
explicitly cleared empty string is preserved instead of being replaced by the
displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username,
auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A
second
+ copy of these rules would eventually reject a valid key pair for carrying
no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not
password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half
filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its
rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then
silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential
configured".
+ Accepts the value as a dict or as the JSON string the connection dialog
posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field
validators
+ that have no connection, and therefore no engine spec, in hand. An engine
that
+ overrides ``validate_embedded_credentials`` to accept a shape the
canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real
problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon
as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key !=
"auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `3608687`.
Credential completeness now delegates to `validate_credentials(value)`, with
the helper documenting that callers use the engine’s query-time validator when
the connection is known.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
