rails/activeresource

Using .where on associations performs API call twice. Once without .where params, and once with .where params

tolgap opened this issue · 5 comments

Given the following construction:

# app/models/user.rb
class User < ActiveResource::Base
  has_many :investments
end

# app/models/investment.rb
class Investment < ActiveResource::Base
  schema do
    string :status
  end

  belongs_to :user
end

user = User.find(1)
user.investments.where(status: "success")

running this on my server causes three API calls:

# The User.find call:
- -> /user/1.json

# user.investments line
- -> /investments.json?user_id=1

# .where(status: "success") line
- -> /investments.json?status=success&user_id=1

I would have expected only 2 api calls. One for User.find and one for the entire user.investments.where. If you take a look at what method is called for user.investments, we notice that find_every is called on an association collection. But this collection is immediately ran against the API:

instantiate_collection((format.decode(connection.get(path, headers).body) || []), query_options, prefix_options)

There is no chance to apply the where params on the collection before performing it, as far as I understand it?

This doesn't change what you found, but there is a workaround that might let you have only 2 api calls:

user = User.find(1)
investments = Investment.where(status: "success", user_id: user.id)

This obviously isn't ideal since it'd be nice to just leverage associations.

You could simulate those associations (sort of) by doing something like:

class User < ActiveResource::Base
  def investments(clauses = {})
    Investment.where({user_id: id}.merge(clauses))
  end
end

user = User.find(1) # makes the first API call
user.investments(status: "success") # makes the second

Sorry if I'm stating the obvious - maybe it'll help others at least.

Looking at the code, the real problem is that there are two where methods.

There's a where class method provided by ActiveResource::Base. It's what's being used when you do Investment.where(status: "success").

The other where is an instance method available to instances of ActiveResource::Collection.

That method calls ActiveResource::Base.where, as seen here:

    def where(clauses = {})
      raise ArgumentError, "expected a clauses Hash, got #{clauses.inspect}" unless clauses.is_a? Hash
      new_clauses = original_params.merge(clauses)
      resource_class.where(new_clauses)
    end

When you do...

user = User.find(1)
user.investments.where(status: "success")

..the where can only work on an instance of a Collection. Said another way, it's simply too late to avoid the API call that populated that Collection instance.

To make it avoid that extra API call a few things would need to happen:

Before instantiating a new Collection (ie, the investments in user.investments, you'd need to determine if, once called, where was also going to be called. If so, you'd need then merge those where options into the API call for investments.

It's not impossible, but it's not trivial either. You'd need sometime like AREL to have it behave properly for most cases. Maybe I'm wrong?

@elbowdonkey You are correct that your workaround works. It's basically what I'm doing right now.

It would be nice to have something like ActiveRecord::Associations::CollectionProxy that would delegate performing the actual API call, until the result is required (like doing any Enumerable method).

I agree, that'd be pretty useful. I think, unfortunately, that ActiveResource just hasn't kept up the same pace of innovation over the years as ActiveRecord. I still use ActiveResource, and still think it's useful, but it could use some modernization.

I also didn't even consider CollectionProxy maybe something similar could be adapted to work for ActiveResource as well?

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.

If it is an issue and you can still reproduce this error on the master branch,
please reply with all of the information you have about it in order to keep the issue open.

If it is a pull request and you are still interested on having it merged please make sure it can be merged clearly.

Thank you for all your contributions.