codeant-ai-for-open-source[bot] commented on PR #36742: URL: https://github.com/apache/superset/pull/36742#issuecomment-3671205004
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36742/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R159-R166'><strong>Possible Bug</strong></a><br>The filter uses `User.username.not_in(...)`, but SQLAlchemy's Column API provides `notin_(...)`. Calling a non-existent `not_in` attribute will raise an AttributeError when the filter runs, causing the user listing API to error.<br> - [ ] <a href='https://github.com/apache/superset/pull/36742/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R160-R163'><strong>Config access risk</strong></a><br>The code accesses `current_app.config["EXCLUDE_USERS_FROM_LISTS"]` directly. If the config key is missing this will raise a KeyError at runtime. Prefer using `current_app.config.get(...)` to avoid exceptions when the setting is absent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36742/files#diff-353c035aa802221d1e247d70d13599d3952501c165960df8c50f495dd862a0a4R33-R41'><strong>Config patching fragility</strong></a><br>The tests use mocker.patch to replace `superset.security.manager.current_app.config` with a plain dict. Patching the imported `current_app.config` like this can be brittle (may bypass normal Flask app context behavior) and could leak between tests if not restored properly. Consider using patch.dict or an app context to mutate config safely.<br> - [ ] <a href='https://github.com/apache/superset/pull/36742/files#diff-353c035aa802221d1e247d70d13599d3952501c165960df8c50f495dd862a0a4R71-R79'><strong>Brittle SQL string assertions</strong></a><br>Several assertions rely on the exact compiled SQL string and alias ("ab_user.username NOT IN") which can vary across SQLAlchemy versions or engine dialects. These assertions may produce false negatives. Prefer checking for the presence of "NOT IN" and the expected usernames (with quotes), or use a regex / SQLAlchemy expression inspection to be more robust.<br> </td></tr> </table> -- 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]
