Concerns about Valkyrie::ChangeSet (discussion)

[dchandekstark]

I would like to raise a couple of concerns about Valkyrie::ChangeSet.

1. The libraries that it is based on -- [reform](https://github.com/trailblazer/reform and disposable -- ceased development a few years ago. While this does not appear to be an immediate issue by itself, I think it's a potentially major vulnerability, and I would like to see an alternative implementation for that reason. Valkyrie already requires activemodel, do ActiveModel::Dirty might be a good option, at least for the change tracking part, which leads me to ...
2. There seems to be issues with change tracking of properties for embedded Valkyrie::Resource objects. I am finding that change_set.changed? returns false and change_set.changed returns a hash with attribute keys and false values -- however, save works to persist the changes. Change tracking is important to our project, and it seems that reform/disposable are not reliable for the kinds of data that we are persisting. It is possible, I suppose, that user error is an issue, but that takes me back to point 1. The functionality involved here is in a secondary dependency and browsing the code of disposable gives the impression of a somewhat incomplete/abandoned effort (lots of TODO and FIXME at https://github.com/trailblazer/trailblazer-disposable/blob/master/lib/trailblazer/disposable/twin.rb, last updated 7y ago).

I am interested in whether others share these concerns or have different perspectives on the issues.

Thanks!

[tpendragon]

@dchandekstark Change tracking is definitely something that takes some finesse, and nested objects in change sets has taken some significant work from us in the past for our digital repo. I think we got it eventually, but I'd have to go back and look into it.

I'm open to a new backend for ChangeSets - we've already changed it once (from Virtus to Reform.) I think it'd take some evaluation and pretty deep work to make sure it works, and I'd like to not require a huge migration of ChangeSets. Is this something you're interested in trying?

[dchandekstark]

@tpendragon I am interested ... Given that neither reform nor disposable are actively developed, maybe it's worth forking as a starting point. I do think AM::Dirty might be a good choice, given that it's Rails, and not likely to be abandoned. I'm not sure if there's a dry-rb option/approach as an alternative, but I note that V::Resource is built on dry-struct and dry-types.

[tpendragon]

@dchandekstark I think dry-struct was used for it once, but I'm not wildly up to date. I see https://itnext.io/using-dry-rb-for-form-objects-in-ruby-on-rails-f965e0c0e86a.

I think the biggest thing is just gonna be sync and preventing AR validations from breaking - there was a brief time I considered using dry-validation because the reform folks seemed to really want that.

I will mention that things like reform-rails are still updating: https://github.com/trailblazer/reform-rails, makes me wonder.

[dchandekstark]

If we can manage with either Rails or dry-rb, we could drop one "framework" in the process, which I think is an additional benefit.

[dchandekstark]

I think I will experiment in my own environment first. Given that V::ChangeSet is more or less optional, maybe a "plugin" or second option is a better option, at least initially.

[dchandekstark]

I decided to prototype a change set using ActiveModel::Dirty, and initial ad hoc testing is promising:

  # @abstract                                                                                                                                                     
  class AbstractChangeSet
    include ActiveModel::Dirty

    # Factory method                                                                                                                                              
    # @param model [Class]                                                                                                                                        
    # @return [Class]                                                                                                                                             
    def self.change_set_for(model, *attrs)
      Class.new(self) do
        define_attribute_methods *attrs

        delegate *attrs, to: :resource

        attrs.each do |attr|
          define_method(:"#{attr}=") do |value|
            send(:"#{attr}_will_change!") unless value == resource[attr]
            resource.set_value(attr, value)
          end
        end
      end
    end

    attr_reader :resource

    def initialize(resource)
      @resource = resource
    end

    def save
      @resource = Ddr.persister.save(resource:)
      changes_applied
      resource
    end

    def reload!
      @resource = Ddr.query_service.find_by(id: resource.id) unless resource.new_record
      clear_changes_information
      resource
    end

    def rollback!
      restore_attributes
    end
  end