-
Notifications
You must be signed in to change notification settings - Fork 661
fix(json): handle BLOB arguments correctly in JSON functions #4213
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
base: main
Are you sure you want to change the base?
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.
Please review @pedrocarlo
d7aaafb to
7c5a58e
Compare
|
Messed up the rebase, fixing it now 🙃 Edit: Done! |
7c5a58e to
dd26104
Compare
|
@jussisaurio or @PThorpe92 need someone to approve the workflow here to run the test |
dd26104 to
397a47a
Compare
|
Fixed the failing |
|
|
||
| do_execsql_test_in_memory_error_content json_object_blob_error { | ||
| CREATE TABLE t(a); | ||
| INSERT INTO t VALUES (x'74727565'); -- ASCII "true"; |
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.
could u please add more tests? specifically like using jsonb? are we supposed to reject them too?
json_funcs(jsonb())
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.
You are absolutely right, thanks for catching that! I have pushed a commit adding more tests for the jsonb functions and for json/jsonb interop. Let me know if these tests cover what you were expecting.
Also, while adding these tests, I discovered a bug: Turso wasn’t unescaping strings when converting jsonb to db/sql values, whereas SQLite does.
397a47a to
61ec540
Compare
61ec540 to
fcfc047
Compare
fcfc047 to
2442522
Compare
Description
This PR updates JSON functions to correctly reject BLOB values, aligning their behavior with SQLite. It also fixes handling of text-based JSON passed as BLOB arguments to
json(), which previously did not work as expected.Previously, JSON functions (
json_object,json_set,json_insert,json_replace, and theirjsonbvariants) attempted to convert BLOB values into JSON instead of rejecting them with an error. This differed from SQLite's behavior, which returns the errorJSON cannot hold BLOB values. In addition, these functions were silently converting errors toNULLin the executor rather than propagating them to the user.Additionally,
json()previously returned incorrect results when given BLOBs containing JSON text. The root cause was that the function assumed all BLOBs were in binary JSONB format and attempted to parse them as such, leading to incorrect results when the BLOB actually contained textual JSON.With these changes applied, JSON functions are matching SQLite's behavior:
Motivation and context
This PR fixes the issues identified in #4195 to align with SQLite’s behavior.
Currently, Turso handles BLOB inputs inconsistently across JSON functions:
Expected behavior:
Fixes #4195.
AI Disclosure
This PR was developed with assistance from GitHub Copilot (Claude Sonnet 4.5). The AI helped identify the root cause and assisted in writing the tests.