is reloading AR models a code smell?

[Denis Haskin]

This is a problem I had back in the Java Hibernate days as well, and I always wonder if I'm doing something wrong.

I frequently think of ActiveRecord as being magical (perhaps that's my first mistake), and what usually surprises me is when I have to explicitly reload an object to have a modification to the object be visible.

This often happens with callbacks, and in particular with association callbacks.  If I thought it through I could probably figure it out, but...

So here's my current scenario.  I have Article which has_many ArticleStatistics.  There is an attribute Article#last_statistic_at which is the most recent datetime for any of an Article's ArticleStatistics.

I'm keeping it updated with a callback:

has_many :article_statistics, after_add: :update_article_last_statistic

  def update_article_last_statistic article_statistic

    article_statistic.update_article_last_statistic

  end

and then in ArticleStatistic:

  after_save :update_article_last_statistic

  def update_article_last_statistic

    article.update_attribute(:last_statistic_update,

      self.statistic_at) if !article.nil? and

      (article.last_statistic_update.nil? or (article.last_statistic_update < self.statistic_at))

  end

In my spec, I have something like:

    as = FactoryGirl.create(:article_statistic, statistic_at: t)

    a.article_statistics << as

    a.reload

    a.should_not be_statistics_changed_in_last_hour

If I don't have that a.reload, the article (a) is stale.

I feel like I'm doing something wrong here...

Thanks --

Denis

[Maurício Linhares]

It is a smell, specially inside callbacks, but then you would have to
decide to not use callbacks anymore and move this stuff to a higher
level API that would be external to the article.

I this specific case of article statistics, I think it wouldn't be
really complicated for you to do that.
-
Maurício Linhares
http://mauricio.github.io/ - http://twitter.com/#!/mauriciojr

> --
> You received this message because you are subscribed to the Google Groups
> "Boston Ruby Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to boston-rubygro...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[Denis Haskin]

Thanks -- I guess I'm not really following you.

Are you suggesting something like an ArticleStatisticsManager? (speaking of Java...)

[Maurício Linhares]

Definitely.

But minus the manager :)

Could also just be a static method somewhere at the ArticleStatistic
class, as long as you get rid of the callback and don't allow people
to change or view the collection directly.

-
Maurício Linhares
http://mauricio.github.io/ - http://twitter.com/#!/mauriciojr

[Abby Howell]

Why not just an instance method on article?

def last_statistic_created_at
    self. article_statistics.order(created_at DESC).pluck(created_at).take
end

It would do the active record database query when you called the method rather than when you create the statistic,  but if you are indexing the statistics table on article id and created_at,  it should be a quick search.

[Patrick Robertson]

I have a pretty well established pattern of being one of those folks that advocates for java-esque development of Rails applications because I’ve found that maintaining a 50k sloc Java app is generally a breeze compared to maintaining the same Rails application and I attribute that primarily to the fact that Java best practices center around a complete separation of the persistence and business logic layers of the application whereas in Rails we marry the two happily. What you’re asking about is one of the easiest areas to split the two layers up and profit.

At Iora, we drew a hard line in the sand and said no AR lifecycle methods nor AR::Observers be allowed* (all rules are allowed to be broken when the rule gets in the way of getting software to users). We’ve found that implementing lifecycle style events in service objects that then invoke persistence methods and are wrapped in an explicit transaction is a superior implementation pattern to delegating the whole deal to ActiveRecord. There are a number of reasons why:

• model.reload is in fact not a unit test. We are testing how business logic integrates with the persistence layer. We can instead test the business layer without recording objects to the db in a unit test and then test the layer interaction in a functional test. There is no real way to separate the persistence from the biz logic with AR lifecycle events.
• AR lifecycle events have a limited number of actual business cases. They are necessary in at most a few controllers and potentially a worker event or two. By pushing this to every save/update/create you are invoking methods considerably more on both test and production than you need.
• Explicitly named classes on controllers convey the intention of the code significantly more than a callback event embedded in a model. We highly value intention revealing code.
• Changes to business logic on lifecycle events often encounter interesting dependencies on the order in which the events where triggered when you operate in the implicit land. A developers knowledge of callback order is directly proportional to the number of bugs introduced on feature change. This is non-existent when things are ordered explicitly. Invoking callbacks on associations are especially true here as it causes very hard dependencies on DB fields in external classes making the coupling high and cohesion low.
• Removing the code from a model class reduces its responsibility and complexity. Lifecycle events are the lowest hanging fruit to extract when dealing with God objects.
• The overall difference in implementation can be measured in minutes. I removed all of our lifecycle events on a plane trip to RailsConf.

Of course there’s always the very Java-esque pattern of using stored procedures instead of code in the application layer :). I’d only use that pattern when you’re doing things like de-normalizing data for reporting or something that has nothing to do with end-user acceptance.

- Patrick

[Maurício Linhares]

You should turn this into a blog post.

-
Maurício Linhares
http://mauricio.github.io/ - http://twitter.com/#!/mauriciojr

[Joel Oliveira]

What Maurício said.

[Denis Haskin]

Yeah, what Mauricio said.  Excellent advice.

On Mon, Jan 20, 2014 at 12:25 PM, Maurício Linhares <mauricio...@gmail.com> wrote:

[Wyatt Greene]

Yes, I would love to hear more about this, Patrick.

[Dan Pickett]

Great response, Patrick! I find save invocations and callbacks get abused a lot! after_save’s in particular. It almost always warrants the extraction of some type of service object. I think your point about reducing responsibility and complexity is the best justification for avoiding such dependency on the AR lifecycle. Definitely a blerg-worthy topic, or even BostonRB presentation worthy *hint* *hint*

Just remember...http://i.imgur.com/QZySlRP.jpg

[Barun Singh]

We've done something very similar at WegoWise, with very positive effects. The reasons Patrick listed for avoiding lifecycle events are spot-on, IMO, but we took a slightly less hard-line approach, and extended the service object concept to broader domains.

The primary motivation is to write clean code that enforces separation of concerns and reveals intent. This makes debugging, optimizing, and extending your application much easier. Most usage of AR lifecycle callbacks obscure intention, and grossly violate separation of concerns. But there are cases where a callback might make very good sense, and we allow them in those cases. We've found that these types of callbacks usually involve things like setting default values for dependent fields in your database (there are cases where default values in the database itself are not sufficient). Those cases should be rare, but they do exist.

Once you start refactoring code to use service objects to get rid of observers and callbacks, you'll probably find that you can use similar refactoring ideas for other things as well. In our codebase right now, we have the following categories of objects that are not part of the default Rails approach: service objects, cache objects, presenters, context objects, and routing objects. Good refactoring (in OO languages, at least) usually involves turning complexity into distributed simplicity, so many categories of objects can be created. I've found that to do this well, you need to be very clear about how you define your categories. For us, a service object is an object that performs exactly one function, through a method named `execute!`, and is a subclass of a `BaseServicer` class, which provides a uniform API.

The service object pattern is actually much faster to code for than callbacks, generally speaking, because your unit tests are simpler, you don't have as many permutations of things to deal with. That having been said, it wasn't particularly fast for us to change existing callback code into service objects, mostly because we did the transition after the callbacks were already overly-complex, but also because changing the code is pretty easy, but rewriting specs adds a lot of time.

[Andrew DiMichele]

I know this deviates from the great advice that has been discussed so far, but FWIW there is an ActiveRecord feature that is helpful in this situation. The problem is that article.article_statistics[n].article is a different instance than article even though they reference the same record in the database. That's why you have to reload article... the database was updated via article_statistics.article, so article goes stale and needs to be reloaded to grab the latest data.

Fortunately, the ActiveRecord associations all support an :inverse_of parameter that can be used to avoid model reloads when you lose referential integrity in cases like this. This is covered in the docs (E.G.): http://apidock.com/rails/ActiveRecord/Associations/ClassMethods/belongs_to.

For example, change your has_many association in your Article class as follows:

has_many :article_statistics, inverse_of: :article, after_add: :update_article_last_statistic

And then in your ArticleStatistic class:

belongs_to :article, inverse_of: :article_statistics

Again, not sure if this is the best advice but it may solve your problem.

Andrew

This email may contain material that is confidential and/or privileged for the sole use of the intended recipient. Any review, reliance, or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. Also note that email is not an appropriate way to send protected health information to Omada Health employees. Please use your discretion when responding to this email.

[Denis Haskin]

Andrew -- I think that might be helpful.  (I'm not ready to refactor things to get rid of the callbacks, although I think I should based on the advice here).

I wondered whether that might not be what I was encountering.

Thanks,

--

[Barun Singh]

Andrew's response reminded me of something important that I forgot in my earlier reply: Even if you avoid ActiveRecord lifecycle objects, there are still numerous situations in which you may need to call `reload`. The fundamental code smell is violating separation of concerns. Calling reload from within the object class itself does so, and should be avoided. This is also true when reloading one model object from a callback in another model object. But, there are times when you will need to, and should, reload an object instance from a service object or controller.

One common situation where this arises is when you go outside of ActiveRecord to do database updates. For example, if you want to efficiently bulk insert article statistics for an @article instance (using a single insert statement instead of using AR) you may need to reload @article afterwards to refer to its statistics, since those might have been cached from before you did the insert.

On Mon, Jan 20, 2014 at 2:12 PM, Andrew DiMichele <and...@omadahealth.com> wrote:

--

[Winfield]

Extracting service objects can be a good pattern, but I'd say "it depends" on whether your AR-lifecycle logic should be extracted.  There are two scenarios when extracting service objects makes sense and one scenario where I don't think you should.

When Shouldn't You Extract Service Objects?

When you have small transformations or rules needed to keep data inside the model consistent - the lifecycle and the logic should be kept inside the model for better encapsulation.  Especially if this logic is MUST be executed to keep the data correct.

I don't think that counter caches or last updated timestamps count for this.

When Should You Extract a Service Object?

When you're implementing secondary effects between a model and the rest of the system, you should extract a service object.  This is the most common reason for extracting code - it's encapsulating the collaboration of external actors into a well named context.

The second, and most compelling reason for extracting a service object is when you have asynchronous or background processing.  Counter caches, last updated timestamps, and any other "eventually consistent" data can/should be executed asynchronously.  Extracting behavior into a service object allows easy delegation of asynchronous processing and prevents putting logic into "worker" classes or tying the logic to the background processing.  

[Patrick Robertson]

BostonRB mailing list:

Ask for blogs and you shall receive: http://adequate.io/culling-the-activerecord-lifecycle

Next time ask about something fun more fun. Examples of things that are more fun include (but are not limited to):

- puppies

- ten ways to combine sous vide and duck fat for fun and profit

- sneaking butter into vegan lunch options to annoy my coworkers who aren't down with the cow

- laser tag

- node.js (I kid, I kid)

- Patrick