Skip to content

Update wrap_jsonp to handle lists#12397

Open
jimchamp wants to merge 2 commits intomasterfrom
wrap_jsonp-update
Open

Update wrap_jsonp to handle lists#12397
jimchamp wants to merge 2 commits intomasterfrom
wrap_jsonp-update

Conversation

@jimchamp
Copy link
Copy Markdown
Collaborator

Addresses regression identified here

starlette.responses.Response.render is throwing an AttributeError when data is a list. These changes ensure that lists are also serialized.

Technical

Testing

Screenshot

Stakeholders

@RayBB
@cdrini

Copilot AI review requested due to automatic review settings April 17, 2026 01:20
Copy link
Copy Markdown
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

Updates the FastAPI wrap_jsonp helper to correctly JSON-serialize list responses, preventing starlette.responses.Response.render errors when endpoints return [].

Changes:

  • Expand wrap_jsonp input type to accept list in addition to dict/str.
  • Serialize dict/list inputs via json.dumps while preserving pre-serialized JSON strings.
  • Add an explicit type guard for unexpected data types.
Comments suppressed due to low confidence (1)

openlibrary/fastapi/models.py:77

  • wrap_jsonp raises ValueError when the callback query param is invalid. Since this helper is used directly in FastAPI route handlers (e.g., openlibrary/fastapi/books.py) and there is no global exception handler for ValueError in the ASGI app, this will surface as a 500 error for a client-supplied bad callback. Prefer raising HTTPException(status_code=400, ...) so invalid user input returns a 4xx response (and update the existing unit test that currently expects ValueError).
    if callback := request.query_params.get("callback"):
        if not JS_CALLBACK_RE.match(callback):
            raise ValueError("Invalid callback parameter: must be a valid JavaScript identifier (only letters, numbers, underscore, $, and . allowed)")
        return Response(content=f"{callback}({json_string});", media_type="application/javascript")

Comment on lines +67 to +70
if isinstance(data, str):
json_string = data
elif isinstance(data, (dict, list)):
json_string = json.dumps(data)
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

List support was added to wrap_jsonp (serializing list via json.dumps), but the existing unit tests for wrap_jsonp (see openlibrary/tests/fastapi/test_models.py) don’t cover the list case. Please add a test that passes a list (with and without a callback) to prevent this regression from reappearing.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Implemented in 81fbb53. I added regression tests in openlibrary/tests/fastapi/test_models.py covering list payloads for both cases: no callback (application/json) and with callback (application/javascript).

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 17, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api-inference.huggingface.co
    • Triggering command: /opt/java/openjdk/bin/java /opt/java/openjdk/bin/java -server -Xms512m -Xmx512m -XX:+UseG1GC -XX:+PerfDisableSharedMem -XX:+ParallelRefProcEnabled -XX:MaxGCPauseMillis=250 -XX:+UseLargePages -XX:+AlwaysPreTouch -XX:+ExplicitGCInvokesConcurrent -Xlog:gc*:file=/var/solr/logs/solr_gc.log:time,uptime:filecount=9,filesize=20M -Dsolr.jetty.inetaccess.includes= -Dsolr.jetty.inetaccess.excludes= -Dsolr.log.dir=/var/solr/logs -Djetty.port=8983 -DSTOP.PORT=7983 -DSTOP.KEY=solrrocks -Djava.io.tmpdir=/tmp -Duser.timezone=UTC (dns block)
  • api.mistral.ai
    • Triggering command: /opt/java/openjdk/bin/java /opt/java/openjdk/bin/java -server -Xms512m -Xmx512m -XX:+UseG1GC -XX:+PerfDisableSharedMem -XX:+ParallelRefProcEnabled -XX:MaxGCPauseMillis=250 -XX:+UseLargePages -XX:+AlwaysPreTouch -XX:+ExplicitGCInvokesConcurrent -Xlog:gc*:file=/var/solr/logs/solr_gc.log:time,uptime:filecount=9,filesize=20M -Dsolr.jetty.inetaccess.includes= -Dsolr.jetty.inetaccess.excludes= -Dsolr.log.dir=/var/solr/logs -Djetty.port=8983 -DSTOP.PORT=7983 -DSTOP.KEY=solrrocks -Djava.io.tmpdir=/tmp -Duser.timezone=UTC (dns block)
  • langchain4j.dev
    • Triggering command: /opt/java/openjdk/bin/java /opt/java/openjdk/bin/java -server -Xms512m -Xmx512m -XX:+UseG1GC -XX:+PerfDisableSharedMem -XX:+ParallelRefProcEnabled -XX:MaxGCPauseMillis=250 -XX:+UseLargePages -XX:+AlwaysPreTouch -XX:+ExplicitGCInvokesConcurrent -Xlog:gc*:file=/var/solr/logs/solr_gc.log:time,uptime:filecount=9,filesize=20M -Dsolr.jetty.inetaccess.includes= -Dsolr.jetty.inetaccess.excludes= -Dsolr.log.dir=/var/solr/logs -Djetty.port=8983 -DSTOP.PORT=7983 -DSTOP.KEY=solrrocks -Djava.io.tmpdir=/tmp -Duser.timezone=UTC (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants