shrinerb / shrine

File Attachment toolkit for Ruby applications
https://shrinerb.com
MIT License
3.17k stars 274 forks source link

Skip serialization for all JSON/JSONB attributes in activerecord plugin #655

Open vojtad opened 1 year ago

vojtad commented 1 year ago

Use type_for_attribute instead of columns_hash to check whether an attribute type is JSON or JSONB. Serialization is now skipped not only for database attributes but also for attributes defined using Attributes API.

This is a breaking change. Do you want to hide it behind a feature flag or how should I approach this?

janko commented 1 year ago

Thanks for the patch, it looks good to me 👍🏻 In what way is this a breaking change?

vojtad commented 1 year ago

If you have attribute :avatar_data, :json defined in your model then avatar_data returned a String and you could call JSON.parse on it to get an object before.

With this patch avatar_data returns an object without any need for deserializing it because it never gets serialized since the attribute is a JSON attribute.

Here is an example code which throws an exception when calling `JSON.parse` when run with this patch. ``` require 'active_record' require 'minitest' require 'shrine' require_relative '../test/support/file_helper' require_relative '../test/support/shrine_helper' include FileHelper include ShrineHelper ActiveRecord::Base.establish_connection( adapter: "sqlite3", database: ":memory:", ) ActiveRecord::Base.connection.create_table(:users) do |t| t.string :name end @shrine = shrine do plugin :activerecord plugin :backgrounding end user_class = Class.new(ActiveRecord::Base) user_class.table_name = :users user_class.class_eval do # needed for translating validation errors def self.model_name ActiveModel::Name.new(self, nil, "User") end attribute :avatar_data, :json end user_class.include @shrine::Attachment.new(:avatar) user = user_class.new pp user.avatar_data user.avatar = fakeio user.save! pp user.avatar_data # user.avatar_data used to be a String so we can call JSON.parse to deserialize it pp JSON.parse(user.avatar_data) ```