Skip to content

Fix parsing of fastapi search.json boolean parsing#11548

Closed
cdrini wants to merge 2 commits intointernetarchive:masterfrom
cdrini:fix/search-json-facet-parsing
Closed

Fix parsing of fastapi search.json boolean parsing#11548
cdrini wants to merge 2 commits intointernetarchive:masterfrom
cdrini:fix/search-json-facet-parsing

Conversation

@cdrini
Copy link
Collaborator

@cdrini cdrini commented Dec 4, 2025

Noticed a bug with the new release and the boolean facet params

Technical

Testing

✅ This no longer errors: https://testing.openlibrary.org/_fast/search.json?q=harry+potter&has_fulltext=true

Screenshot

Stakeholders

@cdrini cdrini marked this pull request as ready for review December 4, 2025 23:20
Copilot AI review requested due to automatic review settings December 4, 2025 23:20
@cdrini cdrini added the Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle. label Dec 4, 2025
@github-project-automation github-project-automation bot moved this to Waiting Review/Merge from Staff in Ray's Project Dec 4, 2025
Copy link
Contributor

Copilot AI left a 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 fixes the parsing of boolean query parameters in the FastAPI /search.json endpoint by changing the type annotations for has_fulltext and public_scan_b from bool | None to Literal['true', 'false'] | None. This change ensures that these parameters are handled as string literals matching the expected format for the downstream Solr query processing code, which uses facet_rewrites that explicitly check for string values 'true' and 'false'.

Key changes:

  • Added Literal import from typing
  • Changed has_fulltext and public_scan_b type annotations to accept only the string literals 'true' or 'false' (or None)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@RayBB
Copy link
Collaborator

RayBB commented Dec 4, 2025

If the code expects a string of true/false this will fix that. Would be ideal to instead have it accept an actual bool but that would probably break the web.py endpoint. So this seems like a good middle ground for now.

@RayBB
Copy link
Collaborator

RayBB commented Dec 5, 2025

@cdrini pushed up a commit fixing the test for you!

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized this leads to api divergence.
If you want it to be completely the same it needs to be
public_scan_b: list[Literal["true", "false"]]

Or
Remove
public_scan_b=[] from worksearch/code.py

I think that the second one makes more sense.

In this PR I wrote tests to ensure the API contracts are an exact match and it caught the bug this PR fixes but also made me realize the above.
#11549

Also related to maybe we don't even want public_scan_b ?
#7272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants