Skip to content
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

Detect JSON subtypes and JSON.parse() before returning. #106

Merged
merged 3 commits into from
Aug 6, 2023

Conversation

asg017
Copy link
Contributor

@asg017 asg017 commented Aug 4, 2023

This PR adds a new potentially backwards-incompatible update: for columns that are the return values of JSON functions, it'llJSON.parse() the text value and return list objects instead of serialized JSON text.

When using SQLite's builtin JSON functions, SQLite will give JSON values a special "subtype" value. We can use this "JSON subtype" to detect if a value is valid JSON. If so, we can parse the text string before returning the values back to the user.

An example:

const [list] = db
  .prepare("SELECT json_array(1, 2, 3) as list")
  .value<[number[]]>()!;
// list = [ 1, 2, 3 ]

const [object] = db
  .prepare("SELECT json_object('name', 'Peter') as object")
  .value<[{ name: string }]>()!;

// object = { name: "Peter" }

This also works for all the other JSON builtin functions, including json(), json_each(), json_tree(), and more. Other SQLite extensions can also use the JSON subtype convention in their own custom functions.

Potentially backwards incompatible

Before, these functions would just return a string, and users would probably JSON.parse() it themselves. But now, it can possibly return arrays, objects, decoded JSON strings and numbers, etc. Because of that, I think merging this PR as-is would be a major version bump (well, minor since it's pre-v1)

However, we could also add a new database-level setting to make this new behavior opt-in. Maybe something like db.enableJsonDeserializing(true) or something in the constructor. Happy to include if you think it's best!

@DjDeveloperr
Copy link
Member

LGTM, good addition! What do you think about serializing objects using JSON by default when binding parameters?

@asg017
Copy link
Contributor Author

asg017 commented Aug 5, 2023

@DjDeveloperr I dig that! I'll push that in some new commits in a bit

@asg017
Copy link
Contributor Author

asg017 commented Aug 6, 2023

Ok @DjDeveloperr just pushed that feature - if an object or array is passed as a bound parameter, then JSON.stringify() is called on the value, and that text value is bounded as a parameter. I also updated the BindValue type to handle those cases.

Although now that I think about it, there might be some bugs. For example, if you bind an array of Dates, it would call JSON.stringify() on those dates instead of toISOString() on each individual date.

Let me know if you want me to update it in anyway! Not sure how easy it'll be to fix the Date stuff, short of writing a custom serializer or something...

@DjDeveloperr
Copy link
Member

DjDeveloperr commented Aug 6, 2023

Shouldn't JSON.stringify on Date return same value as toISOString?
MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toJSON

I think it makes sense to just follow the usual JSON.stringify semantics.

@asg017
Copy link
Contributor Author

asg017 commented Aug 6, 2023

Ah yup, totally missed that, good catch! Can't think of much else for this PR then, lmk if there's anything else to add

Copy link
Member

@DjDeveloperr DjDeveloperr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@DjDeveloperr DjDeveloperr merged commit 5b23fc6 into denodrivers:main Aug 6, 2023
5 checks passed
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.

2 participants