Unable to use id in ActiveModel validations since reform-rails 0.2.4

[valscion]

Hi!

We are struggling to update reform-rails from 0.2.3 to 0.2.5 (we also had this same struggle with reform-rails version 0.2.4).

The problem seems to stem from us having this kind of validation:

property :id, virtual: true
property :_destroy, virtual: true
property :photo

validates :_destroy, presence: true, if: -> { id }
validates :id, presence: true, if: -> { _destroy }

And we have this kind of validation call:

form.validate(photo: a_new_image)

With reform-rails 0.2.4 and 0.2.5, we get a validation error saying that _destroy can't be blank while with 0.2.3 we don't get that.

The change is that in reform-rails 0.2.3, the value of id in the if: -> { id } lambda was read from the parameters passed to form.validate while in 0.2.5, the value of id is read from the form.model. I think the problem is somehow caused by 294de5f and #100 but I don't really understand why.

Is it possible that with the old Uber::Delegates, the id delegation was somehow different than how it is now with Rails provided delegate method?

---

Here's the full source code for our form

# frozen_string_literal: true
module Profiles::Common
  class ProfileEditImagesForm < Reform::Form
    model :profile

    ImagePopulator =
      lambda do |fragment:, collection:, **|
        # `fragment` represents the incoming params piece regarding one image.
        # `collection` represents the methods usable to manage the `images` collection.
        # http://trailblazer.to/gems/reform/populator.html

        if (fragment[:_destroy] && !fragment[:id]) ||
             (fragment[:id] && !fragment[:_destroy])
          # Validations will prevent this kind of populator from going through. We just
          # need this populator to return some valid value so that Reform won't choke yet,
          # but one that allows Reform to provide validation errors anyway. Whew.
          return collection.append(::Profiles::Common::ProfileImage.new)
        end

        if fragment[:_destroy]
          item =
            collection.find { |itm| itm.model.id.to_s == fragment[:id].to_s }
          collection.delete(item) if item
          skip!
        else
          collection.append(::Profiles::Common::ProfileImage.new)
        end
      end

    # `save: false` stops existing images being touched when collection is changed
    collection :images, populator: ImagePopulator, save: false do
      property :id, virtual: true
      property :_destroy, virtual: true
      property :photo

      validates :id, presence: true, if: -> { _destroy }
      validates :_destroy, presence: true, if: -> { id }
    end
  end
end

And here's how we have a unit test for it:

it 'does not modify existing images' do
  existing_image = create(:image, :with_real_file)
  profile = create(Profiles::SupplierProfile.factory_name, images: [existing_image])
  form = Profiles::Common::ProfileEditImagesForm.new(profile)

  new_image = fixture_file_upload('lol.png', 'image/png')
  result = form.validate(images: [{ photo: new_image }])
  puts "We have errors but shouldn't: #{form.errors.messages}"
  assert(result)
  expect { form.save }.not_to(change { existing_image.reload.updated_at })
end

Back in reform-rails 0.2.3, this code didn't have errors but now with 0.2.5, we get an error:

We have errors but shouldn't: {:"images._destroy"=>["can't be blank"]}

[apotonick]

Hi Vesa, thanks for the detailed report. I feel like we had a similar discussion about delegation here #101 could you verify?

[yogeshjain999]

@valscion IMO, the validation seems correct here based on the test case because it says that _destroy should be present when id is present for the existing record (not sure why you didn't get error in older version, could be because id wasn't getting delegated or something 😄).

[valscion]

Hi Vesa, thanks for the detailed report. I feel like we had a similar discussion about delegation here #101 could you verify?

Thanks, I'll take a look!

it says that _destroy should be present when id is present for the existing record (not sure why you didn't get error in older version, could be because id wasn't getting delegated or something 😄).

I am trying to validate the incoming form values, not what's in the existing record. It used to work in the past but now id seems to be read from the model even though the form has been configured with

property :id, virtual: true

[apotonick]

That is indeed a good point, @valscion. I wonder if that maybe is a bug in Reform itself?

[valscion]

Yeah I've added a debug entry point inside the :if lambda of

      validates :_destroy, presence: true, if: -> { id }

and in reform-rails 0.2.3, the id was nil when incoming form parameters didn't specify any id value but now with reform-rails 0.2.5 the value of id inside that lambda is the id of the model.

I did check out #101 but that seems slightly different to what we have — and their patch that they added (changing the populator) also doesn't apply for us because we are already doing the "find the item" part of the populator the same way as their patch did:

collection.find { |itm| itm.model.id.to_s == fragment[:id].to_s }

[valscion]

Here are screenshots showing this single test run with some puts printing added, both with reform-rails 0.2.3 and 0.2.5:

0.2.3

0.2.5

---

I haven't dug deeper why the lambda is evaluated twice and why on the second time with 0.2.5, the value is nil as expected but not on the first try.

[valscion]

I haven't dug deeper why the lambda is evaluated twice and why on the second time with 0.2.5, the value is nil as expected but not on the first try.

I dug a bit deeper and noticed that the lambda invokation happens when form.validate is called and on both times, the caller is the same. Here's the relevant part of caller:

/Users/vesa/code/v/venuu/app/features/profiles/common/forms/profiles/common/profile_edit_images_form.rb:41:in `block (2 levels) in <class:ProfileEditImagesForm>'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:428:in `instance_exec'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:428:in `block in make_lambda'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:180:in `block (2 levels) in halting_and_conditional'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:180:in `all?'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:180:in `block in halting_and_conditional'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:513:in `block in invoke_before'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:513:in `each'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:513:in `invoke_before'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:134:in `run_callbacks'
--snip--/gems/activesupport-6.0.6.1/lib/active_support/callbacks.rb:825:in `_run_validate_callbacks'
--snip--/gems/activemodel-6.0.6.1/lib/active_model/validations.rb:406:in `run_validations!'
--snip--/gems/activemodel-6.0.6.1/lib/active_model/validations.rb:337:in `valid?'
--snip--/gems/reform-rails-0.2.5/lib/reform/form/active_model/validations.rb:93:in `call'
--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:46:in `block in call'
--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:44:in `collect'
--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:44:in `call'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:31:in `validate!'
--snip--/gems/reform-rails-0.2.5/lib/reform/form/active_model/validations.rb:70:in `validate!'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:56:in `block (2 levels) in validate_nested!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `block in collection!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each_with_index'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `collect'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `collection!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:16:in `call'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:53:in `block in validate_nested!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/definitions.rb:27:in `block in each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/definitions.rb:27:in `each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/definitions.rb:27:in `each'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:51:in `validate_nested!'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:37:in `validate!'
--snip--/gems/reform-rails-0.2.5/lib/reform/form/active_model/validations.rb:70:in `validate!'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:11:in `validate'
--snip--/gems/reform-2.6.2/lib/reform/form/validate.rb:30:in `validate'
/Users/vesa/code/v/venuu/app/features/profiles/common/forms/profiles/common/profile_edit_images_form.spec.rb:23:in `block (4 levels) in <main>'

[apotonick]

Why can't every OSS bug hunter be like you? 💚

I have a suspicion, could you figure out which disposable version with 0.2.3 is being used? 🔍

[valscion]

Based on the output of caller, I'd expect the version of disposable to be 0.6.3:

--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `block in collection!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each_with_index'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `each'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `collect'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:24:in `collection!'
--snip--/gems/disposable-0.6.3/lib/disposable/twin/property_processor.rb:16:in `call'

That's also what we have in Gemfile.lock. And the same what puts Disposable::VERSION outputs.

[apotonick]

So we're using the same version of disposable in all reform-rails versions?

[valscion]

I don't really understand your question? We do have only one version of disposable in our application, yes.

[apotonick]

Of course you do 😆 my questions was whether across the reform-rails updates the disposable version remains 0.6.3?!

[valscion]

Oh yes, both reform-rails 0.2.3 and 0.2.5 are using the same version of disposable, 0.6.3. The version of reform-rails is the only thing that is changing here.

[apotonick]

Ok that's good, I was dreading a bug inside disposable, which would be hard to fix right now. Hm well then it really must be about the delegation, I guess? I might find some time during the week, or maybe this evening.

[valscion]

Cool! Let me know if I can help debug this any further ☺️

[apotonick]

Well you could look into the form object and figure out if, after instantiation, it read the virtual attribute or not?

[valscion]

Looks like 0.2.5 does read the virtual attribute but 0.2.3 does not:

[apotonick]

What reform version is being used here?

Thanks for this great reporting style, this is so much more helpful then "it simply doesn't work"! Appreciate it! ☕

[valscion]

What reform version is being used here?

That's visible in the output of caller as well:

--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:46:in `block in call'
--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:44:in `collect'
--snip--/gems/reform-2.6.2/lib/reform/validation/groups.rb:44:in `call'
--snip--/gems/reform-2.6.2/lib/reform/contract/validate.rb:31:in `validate!'

That is, we are using reform version 2.6.2

[apotonick]

Please try master branch, @valscion. As you guesses, the way Rails delegates works broke things, as it doesn't allow overriding the delegated methods.

[valscion]

Looks like master did indeed fix the form this bug report was about! I'll get back to you toomorrow to see if all tests in our CI also complete later as I'll be clocking out now ☺️

EDIT: Looks like all issues we had are fixed now indeed! 👏

[valscion]

Looks like 0ba0ac0 did indeed fix our issues and all would seem to work with that commit in a version of reform-rails ☺️

[apotonick]

Thanks again for your debugging and reporting, I wish we had more people like you! 💪