Merged
Conversation
cdrini
requested changes
Jan 2, 2026
Collaborator
cdrini
left a comment
cdrini
left a comment
There was a problem hiding this comment.
Niiiice, great work! The APIs look so much tighter in their own file like this, too!
A few bugs while testing:
Tested
- ❌ search inside
- ✅ .json work as normal
- ✅ Works with limit
⚠️ Works with offset- Internal error, but this parameter wasn't supported before
- ✅ Works with page
- ❌ Works from search results fallback partial
- See comment in code
- ✅
/search/subjects.json- ✅ Works at all
- ✅ offset
- ✅ limit
- ✅
/search/authors.json - ❌
/search/lists.json- ❌ Works at all
startis set tonullinstead of0
- ✅ offset
- ✅ limit
- ❌ page
- Does nothing; previously used to work
- ✅ sort
- ✅ As partial request for list carousel
- ❌ Works at all
| from pydantic import BaseModel, Field, model_validator | ||
|
|
||
|
|
||
| class Pagination(BaseModel): |
Collaborator
There was a problem hiding this comment.
Niiiice! Love seeing this dried up!
cdrini
added
the
Needs: Submitter Input
RayBB
force-pushed
the
fastapi-more-search-endpoints
branch
from
c0139c0 to
f89a200
Compare
github-actions
bot
removed
the
Needs: Submitter Input
…he legacy implementation.
…rch JSON implementation.
…to search endpoints.
… and a mock for its fulltext search dependency.
…ebpy `search/inside` endpoints, including a new mock for the synchronous search function.
…duce Solr query mocks.
…ontract test for it.
RayBB
force-pushed
the
fastapi-more-search-endpoints
branch
from
f89a200 to
829e60f
Compare
Collaborator
Author
|
@cdrini all the issues mentioned above should be resolved now! |
… sort parameter.
…d update the author search endpoint to use a new Pydantic request model.
…Solr and using Solr's start value.
cdrini
approved these changes
Jan 7, 2026
Collaborator
cdrini
left a comment
cdrini
left a comment
There was a problem hiding this comment.
Lgtm! All the tests now pass! 🥳
- ✅ search inside
- ✅ .json work as normal
- ✅ Works with limit
- ✅ Works with offset
- Internal error, but this parameter wasn't supported before
- ✅ Works with page
- ✅ Works from search results fallback partial
- See comment in code
- ✅
/search/subjects.json- ✅ Works at all
- ✅ offset
- ✅ limit
- ✅
/search/authors.json - ✅
/search/lists.json- ✅ Works at all
startis set tonullinstead of0
- ✅ offset
- ✅ limit
- ✅ page
- Does nothing; previously used to work
- ✅ sort
- ✅ As partial request for list carousel
- ✅ Works at all
Comment on lines
+216
to
+218
| page=pagination.page, | ||
| offset=pagination.offset, | ||
| limit=pagination.limit, |
Collaborator
There was a problem hiding this comment.
We might want to update these to take the pagination model directly.
Comment on lines
+341
to
+344
| class AuthorSearchRequestParams(Pagination): | ||
| q: str = Field("", description="The search query") | ||
| fields: str = Field("*", description="Fields to return") | ||
| sort: AuthorSortOption = Field("", description="Sort order") # type: ignore[valid-type] |
Collaborator
There was a problem hiding this comment.
Debating on whether we can inline this class object ; having these be params is a bit easier to read and list indirection. Ray said that having Depends() twice with query params is "broken" in fastapi (causes some sort of conflict between fastapi + pydantic).
mystic-06
pushed a commit
to mystic-06/openlibrary
that referenced
this pull request
Jan 11, 2026
* feat: Add FastAPI subject search endpoint and deprecate old implementation. * feat: migrate author search to a new FastAPI endpoint and deprecate the legacy implementation. * feat: Add FastAPI endpoint for list search and deprecate old list search JSON implementation. * feat: Extract Pagination model to a shared module and integrate it into search endpoints. * refactor: Consolidate individual search endpoints to search.py * feat: Add tests for `/search/inside.json` endpoint parameter handling and a mock for its fulltext search dependency. * feat: Add contract test comparing parameter passing for FastAPI and webpy `search/inside` endpoints, including a new mock for the synchronous search function. * feat: Add API contract test for `/search/subjects` endpoint and introduce Solr query mocks. * feat: add `/search/lists` endpoint to the web.py test app and a new contract test for it. * feat: Add contract test for `/search/authors` endpoint. * refactor: restrict 'api' query parameter to literal values 'next' or empty string * feat: Introduce direct offset parameter for fulltext search and its FastAPI integration. * fix: default search results start to 0 if offset is falsy * fix pagination issue for list search * feat: Add `ListSortOption` type with validation and schema for search sort parameter. * feat: Refactor sort option validation into a Pydantic type factory and update the author search endpoint to use a new Pydantic request model. * refactor: Simplify list search offset calculation by passing page to Solr and using Solr's start value. * feat: Pass page parameter in subject and author search API calls. * fix: Provide a default 'start' value in search response if the key is missing. * Remove commented-out ValueError exception handler.
lokesh
pushed a commit
to lokesh/openlibrary
that referenced
this pull request
Feb 4, 2026
* feat: Add FastAPI subject search endpoint and deprecate old implementation. * feat: migrate author search to a new FastAPI endpoint and deprecate the legacy implementation. * feat: Add FastAPI endpoint for list search and deprecate old list search JSON implementation. * feat: Extract Pagination model to a shared module and integrate it into search endpoints. * refactor: Consolidate individual search endpoints to search.py * feat: Add tests for `/search/inside.json` endpoint parameter handling and a mock for its fulltext search dependency. * feat: Add contract test comparing parameter passing for FastAPI and webpy `search/inside` endpoints, including a new mock for the synchronous search function. * feat: Add API contract test for `/search/subjects` endpoint and introduce Solr query mocks. * feat: add `/search/lists` endpoint to the web.py test app and a new contract test for it. * feat: Add contract test for `/search/authors` endpoint. * refactor: restrict 'api' query parameter to literal values 'next' or empty string * feat: Introduce direct offset parameter for fulltext search and its FastAPI integration. * fix: default search results start to 0 if offset is falsy * fix pagination issue for list search * feat: Add `ListSortOption` type with validation and schema for search sort parameter. * feat: Refactor sort option validation into a Pydantic type factory and update the author search endpoint to use a new Pydantic request model. * refactor: Simplify list search offset calculation by passing page to Solr and using Solr's start value. * feat: Pass page parameter in subject and author search API calls. * fix: Provide a default 'start' value in search response if the key is missing. * Remove commented-out ValueError exception handler.
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.
This PR moves all the search endpoints to fastapi.
Note: most of the test code here will be deleted quickly once this stuff is running on production. It's AI generated but gives me a bit more confidence that there aren't any major discrepancies in how we call things. The small ones are noted in the tests. Mostly differences around when to pass none due to the unified Pagination model.
Technical
Testing
Screenshot
Stakeholders