Support storing settings/category_settings as jsonb types

[TylerRick]

Describe the solution you'd like

I'd like to change, or have the option to change, the settings, category_settings columns from text type to jsonb.

JSON-serialized data seems to be a better and more standard option than YAML-based serialization. Using a JSONB data type in the database makes it possible to efficiently search, validate, and manipulate the data in those columns at the database level.

For more about using jsonb types in Rails, see: https://guides.rubyonrails.org/active_record_postgresql.html#json-and-jsonb

Implementation required / options

Change data type

This is something the end user can do if they want — just edit the generated migration to use:

t.jsonb :settings
t.jsonb :category_settings

instead of

t.text :settings
t.text :category_settings

like they do here: https://github.com/paper-trail-gem/paper_trail#postgresql-json-column-type-support

Change/disable the serializer

Currently, we have this in notification-settings/app/models/notification_settings/setting.rb:

    serialize :settings, Hash
    serialize :category_settings, Hash

Unfortunately that doesn't seem to be compatible with jsonb columns. When a class name (Hash) is passed to serialize, it serializes it as a YAML document (see serialize docs).

We need to a way to either have it not call serialize at all... or a way to specify which "coder" to use ... or have it check the column's data type and automatically do the right thing

Since it's just as easy, and (something like this) might even be necessary (in order to allow the hash to still be accessible with symbol keys like the code currently does), I'd recommend just allowing a different serializer to be specified in the configuration.

This is how I have my app configured currently and is working so far: (based on HashSerializer example from https://nandovieira.com/using-postgresql-and-jsonb-with-ruby-on-rails)

class JsonColumnHashWithIndifferentAccessSerializer
  def self.dump(hash)
    hash
  end

  def self.load(hash)
    (hash || {}).with_indifferent_access
  end
end

module NotificationSettings
        Setting
  class Setting
    serialize :settings,          JsonColumnHashWithIndifferentAccessSerializer
    serialize :category_settings, JsonColumnHashWithIndifferentAccessSerializer
  end
end

(Note: something like this may also solve #38)

So far, this didn't even require any changes to the gem, since calling serialize :settings again appears to override any previous serialization configuration you may have. But I'd prefer it if the gem officially supported jsonb and provided something like a config.hash_serializer option so end user doesn't have to monkey-patch the gem. (Better yet, configure itself automatically as mentioned below.)

Challenge: Remaining compatible with databases other than Postgres

I assume it wouldn't be an acceptable solution to require use of PostgreSQL... so the question is, can we take advantage of jsonb columns if the schema uses them but still make it work for databases that don't have them? (MySQL actually has a JSON data type too but I don't know much about it.)

I think we can. The changes suggested above don't break compatibility with the old way; they just allow users to use the much nicer jsonb type if they want to (I sure do).

Can we automatically detect data type and do the right thing?

Probably, but not sure how yet...

Then we wouldn't have to add any new config options. (And it could detect it on a per-column basis so folks could use jsonb for whichever hash columns they want; wouldn't force you to use it for all of them. There's also serialize :metadata, Hash in notification-handler.)

This is what PaperTrail does. Unfortunately, I don't think it's safe to check columns_hash in the model's class definition where we're calling serialize. PaperTrail gets away with it because it handles the deserialization itself when the object/object_changes attribute readers are called (object_deserialized method, actually) and doesn't call serialize in the class definition like we're doing. Hmm.

[TylerRick]

Further exploration: Integrate with attr_json gem?

Having successfully changed the columns to jsonb in my app, I'm about to try integrating with the amazing-looking attr_json gem.

It could give us more control over the data type of the elements inside the settings/category_settings/metadata attributes. A plain nested Ruby Hash is kind of a terrible data type, if you think about it:

• You access things with hash[:key] instead of object.key like every other Ruby object (you can't just send it key or key= messages).
• You have to decide whether keys should be strings or symbols (or allow both, and use HashWithIndifferentAccess).
• You don't have much control over defaults (#38), especially for nested hashes.

... You get the idea. Can't we do better than that?

Imagine something like this, for example:

# Needs better name. (`NotificationSetting` conflicts with module name.)
class SpecificNotificationSetting
  include AttrJson::Model

  attr_json :enabled, :boolean, default: true
  # Not sure what else we really need since so far I only care about :enabled.
  # I guess the ability to enable/disable specific pushers?
  # Question: Could we add more pushers to this type later if they are dynamically registered?
  # Maybe it doesn't matter if it's not a required attribute; it probably allows keys in addition to those defined here...
  attr_json :ActionMailer, :boolean, default: true
end

class NotificationSettings::Settings < ActiveRecord::Base
  include AttrJson::Record
  include AttrJson::Record::QueryScopes

  attr_json :settings,          SpecificNotificationSetting.to_type
  attr_json :category_settings, SpecificNotificationSetting.to_type, array: true
end

It would be almost as if you're using a has_many and another database-backed for each "setting" inside the settings attribute ... except lighter weight since there's really no need for that model to have its own database table.

We could even add proper validations to ensure that only valid (and properly formatted) data gets saved in those loosely structured attributes...

I think it could also potentially make dealing with forms nicer (#39)... Just like if we were using has_many :category_settings with accepts_nested_attributes_for, attr_json gives us attr_json_accepts_nested_attributes_for...

Assuming that goes well, I really wish we could just make a jsonb column a requirement so we could make use of attr_json in this gem. But since that's probably not an option, I'll see if I can add it in the app without any changes to the gem...

[jonhue]

I don't know if I just got this wrong, but this problem could be resolved, temporarily, by providing a configuration option hash_serializer that defaults to Hash. Right?

[TylerRick]

Right. Well, technically, you don't have to change anything, as I managed to get it working without any changes to notifications-rails. I guess it depends how well you/we want this gem to support it?

I don't know if I just got this wrong, but this problem could be resolved, temporarily, by providing a configuration option hash_serializer that defaults to Hash. Right?

So, yes, that would allow users to at least have control as to whether it serializes using YAML (which it does by default when you use Hash) or JSON or whatever they wanted, without having to re-open/monkey patch the NotificationSettings::Setting class themselves. I think that would be a nice gesture to users. (I wish I knew what other libraries were doing with things like this...)

They would still have to provide their own serializer, though, unless we wanted to include one for JSON. Maybe we could provide one (under some NotificationRails namespace so we don't add another global consant?), since it would be easy to shoot one's self in the foot. It needs to use .with_indifferent_access, for example, or this gem's symbol lookups would fail. (But that conversion arguably should belong somewhere else and not need to be part of the serializer. {} and {}.with_indifferent_access both serialize to {} in JSON, after all.)

I think my recommendation currently would be:

• Add a config.hash_serializer option under notification-settings and notification_handler (for serialize :metadata)
• Add a NotificationRails::JsonColumnHashWithIndifferentAccessSerializer serializer
• Allow config.hash_serializer to be set to a class or false or :json
  • If false don't even call serialize. (It caused problems when I tried to use attr_json. Though I may not end up using attr_json...)
  • Let :json be a shortcut for whatever default/recommended/compatible JSON serializer we add
• Add tests so we can ensure everything functions correctly with either (supported) option