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

Support mounting an attribute at the top level of a JSON column instead of mounting it to a key _within_ a JSON column container #45

Closed
TylerRick opened this issue Jan 3, 2019 · 5 comments · Fixed by #89

Comments

@TylerRick
Copy link

TylerRick commented Jan 3, 2019

attr_json provides good support for rich data types as values inside a JSON object (ruby hash). I think it would be useful to also be able to use a rich data type directly on a JSON attribute itself (at the "top level" instead of just for keys inside it).

I tried this:

class Setting < ActiveRecord::Base
  class Settings
    include AttrJson::Model

    attr_json :enabled, :boolean, default: true
    # ...
  end

  attr_json :settings, Settings.to_type, container_attribute: :settings, store_key: nil
end

but not surprisingly, it didn't work (caused stack level too deep) because calling settings tries to look inside its "container", settings. But really, there is actually no container; we just want that JSON attribute itself to have a given type.

(

  attr_json :settings, Settings.to_type, container_attribute: nil, store_key: nil

would probably be more accurate)

You'll probably tell me, that's not what attr_json is for—just use Rails's built-in attribute API! :)

And then I'll say, I tried that, but it doesn't work out of the box currently.

I tried this:

  attribute :settings, Setting.to_type

... which got me pretty close, but it ran into this error when it tried to cast:

main > s.settings
AttrJson::Type::Model::BadCast: Can not cast from "{\"enabled\": true}" to specific_notification_setting
from /home/tyler/.gem/ruby/2.4.5/gems/attr_json-0.5.0/lib/attr_json/type/model.rb:43:in `cast'

When I don't add an explicit attribute :settings line, it automatically casts between a Ruby Hash and JSON string automatically, so it must implicitly set the attribute type of JSONB columns somehow.

I don't know how to introspect into the type of attributes, but I figured out how to get the type of the underlying column:

main > s.class.columns_hash['settings'].instance_eval { @cast_type }
=> #<ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Jsonb:0x000055961f437000 @limit=nil, @precision=nil, @scale=nil>

main > ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Jsonb.superclass.superclass
=> ActiveRecord::Type::Internal::AbstractJson

main > $ ActiveRecord::Type::Internal::AbstractJson#deserialize
def deserialize(value)
  if value.is_a?(::String)
    ::ActiveSupport::JSON.decode(value) rescue nil
  else
    value
  end
end

So I tried adding this to Model#cast in attr_json/type/model.rb:

        elsif v.kind_of? String
          # TODO Maybe we ought not to do this on #to_h?
          model.new_from_serializable(::ActiveSupport::JSON.decode(v))

And it works!

main > s.settings_before_type_cast
=> "{\"enabled\": true}"

main > s.settings
=> #<SpecificNotificationSetting:0x000055961fb90a40 @attributes={"enabled"=>true, "ActionMailer"=>true}>

... except it still doesn't work for saving...

main > s.settings = NotificationSettings::Setting::Settings.new
=> #<NotificationSettings::Setting::Settings:0x0000560d3957cac8 @attributes={"enabled"=>true, "ActionMailer"=>true}>

main > s.save
TypeError: can't quote Hash
from ~/.gem/ruby/2.4.5/gems/activerecord-5.1.6/lib/active_record/connection_adapters/abstract/quoting.rb:196:in `_quote'

(Same error if you set it directly to a hash:

s.settings = {enabled: false}

)

When I dug into it a bit to see how it was able to save regular attr_json fields that use custom type without getting that error, I discovered that even though the custom type serializes to a Hash, the container attribute that it's contained within takes care of serializing those Hashes and everything inside of it to JSON:

lib/attr_json/type/container_attribute.rb:

   36:         super(v.collect do |key, value|
   37:           attr_def = model.attr_json_registry.store_key_lookup(container_attribute, key)
=> 38:           [key, attr_def ? attr_def.serialize(value) : value]
   39:         end.to_h)

(AttrJson::Type::ContainerAttribute's superclass is ActiveRecord::Type::Internal::AbstractJson (I'm on Rails 5.1).)

How could we get it to work?

It seems like we just need the AttrJson::Model to serialize to a Hash when in a container and to a JSON string when used directly on a JSON attribute. What's the best way to do that?

I don't suppose there's an easy way to detect if it's in a container not within the serialize method...

What about adding another conversion, like to_json_serialized_type or something, that you can call instead of to_json if you want it to serialize directly to/from JSON instead of to/from a Hash?

Why would you want this?

Mostly so you can reuse the same types both directly on an attribute and for attributes stored inside a container attribute without having to duplicate the logic in your own custom type.

A random example...

attribute :preferred_language, Language.to_type
attr_json :languages_spoken, Language, array: true

notifications-rails

The particular use case that led me to this idea was was trying to make a nicer API for accessing settings in a NotificationSettings::Setting::Setting model object.

Since hashes are such a pain to work with, I was trying to change it to use jsonb, so we could do something like:

s.settings.enabled = false
s.category_settings.some_category.enabled = false

instead of

s.settings[:enabled] = false
s.category_settings[:some_category][:enabled] = false
@TylerRick TylerRick changed the title Support mounting an attribute at the top level of a JSON column instead of mounting it to a key within a JSON column container Support mounting an attribute at the top level of a JSON column instead of mounting it to a key _within_ a JSON column container Jan 4, 2019
@jrochkind
Copy link
Owner

Yeah, I'm leaning on the default ActiveRecord json serialization to serialize the top-level.

I do see the value of this, although I don't need it myself. I'm scared of any increase in code complexity. :)

I still wonder if you need attr_json for it. Hmm. Just brainstorming too here...

So:

   attribute :settings, SomeActiveModelType.new

Would work, if SomeActiveModelType.new is something that adheres to the ActiveModel Type interface to succesfully turn something to and from it's database representation. Which, yeah, an AttrJson::Model does not.

How does Rails handle ordinary json/jsonb columns? Check out the ActiveRecord::Type::Json. It pretty much does what you already figured out -- just serializes and deserializes to actual strings.

We can see that the type used for postgres jsonb doesn't seem to do anything different than that either. (Confirmed with introspection that the parent class of ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Jsonb is indeed just ActiveRecord::Type::Json)

So, yes, when you do AttrJsonModel.to_type, you get an instance of one of our AttrJson::Type::Model objects. Which you can see, yes, converts to/from a hash, not a string representation.

Rather than make AttrJson::Type::Model do more than one thing, I think you want a new type.

The important methods for complying with the Rails' own ActiveModel Type interface (the one used in rails attribute, also what we use internally) are, as you can see, #serialize, #deserialize, and #cast. #changed_in_place? is also important for mutable values, as these are. And #type is useful mainly for introspection/logging/error messages etc.

Due to the good OO design of Rails ActiveModel type, which we follow here, I think you could make a new type something like the following, just delegating to both relevant existing types. You don't actually need AttrJson changes to make this work, so I'd suggest you try it out and if you get it working well and well-tested, perhaps PR it here to share.

It might look something like this (I am typing this into this comment, haven't even had MRI parse it)

class TopLevelAttrJsonModelType
   attr_reader :model_type
   def initialize(attr_json_model)
       @model_type = attr_json_model.to_type
       @wrapper_type = ActiveRecord::Type::Json.new
   end

   def type
      model_type.type
   end

    def cast(*args)
        wrapper_type.cast( model_type.cast(*args) )
    end

    def serialize(*args)
       wrapper_type.serialize( model_type.serialize(*args) )
    end

    def deserialize
       wrapper_type.deserialize( model_type.deserialize(*args) )
    end

    def changed_in_place?(*args)
        model_type.changed_in_place?(*args)
    end
end

Then you'd just tell rails, using the standard activerecord attribute method:

 attribute :settings, TopLevelAttrJsonModelType.new(Settings)

The beauty of the newish ActiveRecord/Model Type API, is you can just give it anything that complies with the fairly simple interface. Which is how attr_json does it's stuff too. The above might Just Work. I suggest playing around with that in your app and see what you think. If you can get it well-tested and solid and be sure you understand all the implications, I'd be happy to consider it as a PR.

@jrochkind
Copy link
Owner

I realize my above psuedo-code isn't quite right, the order of wrapper_type and model_type depends on the method. For instance, deserialize is probably:

    def deserialize
       model_type.deserialize( wrapper_type.deserialize(*args) )
    end

Get why? You just gotta play around with it, but I think something basically along these lines would probably work.

@jrochkind
Copy link
Owner

I have no opinion on if this would actually be a good idea in notification-rails or not. I'm not familiar with that gem, but adding attr_json into it would add significant complexity, may not be worth it unless it adds a lot of value. Or it might be! No opinion!

@jrochkind
Copy link
Owner

There is another new gem that provides support for "top-level" serialized-to-json models. https://github.com/DmitryTsepelev/store_model

It doesn't do everything attr_json does, and I'm not sure if it would allow "nested" models.

The existence of that gem does kind of make me want to add the "top-level" model feature to attr_json to "compete" with it, although I'm not sure I'll have time to get to it anytime soon, but happy to review a PR!

@jrochkind
Copy link
Owner

Hey, I'm coming back to this, not sure if you're still around and interested, @TylerRick ?

I think I figured out a way to do this supported by the not-so-well-known ActiveRecord serialization feature, that pretty much already works, with very little additional code.

It needs a tiny shim in your local app, but I'm looking to write a tiny bit of code in attr_json to support it in a documented and tested way.

If anyone with this use case wants to try it out, that'd be super helpful!

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 a pull request may close this issue.

2 participants