Support defining multiple stores with the same store_attribute

[AlphonseSantoro]

We are currently using StoreExt in our projects which is now archived and not maintained anymore. Upon migrating one of our projects I discovered one case that is not supported here. I also came across #36 which also touches the same issue.

In some of our models we have defined a store with fields specific for that model and also included a store that is common among other models.
F.ex

module AddressStore
  extend ActiveSupport::Concern

  included do
    typed_store :data do |s|
      s.string :address_city
      s.string :address_zip
    end
  end
end

class User
 include AddressStore

  typed_store :data do |s|
    s.string :phone_number
  end
end

User.store_accessors # => #<Set: {"phone_number"}>

The example above results in a redefinition of the store constant and the class variable store_accessors contain only the last defined store. You can still access the all the fields via the accessors, but you lose the typed store for the first defined store.

This PR solves this by:

• Only defining the constant when necessary. When a parent class has an existing store defined the sub class defines it including the fields from the parent class (Any duplicated fields is overridden by the sub class). When the class itself has a store defined, the stores are combined (Any duplicated fields is overridden by the last defined store).
• Due to CVE-2022-32224 serialization fails due to the types used. I've decided to set this in the tests only and not set this as a default to not surprise anyone using this gem. It's then up to the appliccation to add the permitted classes.

[AlphonseSantoro]

@byroot This is ready for review

[byroot]

I'm in vacation this week. I'll try to have a look this week end.

[casperisfine]

Ok, so I had a quick look at this, and I don't think it's typed-store job to work around such a problem.

I think we should improve this in Active Record, either have a store "inheritance" behavior or either explicitly break when such case happens.

[AlphonseSantoro]

Just to clarify, the rails store works as intended and store attributes are inherited properly there.
The issue here is that when you define more than one store the previous one is discarded. Methods defined by the previous store is still accessible, but you then lose all information on the stored type including which attributes are defined.

I would expect the method typed_store to behave the same way as the rails store method:

# From https://api.rubyonrails.org/classes/ActiveRecord/Store.html
class User
  store :settings, accessors: [ :two_factor_auth ]
  store :settings, accessors: [ :login_retry ]
end
User.stored_attributes[:settings] # => [:two_factor_auth, :login_retry]

But if you do the same with typed_store:

class User
  typed_store :settings, accessors: [ :two_factor_auth ]
  typed_store :settings, accessors: [ :login_retry ]
end
# Typed store:
User.store_accessors # => #<Set: {"login_retry"}>
# Rails:
User.stored_attributes[:settings] # => [:two_factor_auth, :login_retry]

[casperisfine]

Ah I see, my bad.

I'm ok with fixing this, but your PR is really big and a bit hard to grasp.

I think first we should fix the test suite that is failing because of the YAML changes in Rails. For a quick fix we should add all the necessary types to the allow list in the test app. It's best done as a standalone PR, once it's done it will be easier to review.

However I super swamped this week (and probably the couple next weeks as well) so I'm not sure I'll have a whole lot of time to look at this.

[casperisfine]

Marshal.load(Marshal.dump()) isn't acceptable here, and would break if the code contain procs and things like that.

We should dup the necessary things, not rely on a deep_dup hack.

[AlphonseSantoro]

I'll see if I can find another way to dup the stores, this is to prevent modifying the store in parent class

[AlphonseSantoro]

I think first we should fix the test suite that is failing because of the YAML changes in Rails.

I'll see if I can find time to fix the CI. I briefly checked it out last night and it seems there are more issues than just the YAML changes.

[casperisfine]

I fixed CI this morning.

[AlphonseSantoro]

If you define a store multiple times a warning will be thrown on "already initialized constant"

[AlphonseSantoro]

I've let the accessors be for now, but I think those should also be fixed so they are preserved.

With the fixes in this MR:

class User < ActiveRecord::Base
  typed_store(:settings) { |s| s.any :two_factor_auth }
  typed_store(:settings) { |s| s.any :login_retry }
end
# This results in:
User.store_accessors # => #<Set: {"two_factor_auth", "login_retry"}>
User.typed_stores[:settings].fields.keys # => [:two_factor_auth, :login_retry]
User.stored_attributes[:settings] # => [:two_factor_auth, :login_retry]

# When using the accessors option:
class User < ActiveRecord::Base
  typed_store(:settings, accessors: [:two_factor_auth]) { |s| s.any :two_factor_auth }
  typed_store(:settings, accessors: [:login_retry]) { |s| s.any :login_retry }
end
# This results in:
User.store_accessors # => #<Set: {"login_retry"}>
User.typed_stores[:settings].fields.keys # => [:two_factor_auth, :login_retry]
User.stored_attributes[:settings] # => [:two_factor_auth, :login_retry]

I propose that the logic for the accessors should be:

• accessors: false option should only disable accessors for the fields in current store
• You should only be allowed to define accessors that are defined in the store. (why would you define an accessor that will have no value?)
  E.g. typed_store(:settings, accessors: [:key_1, :key_2]) { |s| s.any :key_1 }. makes no sense as key_2 is not defined
• If defining more than one store on the same store attribute the accessors should be merged.

This of course can be fixed after this fix or when someone asks for it

[AlphonseSantoro]

Alternative could be to make fields an accessor, but up to you what you prefer here