-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Don't hit affiliate server on /isbn #11304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't hit affiliate server on /isbn #11304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR disables automatic imports from the affiliate server when querying books by ISBN through the /isbn
endpoint to resolve traffic issues. The change maintains the ISBN lookup functionality while preventing resource-intensive server hits.
- Adds an
allow_import
parameter to theEdition.from_isbn()
method - Sets
allow_import=False
when calling from the/isbn
endpoint to skip affiliate server calls - Preserves existing functionality for other callers of the method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
openlibrary/plugins/openlibrary/code.py | Updated ISBN endpoint to disable imports by passing allow_import=False |
openlibrary/core/models.py | Added allow_import parameter to control affiliate server calls in Edition.from_isbn() |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cls, | ||
isbn_or_asin: str, | ||
high_priority: bool = False, | ||
allow_import: bool = False, |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value False
for allow_import
changes the existing behavior for all callers who don't explicitly pass this parameter. Consider defaulting to True
to maintain backward compatibility, or ensure all existing callers are updated to explicitly pass allow_import=True
where needed.
allow_import: bool = False, | |
allow_import: bool = True, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to default to false as a result of the performance risks associated. Enabling it on a per-use case basis is I think a safer approach.
Reviewed on call with @jimchamp |
Closes #11280 . This remove the auto-import of the
/isbn
. Note the greatest fix long term. We should create a separate issue to investigate how to re-enable. But this did appear to resolve our traffic issues, while still keeping the/isbn
point up for finding books.Technical
Testing
Screenshot
Stakeholders