Initialise and assign attributes separately

[brendon]

What would you think of separating these two steps?

https://github.com/hashrocket/decent_exposure/blob/master/lib/decent_exposure/behavior.rb#L64

So instead:

def build(params, scope)
  instance = scope.new
  instance.attribiutes = params

  instance
end

The reason is subtle, but the build association is not accessible until after the object has been initialised, so if something wants access to the association in any of the attribute assignment methods it won't have it yet.

In my specific case, I have a Paperclip processor that needs something from the instance's associated parent object, and Paperclip processors do all their work when the parameter is applied rather than waiting until the instance is saved, and thus because the association isn't available yet, it all blows up.

The above code is basically functionally identical to the existing code except that it allows the instance to initialise (and assign its parent association), then assigns the attributes after that.

[mattpolito]

@brendon I see what you're going for but am unable to wrap my head around why this would be an issue. Could you provide an example of what case would cause an issue?

[brendon]

Hehe, yea it's mildly obscure, but I'll give it a go:

class Parent < ActiveRecord::Base
  has_many :children

  def something_special
    'special'
  end
end

class Child < ActiveRecord::Base
  belongs_to :parent

  delegate :something_special, to: :parent

  def special_attribute=(value)
    do_something_with(something_special)
  end
end

Using decent exposure, things will blow up as soon as the instance is built with parameters including special_attribute because it will try to access something_special but the relationship method on the new instance object hasn't been established yet (it seems to happen after the attributes are assigned). It's a kind of race condition.

For me, this happens with Paperclip where it does a bunch of processing of the files when the attribute is assigned.

Hope that helps explain things. It's definitely an edge case. My workaround currently is not to allow that particular attribute in the child_params, then assign it manually after accessing the object for the first time.

[mattpolito]

@brendon Interesting. Thanks for explaining this.

It seems like you already have a decent test case laid out in that message. If you want to put that together with the change, I can take a look at that PR.

Thanks again for clarifying

[brendon]

Thanks @mattpolito. I've just started having a go, but in order to do this with AR I'll need to add a database into the testing mix. It's not a problem, but just wanted to check if you're happy with that. Right now it looks like we're just simulating AR sort of... :)

[brendon]

I've managed to work around the AR thing for now. However, attributes= is a rails specific method, and I've found some of the tests are failing when I adjust the build logic to:

def build(params, scope)
  instance = scope.new
  instance.attribiutes = params

  instance
end

This is one failure:

12) DecentExposure::Controller override scope allows overriding model scope using symbol
      Failure/Error: instance.attributes = params

      NoMethodError:
        undefined method `attributes=' for 42:Fixnum
      # ./lib/decent_exposure/behavior.rb:65:in `build'
      # ./lib/decent_exposure/flow.rb:75:in `call'
      # ./lib/decent_exposure/flow.rb:75:in `handle_default_flow_method'
      # ./lib/decent_exposure/flow.rb:57:in `block in handle_flow_method'
      # ./lib/decent_exposure/flow.rb:85:in `fetch_ivar'
      # ./lib/decent_exposure/flow.rb:53:in `handle_flow_method'
      # ./lib/decent_exposure/flow.rb:25:in `method_missing'
      # ./lib/decent_exposure/behavior.rb:9:in `fetch'
      # ./lib/decent_exposure/flow.rb:75:in `call'
      # ./lib/decent_exposure/flow.rb:75:in `handle_default_flow_method'
      # ./lib/decent_exposure/flow.rb:57:in `block in handle_flow_method'
      # ./lib/decent_exposure/flow.rb:85:in `fetch_ivar'
      # ./lib/decent_exposure/flow.rb:53:in `handle_flow_method'
      # ./lib/decent_exposure/flow.rb:25:in `method_missing'
      # ./lib/decent_exposure/exposure.rb:181:in `block in attribute'
      # ./lib/decent_exposure/context.rb:58:in `instance_exec'
      # ./lib/decent_exposure/context.rb:58:in `fetch_value'
      # ./lib/decent_exposure/context.rb:23:in `get'
      # ./lib/decent_exposure/attribute.rb:46:in `block (2 levels) in expose!'
      # ./spec/decent_exposure/controller_spec.rb:318:in `block (3 levels) in <top (required)>'

I'm hoping it's the tests that need to be changed to take into account the seperate attributes= step. What do you think?

[mattpolito]

@brendon Do you have your code on a branch so I can take a look?

[brendon]

Sorry, just needed a fresh look to figure out what was going wrong :) I've made the PR with a passing suite. Let me know what you think :)

[mattpolito]

@brendon I've been thinking about this feature request a lot lately. We actually ran into this issue for the first time in many years using DE. What we found is that results may vary depending on how you're processing your incoming association. For example, building an object through an association, rightfully, has the association attribute processed last (Security reasons come to mind but couldn't find the exact commit in AR to prove that).

After experimenting with this, we've decided that covering for this nuance in AR is not necessarily a concern of DE, so because of that, I'm going to close this issue/PR. I know it's been a long time, but I do very much appreciate the effort and time you put into making the library better.

Thanks

[brendon]

Hi @mattpolito, thanks for taking the time to consider the pull request. There are certainly ways to work around this when necessary. One could argue that depending on an association existing inside an attribute setter is bad design anyway. Do you have any details on how you worked around this in your example? I'm happy with your decision :)

[mattpolito]

@brendon, due to the exposure being lazy, all our scenario needed was to have the exposure loaded before applying. So explicitly setting the attribute that was dependent on association info did the trick.

expose(:thing, scope: :parent_thing)

def create
  thing.association_dependent_attribute = thing_params[:association_dependent_attribute]
  if thing.save
  ...
  end
end

Or, you could reapply attributes wholesale specifically in the case you need, which is basically what you proposed adding to DE:

expose(:thing, scope: :parent_thing)

def create
  thing.attributes = thing_params
  if thing.save
  ...
  end
end

Since most attributes would have already been applied with the exposure build, not much would change with the second assignment of attributes, so there shouldn't be any additional overhead cost to applying a second time in cases where it is necessary.

I hope that clears it up for you. Your situation may not be exactly ours (if it's even your situation anymore... I know it's been a long time ☹️), but thanks for understanding.

Again, thank you for the contribution.

[brendon]

That's an excellent explanation and I'm sure it'll help others in the future too :) All the best :)