🧹 [Code Health Improvement] Simplify verify_access_and_get_folders#805
🧹 [Code Health Improvement] Simplify verify_access_and_get_folders#805abhimehro wants to merge 2 commits into
Conversation
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR SummaryLow Risk Overview Behavior is intended to remain the same, but the access check now delegates JSON parsing and auth-related logging to these helpers, reducing nesting and cyclomatic complexity. Reviewed by Cursor Bugbot for commit d89d876. Configure here. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| def _log_auth_error(code: int, profile_id: str) -> None: | ||
| if code == 401: | ||
| log.critical( | ||
| f"{Colors.FAIL}❌ Authentication Failed: The API Token is invalid.{Colors.ENDC}" | ||
| ) | ||
| log.critical( | ||
| f"{Colors.FAIL} Please check your token at: https://controld.com/account/manage-account{Colors.ENDC}" | ||
| ) | ||
| elif code == 403: | ||
| log.critical( | ||
| "%s🚫 Access Denied: Token lacks permission for Profile %s.%s", | ||
| Colors.FAIL, | ||
| sanitize_for_log(profile_id), | ||
| Colors.ENDC, | ||
| ) | ||
| elif code == 404: | ||
| log.critical( | ||
| f"{Colors.FAIL}🔍 Profile Not Found: The ID '{sanitize_for_log(profile_id)}' does not exist.{Colors.ENDC}" | ||
| ) | ||
| log.critical( | ||
| f"{Colors.FAIL} Please verify the Profile ID from your Control D Dashboard URL.{Colors.ENDC}" | ||
| ) |
There was a problem hiding this comment.
📝 Info: Inconsistent logging style between 401/404 (f-string) and 403 (%-formatting) in _log_auth_error
The _log_auth_error function uses f-string interpolation for the 401 and 404 branches (main.py:1789, main.py:1803) but uses %s-style lazy formatting for the 403 branch (main.py:1796-1800). This inconsistency was inherited from the original inline code in verify_access_and_get_folders, so it's pre-existing rather than introduced by this PR. It's not a bug (both styles work), but it's worth noting for consistency if the author is already touching this code.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| data = resp.json() | ||
|
|
||
| # Ensure we got the expected top-level JSON structure. | ||
| # We defensively validate types here so that unexpected but valid | ||
| # JSON (e.g., a list or a scalar) doesn't cause AttributeError/TypeError | ||
| # and cause the operation to fail unexpectedly. | ||
| if not isinstance(data, dict): | ||
| log.error( | ||
| "Failed to parse folders data: expected JSON object at top level, " | ||
| f"got {type(data).__name__}" | ||
| ) | ||
| return None | ||
|
|
||
| body = data.get("body") | ||
| if not isinstance(body, dict): | ||
| log.error( | ||
| "Failed to parse folders data: expected 'body' to be an object, " | ||
| f"got {type(body).__name__ if body is not None else 'None'}" | ||
| ) | ||
| return None | ||
|
|
||
| folders = body.get("groups", []) | ||
| if not isinstance(folders, list): | ||
| log.error( | ||
| "Failed to parse folders data: expected 'body[\"groups\"]' to be a list, " | ||
| f"got {type(folders).__name__}" | ||
| ) | ||
| return None | ||
|
|
||
| # Only process entries that are dicts and have the required keys. | ||
| result: dict[str, str] = {} | ||
| for f in folders: | ||
| if not isinstance(f, dict): | ||
| # Skip non-dict entries instead of crashing; this protects | ||
| # against partial data corruption or unexpected API changes. | ||
| continue | ||
| name = f.get("group") | ||
| pk = f.get("PK") | ||
| # Skip entries with empty or None values for required fields | ||
| if not name or not pk: | ||
| continue | ||
|
|
||
| pk_str = str(pk) | ||
| if not validate_folder_id(pk_str): | ||
| continue | ||
|
|
||
| result[str(name).strip()] = pk_str | ||
|
|
||
| return result | ||
| return _parse_folders_response(resp.json()) | ||
| except (ValueError, TypeError, AttributeError) as err: | ||
| # As a final safeguard, catch any remaining parsing/shape errors so | ||
| # that a malformed response cannot crash the caller. | ||
| log.error("Failed to parse folders data: %s", sanitize_for_log(err)) | ||
| return None |
There was a problem hiding this comment.
📝 Info: Refactoring correctly preserves original behavior
The extraction of parsing logic into _parse_folders_response and auth error logging into _log_auth_error is a faithful refactor. The call at main.py:1896 (return _parse_folders_response(resp.json())) correctly replaces the old inline parsing, and the except clause at line 1897 still catches ValueError/TypeError/AttributeError from resp.json() (which can throw if the response body isn't valid JSON). The _parse_folders_response function itself doesn't raise these exceptions (it returns None on bad input), so the try/except only guards the resp.json() call, which is the correct behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| def _log_auth_error(code: int, profile_id: str) -> None: | ||
| if code == 401: | ||
| log.critical( | ||
| f"{Colors.FAIL}❌ Authentication Failed: The API Token is invalid.{Colors.ENDC}" | ||
| ) | ||
| log.critical( | ||
| f"{Colors.FAIL} Please check your token at: https://controld.com/account/manage-account{Colors.ENDC}" | ||
| ) | ||
| elif code == 403: | ||
| log.critical( | ||
| "%s🚫 Access Denied: Token lacks permission for Profile %s.%s", | ||
| Colors.FAIL, | ||
| sanitize_for_log(profile_id), | ||
| Colors.ENDC, | ||
| ) | ||
| elif code == 404: | ||
| log.critical( | ||
| f"{Colors.FAIL}🔍 Profile Not Found: The ID '{sanitize_for_log(profile_id)}' does not exist.{Colors.ENDC}" | ||
| ) | ||
| log.critical( | ||
| f"{Colors.FAIL} Please verify the Profile ID from your Control D Dashboard URL.{Colors.ENDC}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🚩 check_api_access does not use the new _log_auth_error helper
The new _log_auth_error helper at main.py:1786 centralizes auth error logging for 401/403/404 codes. However, check_api_access at main.py:1666 still uses its own inline logging for the same error codes. Additionally, check_api_access at line 1689 does NOT sanitize profile_id in the 403 f-string (f"...Profile {profile_id}...") whereas the extracted _log_auth_error at line 1796-1800 does use sanitize_for_log(profile_id). This is a pre-existing inconsistency that the refactoring didn't address — both functions handle the same error codes but only one sanitizes the profile ID for the 403 case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _parse_folders_response(data: dict) -> dict[str, str] | None: | ||
| # Ensure we got the expected top-level JSON structure. | ||
| if not isinstance(data, dict): |
There was a problem hiding this comment.
📝 Info: Type hint data: dict is narrower than actual input from resp.json()
The _parse_folders_response parameter is typed data: dict but resp.json() can return any JSON type (list, str, int, None, etc.). The function correctly handles this at runtime via the isinstance(data, dict) check on line 1743, and from __future__ import annotations (line 16) makes the annotation a string at runtime so no TypeError occurs. However, the type hint is misleading — Any would be more accurate and would prevent a type checker from flagging callers that pass non-dict values (which is the intended defensive use case).
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: Simplified deeply nested logic in
verify_access_and_get_foldersby extracting JSON parsing to_parse_folders_responseand error logging to_log_auth_error.💡 Why: Reduces cyclomatic complexity and improves maintainability by decomposing a Brain Method into logical sub-steps.
✅ Verification: Verified safe via full regression test suite (
uv run pytest tests/) and linter (ruff).✨ Result: Enhanced readability without altering behavior.
PR created automatically by Jules for task 8559911743973236436 started by @abhimehro