Conversation
|
😎 Merged manually by @abhimehro - details. |
…older listing into a single request This implements the only valuable change from Jules PR #170 that wasn't already in main. Reduces startup API calls from 2 to 1, lowering latency. Updates sync_profile(), tests, and documentation accordingly. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
…allthrough logging Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
| mock_response.raise_for_status.return_value = None | ||
|
|
||
| result = m.verify_access_and_get_folders(mock_client, "valid_profile") | ||
| assert result == {"Folder A": "id_a", "Folder B": "id_b"} |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "invalid_token") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "invalid_token") is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "forbidden_profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "forbidden_profile") is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "missing_profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "missing_profile") is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "MAX_RETRIES", 2) | ||
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None | ||
| assert mock_client.get.call_count == 2 |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "MAX_RETRIES", 2) | ||
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None | ||
| assert mock_client.get.call_count == 2 |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert mock_client.get.call_count == 2 | ||
| assert mock_log.error.called | ||
| assert "Network failure" in str(mock_log.error.call_args) | ||
| assert "Network error" in str(mock_log.error.call_args) or "access verification" in str(mock_log.error.call_args) |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| mock_response.raise_for_status.return_value = None | ||
|
|
||
| result = m.verify_access_and_get_folders(mock_client, "valid_profile") | ||
| assert result == {"Folder A": "id_a", "Folder B": "id_b"} |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "invalid_token") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "invalid_token") is None |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "forbidden_profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "forbidden_profile") is None |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "log", mock_log) | ||
|
|
||
| assert m.check_api_access(mock_client, "missing_profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "missing_profile") is None |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "MAX_RETRIES", 2) | ||
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None | ||
| assert mock_client.get.call_count == 2 |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| monkeypatch.setattr(m, "MAX_RETRIES", 2) | ||
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert m.check_api_access(mock_client, "profile") is False | ||
| assert m.verify_access_and_get_folders(mock_client, "profile") is None | ||
| assert mock_client.get.call_count == 2 |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert mock_client.get.call_count == 2 | ||
| assert mock_log.error.called | ||
| assert "Network failure" in str(mock_log.error.call_args) | ||
| assert "Network error" in str(mock_log.error.call_args) or "access verification" in str(mock_log.error.call_args) |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| ) | ||
| elif code == 403: | ||
| log.critical( | ||
| f"{Colors.FAIL}🚫 Access Denied: Token lacks permission for Profile {sanitize_for_log(profile_id)}.{Colors.ENDC}" |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning
| f"Request failed (attempt {attempt + 1}/{MAX_RETRIES}). Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) | ||
|
|
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "e" doesn't conform to snake_case naming style Warning
| return None | ||
|
|
||
|
|
||
| def get_all_existing_rules( |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (103/100) Warning
| # Initial client for getting existing state AND processing folders | ||
| # Optimization: Reuse the same client session to keep TCP connections alive | ||
| with _api_client() as client: | ||
| # Check for API access problems first (401/403/404) |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (136/100) Warning
|
|
||
|
|
||
| def test_check_api_access_404(monkeypatch): | ||
| def test_verify_access_and_get_folders_404(monkeypatch): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning test
|
|
||
|
|
||
| def test_check_api_access_generic_http_error(monkeypatch): | ||
| def test_verify_access_and_get_folders_500_retry(monkeypatch): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning test
|
|
||
|
|
||
| def test_check_api_access_network_error(monkeypatch): | ||
| def test_verify_access_and_get_folders_network_error(monkeypatch): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning test
| assert mock_client.get.call_count == 2 | ||
| assert mock_log.error.called | ||
| assert "Network failure" in str(mock_log.error.call_args) | ||
| assert "Network error" in str(mock_log.error.call_args) or "access verification" in str(mock_log.error.call_args) |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (117/100) Warning test
| monkeypatch.setattr( | ||
| main, "list_existing_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"} | ||
| ) | ||
| monkeypatch.setattr(main, "verify_access_and_get_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"}) |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (114/100) Warning test
| monkeypatch.setattr( | ||
| main, "list_existing_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"} | ||
| ) | ||
| monkeypatch.setattr(main, "verify_access_and_get_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"}) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (114/100) Warning test
| assert mock_client.get.call_count == 2 | ||
| assert mock_log.error.called | ||
| assert "Network failure" in str(mock_log.error.call_args) | ||
| assert "Network error" in str(mock_log.error.call_args) or "access verification" in str(mock_log.error.call_args) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (117/100) Warning test
|
|
||
|
|
||
| def test_check_api_access_network_error(monkeypatch): | ||
| def test_verify_access_and_get_folders_network_error(monkeypatch): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
|
|
||
|
|
||
| def test_check_api_access_generic_http_error(monkeypatch): | ||
| def test_verify_access_and_get_folders_500_retry(monkeypatch): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
|
|
||
|
|
||
| def test_check_api_access_404(monkeypatch): | ||
| def test_verify_access_and_get_folders_404(monkeypatch): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
main.py
Outdated
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
main.py
Outdated
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
main.py
Outdated
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| return False | ||
|
|
||
| existing_folders = list_existing_folders(client, profile_id) | ||
| if not no_delete: |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| # Optimization: Reuse the same client session to keep TCP connections alive | ||
| with _api_client() as client: | ||
| # Check for API access problems first (401/403/404) | ||
| if not check_api_access(client, profile_id): |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
There was a problem hiding this comment.
Pull request overview
This PR introduces a combined Control D API helper to reduce startup latency by replacing the sequential “access check + folder listing” pattern with a single request, and updates call sites, tests, and docs accordingly.
Changes:
- Add
verify_access_and_get_folders()to combine access verification and existing-folder retrieval into one API call with retry/backoff. - Update
sync_profile()to use the combined helper (and pass known folders intoget_all_existing_rules()to avoid redundant calls). - Update tests/mocks and refresh
WARP.mddocumentation to reflect the new helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
main.py |
Adds verify_access_and_get_folders() and switches sync_profile() to use it for a single-call access+folder fetch. |
test_main.py |
Renames/updates API access tests to cover the new combined helper and retry behavior. |
tests/test_parallel_deletion.py |
Updates mocks to stub verify_access_and_get_folders() instead of the prior two-call pattern. |
WARP.md |
Documents the new helper and the updated workflow in the sync phase. |
| except httpx.HTTPStatusError as e: | ||
| code = e.response.status_code | ||
| if code in (401, 403, 404): | ||
| if code == 401: | ||
| log.critical( |
There was a problem hiding this comment.
verify_access_and_get_folders() currently retries for any HTTP status other than 401/403/404 (e.g., 400/422), because all non-(401/403/404) errors fall through to the retry sleep. This diverges from the intended behavior described in the PR (retry only on 5xx) and can add unnecessary delays for non-retriable client errors. Consider explicitly retrying only for 5xx (and optionally 429) and failing fast for other 4xx codes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (119/100) Warning
| existing_folders = verify_access_and_get_folders(client, profile_id) | ||
| if existing_folders is None: | ||
| return False | ||
|
|
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "e" doesn't conform to snake_case naming style Warning
| # Initial client for getting existing state AND processing folders | ||
| # Optimization: Reuse the same client session to keep TCP connections alive | ||
| with _api_client() as client: | ||
| # Check for API access problems first (401/403/404) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "e" doesn't conform to snake_case naming style Warning
|
|
||
|
|
||
| def get_all_existing_rules( | ||
| client: httpx.Client, |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "pk" doesn't conform to snake_case naming style Warning
| f"Request failed (attempt {attempt + 1}/{MAX_RETRIES}). Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) | ||
|
|
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "f" doesn't conform to snake_case naming style Warning
| pk = f.get("PK") | ||
| if not name or not pk: | ||
| continue | ||
| result[str(name).strip()] = str(pk) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Too many branches (17/12) Warning
| pk = f.get("PK") | ||
| if not name or not pk: | ||
| continue | ||
| result[str(name).strip()] = str(pk) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Too many return statements (8/6) Warning
| pk = f.get("PK") | ||
| if not name or not pk: | ||
| continue | ||
| result[str(name).strip()] = str(pk) |
Check warning
Code scanning / Pylint (reported by Codacy)
Too many return statements (8/6) Warning
| pk = f.get("PK") | ||
| if not name or not pk: | ||
| continue | ||
| result[str(name).strip()] = str(pk) |
Check warning
Code scanning / Pylint (reported by Codacy)
Too many branches (17/12) Warning
| f"Request failed (attempt {attempt + 1}/{MAX_RETRIES}). Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) | ||
|
|
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "f" doesn't conform to snake_case naming style Warning
|
|
||
|
|
||
| def get_all_existing_rules( | ||
| client: httpx.Client, |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "pk" doesn't conform to snake_case naming style Warning
| # Initial client for getting existing state AND processing folders | ||
| # Optimization: Reuse the same client session to keep TCP connections alive | ||
| with _api_client() as client: | ||
| # Check for API access problems first (401/403/404) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "e" doesn't conform to snake_case naming style Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (119/100) Warning
Reviewed all 6 open Jules PRs (#146, #149, #163, #164, #166, #170). All features except one are already in main. The sole missing optimization—combining the access check and folder listing into a single API call—is implemented here.
Changes
verify_access_and_get_folders()— New function inmain.pythat replaces the sequentialcheck_api_access()+list_existing_folders()pattern with a single request. Includes retry with exponential backoff for 5xx errors and immediate failure for 401/403/404. ReturnsOptional[Dict[str, str]](None= access denied).sync_profile()call site — Switched from two calls to one:Tests — Renamed and updated 6 test cases for the new function (success, 401, 403, 404, 500 retry, network error). Updated
test_parallel_deletion.pymocks to use the combined function.Docs — Updated
WARP.mdto reflect the new API helper and workflow.PRs to close after merge
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.