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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vojtad
Copy link

@vojtad vojtad commented Sep 13, 2023

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?

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.
@vojtad vojtad force-pushed the use-rails-attributes-api-in-activerecord-plugin branch from 6883350 to 15923e2 Compare September 13, 2023 10:48
@vojtad vojtad changed the title Draft: Skip serialization for all JSON/JSONB attributes in activerecord plugin. Skip serialization for all JSON/JSONB attributes in activerecord plugin. Sep 13, 2023
@vojtad vojtad changed the title Skip serialization for all JSON/JSONB attributes in activerecord plugin. Skip serialization for all JSON/JSONB attributes in activerecord plugin Sep 13, 2023
@janko
Copy link
Member

janko commented Sep 17, 2023

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

@vojtad
Copy link
Author

vojtad commented Sep 18, 2023

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)

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.

2 participants