refactor(http): remove retry logic, add Amazon provider and cache cleanup - #11
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
WalkthroughThis PR refactors the thumbnail provider infrastructure by removing tenacity-based HTTP retry logic, introducing direct request handling with configured timeouts, adding Amazon and Internet Archive providers, and updating caching semantics to consistently return Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
rero_invenio_thumbnails/contrib/utils.py (1)
102-106: Single-source the 50px fallback.This now hard-codes
50even thoughrero_invenio_thumbnails/config.pyalready definesRERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION. If that default changes later, validation inside and outside Flask app context will drift.♻️ Suggested refactor
+from rero_invenio_thumbnails.config import RERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION + ... - if min_dimension is None: - min_dimension = 50 + if min_dimension is None: + min_dimension = RERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rero_invenio_thumbnails/contrib/utils.py` around lines 102 - 106, The code hard-codes the 50px fallback into utils.py via the local variable min_dimension; instead import the package-level default (the RERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION constant) from rero_invenio_thumbnails.config and use that as the fallback when current_app is unavailable so the default is single-sourced; update the logic around min_dimension to set it to the imported config constant instead of the literal 50 when still None after attempting to read current_app.config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/get_thumbnail_url.mmd`:
- Around line 13-15: The diagram shows cache.set being called when providers are
empty, but in get_thumbnail_url() the null/None cache write is only performed
when caching is enabled; update the diagram to gate the cache.set node on the
cached=True condition (e.g., change the H -->|yes| Y[...] branch to indicate
cached=True before showing cache.set and return None, None) and ensure the
cached=False path goes directly to returning None, None without the cache.set
node; reference get_thumbnail_url, the cached parameter, and cache.set when
making the change.
In `@README.md`:
- Around line 75-85: Update the README snippet so it does not present "dnb" as a
copy-paste default: either annotate the RERO_INVENIO_THUMBNAILS_PROVIDERS
example to add a clear licence caveat next to "dnb" (mentioning the required
MVB/DNB data licence) or move "dnb" out of the default list into a separate
"optional providers" example; then mirror the same licence note in INSTALL.md.
Reference the RERO_INVENIO_THUMBNAILS_PROVIDERS example and ensure the change
mentions the DNB/MVB licence requirement explicitly.
In `@rero_invenio_thumbnails/config.py`:
- Around line 41-49: Remove "dnb" from the default
RERO_INVENIO_THUMBNAILS_PROVIDERS list so the DNB provider is no longer enabled
by default; update any config comment or docstring near the
RERO_INVENIO_THUMBNAILS_PROVIDERS constant to state that DNB coverage requires a
paid VLB/MVB licence and must be opted-in at the instance level (e.g., instruct
operators to add "dnb" to their instance-specific settings to enable it).
In `@rero_invenio_thumbnails/contrib/__init__.py`:
- Around line 75-79: The documentation example incorrectly treats the tuple
return from get_thumbnail_url as a single truthy value; update the example usage
of provider.get_thumbnail_url to unpack the returned tuple (e.g., url,
provider_name = provider.get_thumbnail_url(isbn)) and then check url (if url:)
before proceeding, or assign to a temporary and test the first element (result =
provider.get_thumbnail_url(isbn); url = result[0]; if url: ...); ensure
references to get_thumbnail_url and provider.get_thumbnail_url are used so
readers see the correct unpacking and conditional check.
In `@rero_invenio_thumbnails/contrib/google_api/api.py`:
- Around line 70-72: The Google API call using requests.get(url) in the function
that builds url from self.base_url and clean_isbn_value should include a timeout
to avoid blocking; modify the requests.get call (requests.get(url)) to pass
timeout=(1, 5) (connect, read) so the provider follows the same pattern as other
providers and fails fast on slow responses.
In `@rero_invenio_thumbnails/contrib/google_books/api.py`:
- Around line 73-75: The requests.get call that builds the Google Books metadata
URL (the line using response = requests.get(url) with url built from
self.base_url and clean_isbn_value) has no timeout and can hang; modify the call
to pass a timeout (e.g., timeout=5 or a configured timeout constant) so socket
timeouts are raised as requests.exceptions and handled by the existing
decorator, and ensure the timeout value is applied consistently for this Google
Books metadata fetch.
In `@rero_invenio_thumbnails/contrib/internet_archive/api.py`:
- Around line 74-76: The requests.get call for the OCAID search
(requests.get(url, headers=self.headers)) has no timeout and can hang; update
the request to include a bounded timeout (e.g. timeout=5) or, better, use a
configurable attribute on the class (e.g. self.timeout with a sensible default)
and pass timeout=self.timeout to requests.get, and ensure any tests or callers
that construct this API class can override the timeout if needed.
---
Nitpick comments:
In `@rero_invenio_thumbnails/contrib/utils.py`:
- Around line 102-106: The code hard-codes the 50px fallback into utils.py via
the local variable min_dimension; instead import the package-level default (the
RERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION constant) from
rero_invenio_thumbnails.config and use that as the fallback when current_app is
unavailable so the default is single-sourced; update the logic around
min_dimension to set it to the imported config constant instead of the literal
50 when still None after attempting to read current_app.config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a097a0-bfc2-486c-abeb-01033e638c17
📒 Files selected for processing (18)
INSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/__init__.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pytests/conftest.pytests/test_dnb_provider.pytests/test_utils.py
💤 Files with no reviewable changes (2)
- pyproject.toml
- tests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rero_invenio_thumbnails/contrib/google_books/api.py (1)
73-74:⚠️ Potential issue | 🔴 CriticalAdd the fail-fast timeout to the metadata request.
Line 74 still uses a bare
requests.get, so a slow Google Books response can hang this provider indefinitely and bypass the PR's timeout policy. This should use the same(connect, read)timeout style as the rest of the refactor.Minimal fix
- response = requests.get(url) + response = requests.get(url, timeout=(1, 5))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rero_invenio_thumbnails/contrib/google_books/api.py` around lines 73 - 74, The requests.get call for building the Google Books metadata URL uses a bare request (requests.get(url)) and can hang; update the call that creates response to include the same fail-fast timeout tuple used elsewhere in this refactor (a (connect, read) timeout), e.g. change the requests.get(url) invocation that follows the url = f"...{clean_isbn_value}" line to include timeout=(connect_timeout, read_timeout) or the module/class-level timeout variable used by other methods so it mirrors the rest of the provider's timeout behavior.
🧹 Nitpick comments (1)
tests/test_rero_invenio_thumbnails.py (1)
156-179: These tests no longer distinguish cache hits from misses.Now that both a cached miss and a fresh provider miss return
(None, None), lines 163/166 and 176/179 will still pass if the second call ignores the cache, or ifcached=Falseaccidentally reads it. Please assert provider call counts or inspect the cache entry so these tests still catch cache-hit/cache-bypass regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rero_invenio_thumbnails.py` around lines 156 - 179, The tests call get_thumbnail_url(id, cached=...) but only assert return values, so they no longer detect whether the provider was actually invoked or the cache was bypassed; update test_get_thumbnail_url_with_cached_none_result and test_get_thumbnail_url_with_cached_none_and_uncached_call to also verify cache/provider behavior by either (a) mocking the underlying provider fetch function for the "files" provider and asserting its call count between the first and second calls to get_thumbnail_url for the same identifier, or (b) directly inspecting the thumbnail cache entry after the first call (e.g., reading the cache store or extension used by get_thumbnail_url) to ensure a cached miss entry exists and that a subsequent call with cached=False triggers a provider invocation while cached=True does not; reference get_thumbnail_url and the cached parameter when adding the mocks/inspections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rero_invenio_thumbnails/contrib/utils.py`:
- Around line 181-192: The current use of client.keys(pattern) blocks Redis;
change the deletion to use incremental scanning with client.scan_iter(pattern,
count=1000) and perform batched deletes to avoid blocking. In the block that
obtains client (the variables prefix, cache, client, key_prefix, pattern),
iterate keys = client.scan_iter(pattern, count=1000), accumulate a small batch
(e.g., 100–1000 keys), call client.delete(*batch) for each batch (or use a
pipeline if available) and increment a deleted_count, then return deleted_count
instead of len(keys); ensure you handle empty results and that keys' types
(bytes/str) are passed as-is to delete.
In `@tests/test_amazon_provider.py`:
- Around line 188-208: Add a default pytest option to exclude tests marked with
the external marker so network-dependent tests like
test_amazon_real_thumbnail_is_valid_image and
test_amazon_real_thumbnail_pre2010_french do not run in normal CI; update
pytest.ini_options.addopts in pyproject.toml to include an addopts entry that
excludes the external marker (e.g., use a -m "not external" style setting),
ensuring the marker name "external" is registered and the behavior only runs
those tests when explicitly enabled.
---
Duplicate comments:
In `@rero_invenio_thumbnails/contrib/google_books/api.py`:
- Around line 73-74: The requests.get call for building the Google Books
metadata URL uses a bare request (requests.get(url)) and can hang; update the
call that creates response to include the same fail-fast timeout tuple used
elsewhere in this refactor (a (connect, read) timeout), e.g. change the
requests.get(url) invocation that follows the url = f"...{clean_isbn_value}"
line to include timeout=(connect_timeout, read_timeout) or the
module/class-level timeout variable used by other methods so it mirrors the rest
of the provider's timeout behavior.
---
Nitpick comments:
In `@tests/test_rero_invenio_thumbnails.py`:
- Around line 156-179: The tests call get_thumbnail_url(id, cached=...) but only
assert return values, so they no longer detect whether the provider was actually
invoked or the cache was bypassed; update
test_get_thumbnail_url_with_cached_none_result and
test_get_thumbnail_url_with_cached_none_and_uncached_call to also verify
cache/provider behavior by either (a) mocking the underlying provider fetch
function for the "files" provider and asserting its call count between the first
and second calls to get_thumbnail_url for the same identifier, or (b) directly
inspecting the thumbnail cache entry after the first call (e.g., reading the
cache store or extension used by get_thumbnail_url) to ensure a cached miss
entry exists and that a subsequent call with cached=False triggers a provider
invocation while cached=True does not; reference get_thumbnail_url and the
cached parameter when adding the mocks/inspections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ee03ec8-0615-484e-83b3-34ea2c76ef32
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
INSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/amazon/__init__.pyrero_invenio_thumbnails/contrib/amazon/api.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pytests/conftest.pytests/test_amazon_provider.pytests/test_rero_invenio_thumbnails.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/conftest.py
✅ Files skipped from review due to trivial changes (6)
- rero_invenio_thumbnails/contrib/amazon/init.py
- INSTALL.md
- rero_invenio_thumbnails/contrib/google_api/api.py
- README.md
- rero_invenio_thumbnails/contrib/internet_archive/api.py
- rero_invenio_thumbnails/contrib/init.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_utils.py
- rero_invenio_thumbnails/api.py
- pyproject.toml
- rero_invenio_thumbnails/init.py
- rero_invenio_thumbnails/config.py
- rero_invenio_thumbnails/contrib/bnf/api.py
rerowep
force-pushed
the
wep-no-retries
branch
4 times, most recently
from
April 14, 2026 10:52
284f47d to
4c34bc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rero_invenio_thumbnails/contrib/utils.py`:
- Around line 194-199: The current batched-delete logic incorrectly adds
len(batch) to the deleted counter; change both delete sites to use the actual
return value from client.delete(*batch) instead: call client.delete(*batch),
capture its return (e.g., deleted_count) and increment deleted by that value
(for both the in-loop delete and the final if batch block), referencing the
variables client.delete, deleted, and batch to locate the spots to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 083e5175-ab61-4258-99e4-5c1fe2aa04a3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
INSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/amazon/__init__.pyrero_invenio_thumbnails/contrib/amazon/api.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pyscripts/tests.shtests/conftest.pytests/test_amazon_provider.pytests/test_internet_archive_provider.pytests/test_rero_invenio_thumbnails.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/conftest.py
✅ Files skipped from review due to trivial changes (4)
- tests/test_internet_archive_provider.py
- rero_invenio_thumbnails/contrib/amazon/init.py
- scripts/tests.sh
- README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/get_thumbnail_url.mmd
- rero_invenio_thumbnails/contrib/bnf/api.py
- rero_invenio_thumbnails/api.py
- tests/test_rero_invenio_thumbnails.py
- rero_invenio_thumbnails/contrib/google_books/api.py
- rero_invenio_thumbnails/contrib/google_api/api.py
- INSTALL.md
- pyproject.toml
- rero_invenio_thumbnails/init.py
- rero_invenio_thumbnails/contrib/internet_archive/api.py
- tests/test_utils.py
- tests/test_amazon_provider.py
- rero_invenio_thumbnails/contrib/init.py
rerowep
force-pushed
the
wep-no-retries
branch
2 times, most recently
from
April 14, 2026 14:14
081d418 to
4e6fd54
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rero_invenio_thumbnails/contrib/utils.py (1)
155-157: Log request failures before returningFalse.At Line 156, network errors are swallowed, which removes provider-level visibility now that retries are gone. A low-level debug log here will make production troubleshooting much easier.
🛠️ Suggested patch
try: response = requests.get(url, headers=headers, timeout=timeout) - except requests.RequestException: + except requests.RequestException as err: + with suppress(AttributeError, RuntimeError): + current_app.logger.debug( + f"Request error retrieving image from {provider_name} for ISBN {isbn}: {err!s}" + ) return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rero_invenio_thumbnails/contrib/utils.py` around lines 155 - 157, The except block that catches requests.RequestException after the requests.get call should log the failure before returning False: capture the exception (requests.RequestException) and emit a debug (or appropriate level) log that includes the URL, timeout, and exception details to aid troubleshooting; use the module logger (create logging.getLogger(__name__) if not present) and include context such as the url and exception info in the log call, then return False as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rero_invenio_thumbnails/contrib/utils.py`:
- Around line 155-157: The except block that catches requests.RequestException
after the requests.get call should log the failure before returning False:
capture the exception (requests.RequestException) and emit a debug (or
appropriate level) log that includes the URL, timeout, and exception details to
aid troubleshooting; use the module logger (create logging.getLogger(__name__)
if not present) and include context such as the url and exception info in the
log call, then return False as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20e7a5fb-7b39-4946-8051-1c4c9a2aa06a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
INSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/amazon/__init__.pyrero_invenio_thumbnails/contrib/amazon/api.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pyscripts/tests.shtests/conftest.pytests/test_amazon_provider.pytests/test_internet_archive_provider.pytests/test_rero_invenio_thumbnails.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/conftest.py
✅ Files skipped from review due to trivial changes (3)
- rero_invenio_thumbnails/contrib/amazon/init.py
- scripts/tests.sh
- tests/test_amazon_provider.py
🚧 Files skipped from review as they are similar to previous changes (11)
- rero_invenio_thumbnails/api.py
- tests/test_internet_archive_provider.py
- pyproject.toml
- rero_invenio_thumbnails/contrib/google_api/api.py
- docs/get_thumbnail_url.mmd
- rero_invenio_thumbnails/contrib/bnf/api.py
- rero_invenio_thumbnails/contrib/google_books/api.py
- INSTALL.md
- README.md
- tests/test_rero_invenio_thumbnails.py
- rero_invenio_thumbnails/contrib/internet_archive/api.py
PascalRepond
changed the title
PascalRepond
requested changes
Apr 14, 2026
rerowep
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rero_invenio_thumbnails/contrib/utils.py`:
- Around line 102-103: In validate_image_content, the unguarded access to
current_app.config when setting min_dimension can raise RuntimeError outside a
Flask app context; modify the block that reads
current_app.config.get("RERO_INVENIO_THUMBNAILS_MIN_IMAGE_DIMENSION", 50) to be
wrapped in a try/except that catches RuntimeError and falls back to 50 (same
pattern used later in this function), ensuring callers like
fetch_and_validate_thumbnail and external providers can call
validate_image_content without an app context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68b345c3-932c-4ce1-b63f-b2745a8c51c7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
INSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/amazon/__init__.pyrero_invenio_thumbnails/contrib/amazon/api.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pyscripts/tests.shtests/conftest.pytests/test_amazon_provider.pytests/test_internet_archive_provider.pytests/test_rero_invenio_thumbnails.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/conftest.py
✅ Files skipped from review due to trivial changes (2)
- rero_invenio_thumbnails/contrib/amazon/init.py
- tests/test_amazon_provider.py
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/get_thumbnail_url.mmd
- rero_invenio_thumbnails/contrib/bnf/api.py
- tests/test_internet_archive_provider.py
- rero_invenio_thumbnails/api.py
- rero_invenio_thumbnails/contrib/google_api/api.py
- pyproject.toml
- tests/test_rero_invenio_thumbnails.py
- INSTALL.md
- rero_invenio_thumbnails/contrib/google_books/api.py
- rero_invenio_thumbnails/contrib/amazon/api.py
- README.md
- rero_invenio_thumbnails/contrib/dnb/api.py
- rero_invenio_thumbnails/contrib/init.py
- rero_invenio_thumbnails/config.py
rerowep
force-pushed
the
wep-no-retries
branch
3 times, most recently
from
April 14, 2026 20:02
ff14a78 to
833b996
Compare
PascalRepond
approved these changes
Apr 15, 2026
rerowep
force-pushed
the
wep-no-retries
branch
5 times, most recently
from
April 16, 2026 08:56
992dfe0 to
ce023c3
Compare
PascalRepond
approved these changes
Apr 16, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_internet_archive_provider.py (1)
36-170: Consider parametrizing repetitive provider assertions/setup.Many tests repeat the same
(url is None, provider_name == "internet archive")pattern and mock bootstrap. A small helper orpytest.mark.parametrizetable would reduce duplication and keep future provider-behavior changes easier to update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_internet_archive_provider.py` around lines 36 - 170, Multiple tests repeat the same setup and assertions (checking url is None and provider_name == "internet archive") and duplicated request mock configuration; refactor by extracting a small helper (e.g., assert_no_cover_for_isbn or fetch_and_assert_none) or by using pytest.mark.parametrize to drive the cases (reference the existing tests like test_internet_archive_provider_search_http_error, test_internet_archive_provider_image_not_found, test_internet_archive_provider_small_image, test_internet_archive_provider_cover_fetch_network_error, test_internet_archive_provider_notfound_redirect) so they share a common setup for requests_mock and a single assertion block that verifies (url is None, provider_name == "internet archive"), and move repeated mock responses (IA_SEARCH_RE/IA_IMG_RE setups) into that helper or parametrized fixtures to reduce duplication.tests/test_files_provider.py (1)
80-101: Harden cleanup in the relative-path test.If an assertion fails before the cleanup lines run, test artifacts can remain under
app.root_path. Wrapping cleanup intry/finally(or using a temporary path fixture scoped underapp.root_path) would make this test more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_files_provider.py` around lines 80 - 101, The test creates files under app.root_path (relative_dir/full_dir/test_file) and sets app.config["RERO_INVENIO_THUMBNAILS_FILES_DIR"] then calls FilesProvider and provider.get_thumbnail_path; wrap the setup and assertions in a try/finally so the finally block always removes test_file and full_dir (and restores the original app.config value if you override it) to guarantee cleanup even if an assertion fails. Ensure the finally runs after provider.get_thumbnail_path and cleans up relative_dir artifacts and resets app.config["RERO_INVENIO_THUMBNAILS_FILES_DIR"] to its prior value.rero_invenio_thumbnails/contrib/files/api.py (1)
113-114: Avoid duplicate ISBN normalization between URL and path resolution.
get_thumbnail_urlnormalizes ISBN, thenget_thumbnail_pathnormalizes it again. Consider a small internal helper that accepts an already-cleaned ISBN so normalization happens once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rero_invenio_thumbnails/contrib/files/api.py` around lines 113 - 114, get_thumbnail_url currently calls clean_isbn(isbn) and then calls get_thumbnail_path which re-runs normalization; to avoid duplicate normalization, introduce a small internal helper (e.g., _get_thumbnail_path_from_clean_isbn or add a parameter clean=False to get_thumbnail_path) that accepts an already-cleaned ISBN and skips calling clean_isbn again. Update get_thumbnail_url to call the new helper (or pass clean=False) with clean_isbn(isbn) and remove the extra normalization inside get_thumbnail_path/get_thumbnail_path_from_clean_isbn so ISBN normalization occurs only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tests.sh`:
- Around line 65-67: The unconditional suppression "add_exceptions
CVE-2025-71176" must be made time- or version-gated and documented: update the
scripts/tests.sh entry by tying the suppression to a pytest version check (or an
issue/PR ID) and include an explicit removal deadline and target pytest version
"9.0.3" (e.g., only add the exception if pytest < 9.0.3 or include a comment
with the issue/PR and expiry date), so locate the add_exceptions CVE-2025-71176
line and replace it with a conditional or annotated suppression referencing
pytest 9.0.3 and the removal date/issue.
In `@tests/test_open_library_provider.py`:
- Line 141: The test loop using zip(urls, isbns) should be made strict to avoid
silent truncation; update the iteration in tests/test_open_library_provider.py
where the loop reads "for url, isbn in zip(urls, isbns)" to pass strict=True
(i.e., call zip(urls, isbns, strict=True)) so the test fails if the sequences
differ in length.
---
Nitpick comments:
In `@rero_invenio_thumbnails/contrib/files/api.py`:
- Around line 113-114: get_thumbnail_url currently calls clean_isbn(isbn) and
then calls get_thumbnail_path which re-runs normalization; to avoid duplicate
normalization, introduce a small internal helper (e.g.,
_get_thumbnail_path_from_clean_isbn or add a parameter clean=False to
get_thumbnail_path) that accepts an already-cleaned ISBN and skips calling
clean_isbn again. Update get_thumbnail_url to call the new helper (or pass
clean=False) with clean_isbn(isbn) and remove the extra normalization inside
get_thumbnail_path/get_thumbnail_path_from_clean_isbn so ISBN normalization
occurs only once.
In `@tests/test_files_provider.py`:
- Around line 80-101: The test creates files under app.root_path
(relative_dir/full_dir/test_file) and sets
app.config["RERO_INVENIO_THUMBNAILS_FILES_DIR"] then calls FilesProvider and
provider.get_thumbnail_path; wrap the setup and assertions in a try/finally so
the finally block always removes test_file and full_dir (and restores the
original app.config value if you override it) to guarantee cleanup even if an
assertion fails. Ensure the finally runs after provider.get_thumbnail_path and
cleans up relative_dir artifacts and resets
app.config["RERO_INVENIO_THUMBNAILS_FILES_DIR"] to its prior value.
In `@tests/test_internet_archive_provider.py`:
- Around line 36-170: Multiple tests repeat the same setup and assertions
(checking url is None and provider_name == "internet archive") and duplicated
request mock configuration; refactor by extracting a small helper (e.g.,
assert_no_cover_for_isbn or fetch_and_assert_none) or by using
pytest.mark.parametrize to drive the cases (reference the existing tests like
test_internet_archive_provider_search_http_error,
test_internet_archive_provider_image_not_found,
test_internet_archive_provider_small_image,
test_internet_archive_provider_cover_fetch_network_error,
test_internet_archive_provider_notfound_redirect) so they share a common setup
for requests_mock and a single assertion block that verifies (url is None,
provider_name == "internet archive"), and move repeated mock responses
(IA_SEARCH_RE/IA_IMG_RE setups) into that helper or parametrized fixtures to
reduce duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd59078c-d6a6-4c37-b159-bfa9c59658b3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/workflows/continuous-integration-test.ymlINSTALL.mdREADME.mddocs/get_thumbnail_url.mmdpyproject.tomlrero_invenio_thumbnails/__init__.pyrero_invenio_thumbnails/api.pyrero_invenio_thumbnails/config.pyrero_invenio_thumbnails/contrib/__init__.pyrero_invenio_thumbnails/contrib/amazon/__init__.pyrero_invenio_thumbnails/contrib/amazon/api.pyrero_invenio_thumbnails/contrib/api.pyrero_invenio_thumbnails/contrib/bnf/api.pyrero_invenio_thumbnails/contrib/dnb/api.pyrero_invenio_thumbnails/contrib/files/api.pyrero_invenio_thumbnails/contrib/google_api/api.pyrero_invenio_thumbnails/contrib/google_books/api.pyrero_invenio_thumbnails/contrib/internet_archive/api.pyrero_invenio_thumbnails/contrib/utils.pyrero_invenio_thumbnails/ext.pyrero_invenio_thumbnails/views.pyscripts/tests.shtests/conftest.pytests/test_amazon_provider.pytests/test_bnf_provider.pytests/test_dnb_provider.pytests/test_files_provider.pytests/test_google_api_provider.pytests/test_google_books_provider.pytests/test_internet_archive_provider.pytests/test_open_library_provider.pytests/test_rero_invenio_thumbnails.pytests/test_utils.pytests/test_views.py
✅ Files skipped from review due to trivial changes (9)
- rero_invenio_thumbnails/ext.py
- tests/test_views.py
- rero_invenio_thumbnails/contrib/amazon/init.py
- rero_invenio_thumbnails/views.py
- INSTALL.md
- rero_invenio_thumbnails/contrib/amazon/api.py
- tests/test_utils.py
- rero_invenio_thumbnails/contrib/api.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/get_thumbnail_url.mmd
- rero_invenio_thumbnails/api.py
- rero_invenio_thumbnails/contrib/bnf/api.py
- rero_invenio_thumbnails/contrib/dnb/api.py
- rero_invenio_thumbnails/init.py
- rero_invenio_thumbnails/contrib/google_books/api.py
- rero_invenio_thumbnails/contrib/init.py
- tests/test_rero_invenio_thumbnails.py
- rero_invenio_thumbnails/contrib/internet_archive/api.py
- rero_invenio_thumbnails/contrib/google_api/api.py
- pyproject.toml
- tests/test_dnb_provider.py
- tests/conftest.py
- rero_invenio_thumbnails/config.py
- rero_invenio_thumbnails/contrib/utils.py
…anup * Remove fetch_with_retries, _get_retry_config and _DISABLE_RETRIES from utils.py; all providers now call requests.get directly * Replace timeout integers with (connect, read) tuples in BNF (1, 10) and DNB (1, 10); default in fetch_and_validate_thumbnail changed to (1, 5) for fast fail on offline providers * Add AmazonProvider: fetches covers via ISBN-10 ASIN from Amazon CDN * Internet Archive: reject notfound.png redirects, use validate_image_content directly instead of fetch_and_validate_thumbnail * Add clean_all_cache() in contrib/utils.py to delete all thumbnail cache entries by pattern; exported from rero_invenio_thumbnails * Return (None, None) instead of (None, last_provider) on exhausted providers Co-Authored-by: Peter Weber <peter.weber@rero.ch>
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
2 participants
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.
Uh oh!
There was an error while loading. Please reload this page.