Repository navigation
fix(client): log expected 401/404 fetch failures as concise warnings - #28
Merged
Merged
Conversation
added 2 commits
October 5, 2026 21:57
fetch_resource logged every non-200 response as ERROR together with the whole response body. In production (TUL DSpace) this produced alarming ERROR lines for two situations that are handled and harmless: - 401 "Authentication is required": the session token expired; the calling application re-authenticates and retries successfully. - 404 on /core/items/<uuid>/bundles: the item was deleted from DSpace; get_bundles already turns that into an empty result. Both are now a one-line WARNING naming the status and the request path (not the body). Every other status still logs ERROR, now with the status code and path, and the body capped at 500 characters so one failure cannot flood the log. Recording the failing response in _last_err is unchanged.
Review follow-up. The 401 warning claimed that the caller re-authenticates, which the library cannot know, and ignored clients that never logged in. It now reads "authentication required (session expired or not logged in)". The logged path keeps the query string for 401, 404 and other errors. On search endpoints such as /authz/resourcepolicies/search/resource the uuid in the query is the only identifier of the failing resource.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused changes match the stated requirements, preserve error handling, and include regression coverage with no identified blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Reduces noisy DSpace fetch-error logging without changing return values or _last_err behavior.
Changes:
- Logs 401/404 responses as concise warnings containing the request path and query, without the response body.
- Keeps other failures at ERROR level, limiting logged bodies to 500 characters.
- Adds tests for warning content, error truncation, and preserved failure handling.
| File | Description |
|---|---|
| tests/test_client_read.py | Adds logging regression tests for 401, 404, and 500 responses. |
| dspace_rest_client/client.py | Selects logging behavior by status and caps error-body output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fetch_resourcelogged every non-200 response asERRORand included the whole JSON response body. During a run against the TUL DSpace (2026-10-05) this produced alarming ERROR lines for two cases that are handled and harmless:Authentication is required: the session token had expired. The application re-authenticated and the retry succeeded./core/items/<uuid>/bundles: the item was deleted from DSpace.get_bundlesalready turns this into an empty result.Solution
WARNING DSpace returned 401 for [<path>] - authentication required (session expired or not logged in)WARNING DSpace returned 404 for [<path>] - resource not found (deleted?)ERROR, now with the status code and the path. The body is capped at 500 characters.<path>is the request path plus its query string, without the host or body. The query is kept because on search endpoints such as/authz/resourcepolicies/search/resource?uuid=…it is the only identifier of the failing resource. Recording the failing response in_last_erris unchanged, so callers that branch onlast_err.status_codebehave as before.get_bundlesstill logs its own INFO line for a 404; this PR does not change it.Tests
In
tests/test_client_read.py::TestFetchResource:test_401_logs_warning_with_path_not_bodytest_404_logs_warning_with_path_and_query_not_bodytest_other_error_logs_error_with_truncated_body