codeant-ai-for-open-source[bot] commented on code in PR #37009:
URL: https://github.com/apache/superset/pull/37009#discussion_r2675545370
##########
superset/views/base.py:
##########
@@ -479,7 +479,7 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
auth_type = app.config["AUTH_TYPE"]
auth_user_registration = app.config["AUTH_USER_REGISTRATION"]
frontend_config["AUTH_USER_REGISTRATION"] = auth_user_registration
- should_show_recaptcha = auth_user_registration and (auth_type !=
AUTH_OAUTH)
+ should_show_recaptcha = auth_user_registration and (auth_type not in
[AUTH_OAUTH, AUTH_LDAP])
Review Comment:
**Suggestion:** Type-safety and missing-constant handling: the one-line
expression computing `should_show_recaptcha` assumes `AUTH_OAUTH` and
`AUTH_LDAP` are present and that `auth_type`'s type matches those constants;
normalize and guard comparisons so missing constants or mismatched types don't
produce incorrect results. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
# Build a tuple of available auth constants and perform a guarded
comparison.
_auth_constants = tuple(c for c in (AUTH_OAUTH, AUTH_LDAP) if c is not
None)
if _auth_constants:
should_show_recaptcha = bool(auth_user_registration) and (auth_type
not in _auth_constants)
else:
# If constants aren't available, fall back to only checking
registration flag
should_show_recaptcha = bool(auth_user_registration)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If you apply the import fallback above and AUTH_OAUTH/AUTH_LDAP can be None,
this one-liner silently relies on those values being valid. Building the
list/tuple of only-present constants and guarding the comparison is safer and
avoids surprising results when constants are absent or of unexpected types.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 482:482
**Comment:**
*Logic Error: Type-safety and missing-constant handling: the one-line
expression computing `should_show_recaptcha` assumes `AUTH_OAUTH` and
`AUTH_LDAP` are present and that `auth_type`'s type matches those constants;
normalize and guard comparisons so missing constants or mismatched types don't
produce incorrect results.
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.
```
</details>
##########
superset/views/base.py:
##########
@@ -35,7 +35,7 @@
)
from flask_appbuilder import BaseView, Model, ModelView
from flask_appbuilder.actions import action
-from flask_appbuilder.const import AUTH_OAUTH
+from flask_appbuilder.const import AUTH_LDAP, AUTH_OAUTH
Review Comment:
**Suggestion:** Import failure risk: directly importing `AUTH_LDAP` and
`AUTH_OAUTH` from `flask_appbuilder.const` can raise an ImportError or
AttributeError on different Flask-AppBuilder versions where these names may not
be exported; perform a robust import with a fallback to avoid breaking
application start-up. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
try:
from flask_appbuilder.const import AUTH_LDAP, AUTH_OAUTH
except Exception:
# Fallback to tolerate FAB versions that don't export these names
directly.
# Use getattr on the module to avoid ImportError/AttributeError at
import time.
from flask_appbuilder import const as _fab_const
AUTH_OAUTH = getattr(_fab_const, "AUTH_OAUTH", None)
AUTH_LDAP = getattr(_fab_const, "AUTH_LDAP", None)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a reasonable defensive change: if a downstream environment uses a FAB
variation that doesn't export those names directly, the current import would
raise at import time and crash the app. The proposed fallback using getattr
prevents that. The suggestion is actionable and fixes a real compatibility
failure mode.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 38:38
**Comment:**
*Possible Bug: Import failure risk: directly importing `AUTH_LDAP` and
`AUTH_OAUTH` from `flask_appbuilder.const` can raise an ImportError or
AttributeError on different Flask-AppBuilder versions where these names may not
be exported; perform a robust import with a fallback to avoid breaking
application start-up.
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.
```
</details>
--
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]
