Run before_save only if attribute wasn't assigned

rubyonrails-talk
1

I often notice this pattern: I want to update attribute2 when attribute1 changed, but only if attribute2 wasn’t explicitly provided.

For example, if zip_code changed, I want to look up and update the time_zone.

before_save if: -> { zip_code_changed? } do
  self.time_zone = TimeZone.find_by_zip(zip_code)
end

But if time_zone was also assigned, I don’t want to look up and change it. So I could technically do

if: -> { zip_code_changed? && !time_zone_changed? }

but that’s not sufficient, because the provided time_zone might be the same as before, triggering the lookup anyway.

Ideally, there’d be an attribute method in DirtyTracking like [attribute]_assigned?, which returns true even if assigned value is the same.

Is there a way to do this cleanly inside the model, that doesn’t involve overwriting an attribute setter, or doing this in the caller instead? Could this be a welcome new feature in ActiveModel?

2

Is there a way to do this cleanly inside the model, that doesn’t involve overwriting an attribute setter

Why not use attribute setters to track the assignments?

def zip_code= new_code
  @zip_code_assigned = true
  super
end

def time_zone= new_zone
  @time_zone_assigned = true
  super
end

before_save do
  self[:time_zone] = TimeZone.find_by_zip zip_code if @zip_code_assigned && !@time_zone_assigned
  remove_instance_variable :@zip_code_assigned
  remove_instance_variable :@time_zone_assigned
end

Haven’t tested this, but seems like this should work.

3

Yeah this would definitely work, but it stresses me out a little.

Maybe it’s because every time I see this kind of code, I worry that assignment is not what it seems anymore. It has some extra logic, some potentially fragile order of operations. Every time I see the @*_assigned ivar, I also have to go double-check where it’s used, to make sure assigning is safe.

And if instead there was a built-in [attribute]_assigned?, then everyone would be familiar with this pattern, and it wouldn’t be something custom/weird you have to pay extra attention to.

4

Hm, maybe a different ivar name would make me less stressed, like:

def time_zone= new_zone
  @skip_timezone_autodetect = true
  super
end
5

I worry that assignment is not what it seems anymore. It has some extra logic, some potentially fragile order of operations.

I get that and you do want to be careful to remove any order of operation assumptions (putting the evaluation in before_save vs the accessor does that). I also feel the same way about AR hooks sometimes. I go to save a record and next thing I know e-mails are being fired off or whatever.

My general rule is these sort of adjustments to the standard persistence in models should be limited to internal state of the object. I think your case would qualify as you are updating the time zone to match the postal code (unless time zone was explicitly updated). If the accessor (or AR callback) where updating other objects or carrying out other workflow processes then I would lean towards that process more explicit to the caller. But for internal state management of an object I think it’s fine to hide functionality behind accessors and AR callbacks.

6

I 100% agree with all of that.

A couple years ago, I came up with a somewhat radical “rule”, but haven’t figured out how to phrase well yet, something like “always split abstractions on IO”: always split network calls, database calls, filesystem access, ENV access, and similar actions into standalone calls directly from your entry points (i.e. “shell” code). (This of course covers sending emails, etc). Do not tangle them into core logic. Examples of entry points are: controller actions, rake tasks, background job actions.

Been trying to apply it ever since, and it seems to hold up pretty well. It became the subject of this post contrasting this against DHH’s favorite approach, which seems quite the opposite: embrace vast ripple effects, and suppress them as needed.

This example is indeed borderline “probably should be in the caller”, because the timezone lookup uses an obscure dataset lookup, not unlike filesystem or db access.

7

@hakunin You could also pretty easily package this up yourself as an extension on ApplicationRecord. I get it’s not as clean and “conforming” as having Rails core team adding it to AR, but if it works well for you you could then submit it as a PR.

class ApplicationRecord
  def self.track_assigned_attributes(*args)
    args.each do |attribute_name|
      define_method("#{attribute_name}=") do
        instance_varbiable_set("@_#{__METHOD__.split('=').first}_assigned", true)
        super
      end

      define_method("#{attribute_name}_assigned?") do
        instance_variable_get("@_#{__METHOD__.split('_assigned?').first}_assigned")
      end
    end
  end
end

class MyModel < ApplicationRecord
  track_assigned_attributes :zip_code, :timezone
end
8

This is not the answer you want but this feels like business logic that might be better lived in a controller or a helper method. Adding this kind of logic to the model creates hard to debug edge cases.

9

To be fair, this could probably be said about all of ActiveModel::Dirty. Elixir’s Ecto.Changeset is an interesting alternative.

Nice example! :+1: I add features like this sometimes.