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

Jsonb support #48

Closed
wants to merge 2 commits into from
Closed

Jsonb support #48

wants to merge 2 commits into from

Conversation

tzar
Copy link

@tzar tzar commented Feb 4, 2019

See luckyframework/lucky_record#275 for prior discussion

Ninja edit: closes #18 and luckyframework/lucky#612

@paulcsmith
Copy link
Member

This looks awesome! Have tried this in a real app? I’d like to make sure it works as expected before merging it in as I’m not as familiar with JSON and Postgres and I don’t have an app to try it on

@tzar
Copy link
Author

tzar commented Mar 22, 2019

That's fair enough - when I added this feature, I was just prototyping something out quickly; unfortunately I have been busy with other projects since then so haven't updated it since. In other words, at one point it worked and in my rebases I have relied on the specs 😉

I'm hoping to get back to that project later this year, so perhaps it would be wise to wait until something has proven it or alternatively set up a simple test app. Cheers

@paulcsmith
Copy link
Member

Thanks for the heads up and quick response @tzar! I will try to test this out sometime. Even if it doesn’t work in its current state, your work will certainly help move this forward. So thank you! And good luck with your current projects :)

@jwoertink
Copy link
Member

Ok, I have an app I need this on, so I'm gonna give it a shot and see how it goes. I'll report back after I've had a chance to try it out

when String, UUID
value.to_s
else
value.to_json
Copy link
Member

Choose a reason for hiding this comment

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

Even though 1.to_s and 1.to_json return the same value, calling to_json is way slower. We should only call to_json on Array, Hash, or NamedTuple. Maybe there's a better way for that to read?

@@ -1,5 +1,5 @@
module Avram::Migrator::ColumnDefaultHelpers
alias ColumnDefaultType = String | Time | Int32 | Int64 | Float32 | Float64 | Bool | Symbol
alias ColumnDefaultType = Object
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. Not sure about this. It is technically cleaner, but one goal of Avram is to be VERY typesafe, and this would technically allow something like Tuple or Point to match because Tuple.is_a?(Object). @paulcsmith what are your thoughts on this?

@jwoertink
Copy link
Member

Ok, I just finished going over this. For the most part it's pretty good. One thing I'm having issues with is after saving the record, pulling the data back out is a little strange.

p = PreferenceQuery.new.first

# This fails with no method overload
PreferenceForm.update!(p, notifications: {"mobile" => true})

# This also fails with no method overload
PreferenceForm.update!(p, notifications: {"mobile" => true}.to_json)

# This saves and updates the record
PreferenceForm.update!(p, notifications: JSON::Any.new({"mobile" => true}.to_json))

p = PreferenceQuery.new.first
p.notifications
# => "{\"mobile\":true}"
p.notifications["mobile"]
# Unhandled exception: Expected Hash for #[](key : String), not String (Exception)
#   from JSON::Any#[]<String>:JSON::Any
#   from __icr_exec__:JSON::Any
#   from __crystal_main
#   from Crystal::main_user_code<Int32, Pointer(Pointer(UInt8))>:Nil
#   from Crystal::main<Int32, Pointer(Pointer(UInt8))>:Int32
 #  from main
p.notifications.class
# => JSON::Any
JSON.parse(p.notifications.as_s)
{"mobile" => true}

p = PreferenceQuery.new.first
# This stores it as you'd actually expect 
PreferenceForm.update!(p, notifications: JSON::Any.new({"mobile" => JSON::Any.new(true)}))
p.notifications
# => {"mobile" => true}
p.notifications["mobile"]
# => true

So as you can see, saving the field does technically work, but you can save a single string as JSON which means that this could lead to some error with data. If you're storing a ton of data as a string and don't realize until later, this could cause a huge issue.

I think it would be nice to have a few subtypes from JSON::Any to specify something like a field is very specifically an Array(String) that gets checked and mapped for json, or this field is very specifically Hash(String, String), for example. What that looks like, I'm not totally sure yet....

@jwoertink jwoertink mentioned this pull request Jun 15, 2019
@paulcsmith
Copy link
Member

Closing in favor of #108

Thank you so much for your work on this @tzar! We'e making a few small tweaks and then this will be in! 🎉

@paulcsmith paulcsmith closed this Jun 16, 2019
@tzar
Copy link
Author

tzar commented Jun 17, 2019

awesome 👍

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.

Add support for Postgres JSON/JSONB column types
3 participants