madhushreeag commented on PR #44365: URL: https://github.com/apache/superset/pull/44365#issuecomment-5718946172
> ### Code Review Agent Run #0cd991 > Actionable Suggestions - 1 > Additional Suggestions - 3 > * superset/security/api.py - 1 > > * Missing @safe on consume · [Line 337-341](https://github.com/apache/superset/pull/44365/files#diff-ddc89915e8af096a98449594cef72e9d33b26bfd37de6b549fd677000a24ee9bR337) > The GET consume endpoint is missing `@safe`, which the sibling POST `login_token` and every other endpoint in this class use. An exception escaping `login_token_utils.consume()` or the `@transaction()` commit would propagate to Flask's default HTML 500 handler instead of the JSON error response the rest of the API returns. Add `@safe` between `@statsd_metrics` and `@transaction()`. > * superset/security/login_token.py - 2 > > * Non-dict resolver result 500 · [Line 137-137](https://github.com/apache/superset/pull/44365/files#diff-0da3d0cd8948651aa0640372ca7b83f9ca73fb246daf4413e1b4260a6d0c290eR137) > `resolve_identity` only wraps the resolver call in try/except. If the operator-supplied resolver returns a truthy non-dict (e.g. a string), `userinfo.get(...)` at line 137 raises AttributeError outside the try, which `@safe` turns into a 500 — contradicting the documented 'rejection rather than server error' contract. Guard with `isinstance(userinfo, dict)` and return None. > * Unnecessary purge on mint · [Line 157-157](https://github.com/apache/superset/pull/44365/files#diff-0da3d0cd8948651aa0640372ca7b83f9ca73fb246daf4413e1b4260a6d0c290eR157) > `mint` calls `KeyValueDAO.delete_expired_entries` on every mint, but the key is a fresh `uuid4` that cannot collide with an existing row, so the purge is unnecessary DB work and the comment's unique-constraint rationale does not apply. Consider dropping the call (and comment) to avoid a redundant DELETE on each mint. > > Filtered by Review Rules > Review Details > Bito Usage Guide > AI Code Review powered by [<img alt="Bito Logo" width="50" height="20" src="https://camo.githubusercontent.com/810f9c950a15f18a9a21d4e214820ef360ea3b08a4b5dea95d24529fffd09ed6/68747470733a2f2f6269746f2e61692f77702d636f6e74656e742f75706c6f6164732f323032332f31302f4c6f676f2d4269746f2d426c61636b2d63726f707065642e737667";>](https://bito.ai/) 1. Added` @safe` between `@statsd_metrics` and `@transaction(`) as suggested, so rollback still happens before the error is shaped. 2. `resolve_identity` only guarded the resolver call. Now guarded with `isinstance`, logging the offending type, with three cases added to the rejection matrix (string, list, opaque object). 3. Yes, a unique-constraint rationale doesn't apply to a fresh uuid4. But the call is still worth keeping: with a 60-second TTL these rows turn over far faster than the scheduled prune job runs, and minting is not a hot path, so one indexed DELETE is cheap insurance against dead rows accumulating. I fixed the comment to state the actual reason rather than removing the call. -- 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]
