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

Json columns are read as JSON::PullParser instead of JSON::Any #232

Merged
merged 2 commits into from Jul 8, 2021

Conversation

matthewmcgarvey
Copy link
Contributor

Fixes #125

Currently, if you have a json column and read it with this library, it is automatically turned into a JSON::Any. It makes sense when you don't care about the structure, but there's two problems with doing that:

  1. JSON::Any is immutable so that values cannot be changed and then saved back to the database
  2. You cannot convert a JSON::Any into a JSON::Serializable

This PR changes the default deserialization of json columns from JSON::Any to JSON::PullParser. The benefit we get from this is that it can easily be converted into a JSON::Any or a JSON::Serializable. It is still not mutable but it's easier to get to a mutable setting:

properties = result_set.read(JSON::PullParser)
mutable_properties = Hash(String, String).new(properties)

I added a method specifically for still supporting calling result_set.read(JSON::Any) with hopes that will cause the least amount of breakage, because this is definitely a breaking change.

Copy link
Collaborator

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

LGTM. I feel we need a read_io directly that will allow us to use the stream directly into de pullparser or any other consumer of IOs. I'm mentioning this to share this idea that should be done in crystal-db at some point I think.

@will
Copy link
Owner

will commented Jul 8, 2021

Thanks! Could you please add a line to the changelog file about this?

@will will merged commit 69e8605 into will:master Jul 8, 2021
@will
Copy link
Owner

will commented Jul 8, 2021

Awesome, thanks!

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.

Premature parsing of json
3 participants