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

Added JSON Type #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aurimasniekis
Copy link

I have tried to implement native json type. With this code it works for reading and parsing. But I don't know if it's correct way to implement it.

Especially with this part:

def self.type_for(t : JsonType.class)
    MySql::Type::JSON
end

@bcardiff
Copy link
Member

ResultSet#read should receive the class of object that expected to be returned.
In this case I would expect that rs.read(JSON::Any) will return the next value as a JSON::Any.

Since JSON::Any is a union there might be some issue, but at least from the following snippet it seems the overload are picked in correctly.

require "json"

def f(t : JSON::Any.class)
  "a json any"
end

def f(t : String.class)
  "a string"
end

pp f(JSON::Any) # => "a json any"
pp f(String)    # => "a string"

I would say to try that way first.

When writing the json object to the packet we should write directly to the IO. That might require some new methods in packet probably.

@aurimasniekis
Copy link
Author

Sorry, but my current knowledge is really limited with Crystal, so I can try to help but not much more :D

@spalladino
Copy link
Contributor

@aurimasniekis no worries, everyone started out with a really limited knowledge here :-)

I'd suggest going through how support for other types is implemented. As @bcardiff mentioned, ResultSet#read typically receives as an argument the class of the object to read, so the first step would be to add support for JSON::Any there. I'd also advise to try and become familiar with JSON parsing in Crystal, which can be quite difficult (especially if coming from a dynamic language).

The other thing Brian was mentioning was to write the JSON directly on the stream. One of the goals of all crystal-db methods (and its drivers) is to be as efficient as possible by not allocating unnecessary memory. So, instead of serializing an object to a string, and copying that string to the data stream, you could try and serialize the object directly onto the stream (note that to_json has an overload that receives an IO, which could help here).

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