Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rabl doing duplicated database calls? #142

Closed
NielsKSchjoedt opened this Issue Nov 30, 2011 · 48 comments

Comments

Projects
None yet
7 participants

I want to do something similar to this https://github.com/nesquena/rabl/wiki/Tips-and-tricks but my problem is, that it seems that rabl makes duplicated calls to the database. Since some of the request can be quite heavy (due to some geo-stuff), it's a deal breaker for me.

I have simplified the code for this example:

I'm constructing an API that should return a set of cars. I want the format to look like the following:

{

"hits_count": 2,
"cars": [
    {
        "car": {
            "id": 295655,
            "brand": "Citroën",
            "model_name": "Saxo",
            "fuel": "Diesel",
            "km": 245000,
            "year": 2002
        }
    },
    {
        "car": {
            "id": 295656,
            "brand": "Citroën",
            "model_name": "Saxo",
            "fuel": "Diesel",
            "km": 234000,
            "year": 2001
        }
    }
 ]

}

Right now I'm doing it in my /index by saying:
object false
node(:hits_count) { @cars_in_search }
child @cars do
extends "api/v1/cars/car"
end

in the /car it says:
attributes :id, :brand, :model_name, :fuel, :km, :year

the @cars is a query selecting the cars, and the @cars_in_search is just a simple count

This gives me the correct format as I want, but it makes duplicate calls to the database as you can see:
(238.7ms) SELECT COUNT() FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid'
(1.2ms) SELECT COUNT(count_column) FROM (SELECT 1 AS count_column FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid' LIMIT 20 OFFSET 0) subquery_for_count
Car Load (281.4ms) SELECT "cars".
FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid' ORDER BY cars.au_rating DESC, cars.price_difference DESC LIMIT 1 OFFSET 0
Car Load (291.7ms) SELECT "cars".* FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid' ORDER BY cars.au_rating DESC, cars.price_difference DESC LIMIT 20 OFFSET 0

If instead I do the following in my /index view:
collection @cars
extends "api/v1/cars/car"

Then it's not doing duplicate requests ?????? strange:
(228.6ms) SELECT COUNT() FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid'
Car Load (286.9ms) SELECT "cars".
FROM "cars" WHERE "cars"."sales_state" = 'onsale' AND "cars"."estimate_state" = 'valid' ORDER BY cars.au_rating DESC, cars.price_difference DESC LIMIT 20 OFFSET 0

But then the problem of cause is, that I'm not getting the format that I want (as described before)

Hope you can give me a clue as to what to do.

I'm running rails 3.1 and latest rabl. It's the same in both development and production.

Owner

nesquena commented Nov 30, 2011

Try this:

object false
node(:hits_count) { @cars_in_search }
child @cars.to_a do
extends "api/v1/cars/car"
end

alternately:

cars = @cars.to_a
object false
node(:hits_count) { @cars_in_search }
child cars do
extends "api/v1/cars/car"
end

The issue is how relation scopes work. Does this work?

You are the man!!! This did the trick - thanks for a fantastic gem :-D

Owner

nesquena commented Dec 1, 2011

Glad this worked, great to hear it.

@nesquena nesquena closed this Dec 1, 2011

yujingz commented Feb 27, 2013

@nesquena can any one explains this a little bit more about this issue. I fixed same problem but don't quite understand. What's the differences between :property and @object.property.to_a. Why would the :property produces another limit = 1 query. Thanks!

Solution with to_a doesnt work for me. I put .all at the end of query building chain.

Contributor

mrbrdo commented Jun 17, 2013

Huh this is pretty bad, I don't understand how it's being treated so lightly... Basically RABL is making an extra SQL for every collection view? That's very sucky, and the solution proposed (to_a/all) even more. Isn't there something that can be done? I'm getting 2 queries, first LIMIT 1 then the normal one.

Owner

nesquena commented Jun 18, 2013

Which ORM are you guys using? ActiveRecord it sounds like?

Owner

nesquena commented Jun 18, 2013

Also can you guys post specific examples? I use RABL in dozens of projects and I don't run into this problem, if I did I would have probably fixed it by now. Specific examples of the issue and patches welcome.

Contributor

mrbrdo commented Jun 18, 2013

Yes ActiveRecord. Example:

# controller
def index
  @stuff = Stuff.where("1 = 1") # to avoid any confusion with Rails 4 #all
end

# index.json.rabl
collection @stuff
extends 'whatever'

Does a LIMIT 1 query first. Fix:

# controller
def index
  @stuff = Stuff.where("1 = 1").to_a
end

I'm using Rails 4, btw.

Owner

nesquena commented Jun 18, 2013

I see. Try this:

collection @stuff => :items
extends 'whatever'

without the "to_a" and see if it goes away. The issue I think is you need to provide the name for the key otherwise it tries to get the name by inspecting the type of the item. The way scopes work is that when the name is determined it kicks another query.

Contributor

mrbrdo commented Jun 18, 2013

nope, still does the limit query even with that (also I am not using json roots anyway)

Contributor

mrbrdo commented Jun 18, 2013

@nesquena interesting fact: even if I remove everything from the rabl file, it will still do both the queries (LIMIT 1, and then for all), but the result I get is then [ {}, {}, ... ] the count matches the number of results from the query
my (rack-mini-)profiler shows the query is coming from the rabl file (line 1)

Owner

nesquena commented Jun 18, 2013

Ok thanks for the follow up, I'll do some digging into this as soon as I can. I'm sure it's going to be an easy fix, the key is tracking down the culprit to that extra call (since it doesn't appear to be naming the key for this one)

Owner

nesquena commented Jun 18, 2013

I am curious if you go to the rails console and do:

> @stuff = Stuff.where("1 = 1")
> @stuff.respond_to?(:first)

I assume no query is kicked right?

Contributor

mrbrdo commented Jun 18, 2013

One more thing, if I rename the ivar from @name_of_controller to @name_of_controller2 then the LIMIT 1 query does not happen. It seems it only happens if the name of variable matches the "convention". for example in my case keywords_controller and @Keywords

Contributor

mrbrdo commented Jun 18, 2013

[13] pry(SearchKeyword):1> SearchKeyword.where("1 = 1").respond_to?(:first)
=> true
[14] pry(SearchKeyword):1> SearchKeyword.where("1 = 0")
  SearchKeyword Load (0.6ms)  SELECT "search_keywords".* FROM "search_keywords" WHERE (1 = 0)
=> []
Owner

nesquena commented Jun 18, 2013

That's really interesting, so you can confirm that if you name the variable @name_of_controller, it kicks LIMIT 1 but if you name it @name_of_controller_other it doesn't? and in both cases you get the same result?

Contributor

mrbrdo commented Jun 18, 2013

Yes, I can confirm. Also keep in mind that rack mini profiler shows the query is coming from the rabl file, so it must happen somewhere along the way between the controller action and the template.

Owner

nesquena commented Jun 18, 2013

Hmm, thats weird but at least it gives me a place to start looking to find the issue. I'll have to generate a Rails 4 project and play around with it soon.

Contributor

mrbrdo commented Jun 18, 2013

Thanks. I haven't tried with Rails 3 by the way, not sure if this is specific to Rails 4. I am using RC1.

Owner

nesquena commented Jun 18, 2013

OK I will try with both just to be sure. Thanks for the help poking into this issue.

Contributor

mrbrdo commented Jun 18, 2013

No worries. Thank you too.

@mrbrdo mrbrdo added a commit to mrbrdo/rabl that referenced this issue Jun 19, 2013

@mrbrdo mrbrdo Do not use a collection as the default object. Fixes #142 c7be10c

@nesquena nesquena added a commit that referenced this issue Jun 19, 2013

@nesquena nesquena Merge pull request #467 from mrbrdo/default_object_fix
Do not use a collection as the default object. Fixes #142
e61ae73

calvinl commented Jul 11, 2013

By the way this was happening on Rails 3 as well -- just fixed with version 0.8.6. Thanks guys! It was driving nuts for a bit.

Owner

nesquena commented Jul 11, 2013

Glad it's been resolved in the latest version. Thanks for the info.

HassanJ commented Jul 29, 2013

Updated the gem today to version 0.8.6, the problem still exists for me. On ruby 1.9.3p448, rails 3.2.13 and rabl 0.8.6.

Here is how my files are structured:

index.json.rabl

object false
child @venues do
  extends "api/v1/venues/show"
end

show.json.rabl

object @venue
attributes :id, :name

If I use @venues.to_a my log reads like this

Venue Load (0.3ms) SELECT venues.* FROM venues WHERE (name like '%%') LIMIT 20 OFFSET 0
(0.8ms) SELECT COUNT(*) FROM venues WHERE (name like '%%')

and if I dont

(0.6ms) SELECT COUNT() FROM venues WHERE (name like '%%')
Venue Load (0.2ms) SELECT venues.
FROM venues WHERE (name like '%%') LIMIT 1 OFFSET 0
CACHE (0.0ms) SELECT venues.* FROM venues WHERE (name like '%%') LIMIT 1 OFFSET 0
CACHE (0.0ms) SELECT COUNT() FROM venues WHERE (name like '%%')
CACHE (0.0ms) SELECT venues.
FROM venues WHERE (name like '%%') LIMIT 1 OFFSET 0
CACHE (0.0ms) SELECT venues.* FROM venues WHERE (name like '%%') LIMIT 1 OFFSET 0
Venue Load (0.3ms) SELECT venues.* FROM venues WHERE (name like '%%') LIMIT 20 OFFSET 0
CACHE (0.0ms) SELECT COUNT(*) FROM venues WHERE (name like '%%')

@calvinl and @nesquena any ideas?

Owner

nesquena commented Jul 29, 2013

Thanks for report, that duplicated calls due to scopes is pretty brutal. Hope to be able to track that down, if anyone has this problem please respond to the thread. I will see if I can reproduce it. The key part is narrowing down what's causing the duplicated calls if you've already upgraded to 0.8.6, it must be another case.

@nesquena nesquena reopened this Jul 29, 2013

HassanJ commented Jul 29, 2013

I might have a clue about what is happening in my project. I am using will_paginate for pagination and whenever you call 'first' on an object returned by will_paginate, i-e

venues = Venue.page(1)
venues.first

causes
Venue Load (0.4ms) SELECT venues.* FROM venues LIMIT 1 OFFSET 0.

I was going through the source code of rabl and it struck me that it might be will_paginate's issue. Any idea why does will_paginate's return object behave this way?

Contributor

mrbrdo commented Jul 29, 2013

No it hasn't anything to do with will_paginate. It's normal that #first does a query similar to what you show.
Probably rabl calls #first on the collection somewhere before it was coerced to array.

Owner

nesquena commented Jul 29, 2013

It does call first in some cases, seems like I may need to go back to just calling to_a on the collection as it comes into rabl or find another way to avoid triggering that scoped query call.

Owner

nesquena commented Jul 29, 2013

Question, @HassanJ if you change it to:

object false
child @venues => :venues do
  extends "api/v1/venues/show"
end

do you still see the same behavior?

Contributor

mrbrdo commented Jul 29, 2013

@nesquena it's a long shot but is it possible that is_object? malfunctions on will_paginate collection and returns true? Since for me the issue is fixed with 0.8.6, it must be either a dependency or a different code path.

HassanJ commented Jul 29, 2013

@nesquena the aliasing fixes the issue for me.
@mrbrdo does this have anything to do with it? mislav/will_paginate@6d35492

Contributor

mrbrdo commented Jul 29, 2013

@HassanJ not sure, if you do in rails console:

coll = whatever.limit(x)
coll.first

Since it's console the first line should make a query for all of the results (with limit), if it makes another sql query for the second line then this could be the problem.
It's actually possible that this is the case I think. The solution would then be like @nesquena said to coerce the collection into an array first, in rabl.

HassanJ commented Jul 29, 2013

@mrbrdo that was what I tried to mention :)
If I use objects = SomeModel.page(page_number)
calling objects.first results in an extra query

Contributor

mrbrdo commented Jul 29, 2013

Ah I did not know you meant in console, since in code that would only do the limit 1 query (since the scopes are lazy).

Contributor

mrbrdo commented Jul 29, 2013

@nesquena keep in mind this case when fixing:

# controller
@posts = Post.limit(10)
# view
collection @posts.some_extra_scope

Iirc @posts will be automatically guessed as collection. Should be careful in this case not to do the query (i.e. to_a) if the view further modifies the scope before using it.
I remember when doing my pull request I wanted to coerce to array before calling #first also, but then opted not to do it because of (I think) this potential problem.

Owner

nesquena commented Jul 29, 2013

So the source of this duplicated calls problem is here: https://github.com/nesquena/rabl/blob/master/lib/rabl/helpers.rb#L30 where RABL tries to determine the "type" of the collection. The only way to do that is to look at an item in the array which triggers the query. If you supply an alias, then the type is known and the problem doesn't manifest.

Contributor

mrbrdo commented Jul 29, 2013

I think it is possible to determine the model class directly from ActiveRecord::Relation itself. If you don't mind solving this AR-specific, then you can probably do that. Should be one of #klass or #model or #table depending on what you need.

Owner

nesquena commented Jul 29, 2013

Yeah perhaps that could work, could add a special case for an AR scope. I typically use rabl in the context of sinatra / padrino and sequel but I suppose it wouldnt necessarily hurt to have an extra if for the scope case. In general I do like to keep RABL ORM agnostic though this may warrant an exception.

Owner

nesquena commented Jul 29, 2013

@HassanJ would you be able to try git rabl and verify that

object false
child @venues do
  extends "api/v1/venues/show"
end

gives expected output now with no extra queries?

Contributor

mrbrdo commented Jul 29, 2013

off topic but yeah, sequel rocks :)

HassanJ commented Jul 30, 2013

@nesquena it reduced the extra queries by 2, but the rest are still there.

Owner

nesquena commented Jul 30, 2013

Alright thanks ill see if I can hunt down the others

Owner

nesquena commented Sep 3, 2013

@HassanJ if you are still around and you add:

object false
child @venues, :root => "venues" do
  extends "api/v1/venues/show"
end

in rabl 0.9.0.pre can you tell me if the queries are gone?

HassanJ commented Sep 3, 2013

@nesquena I did and it produces extra queries.
This works for me without producing any extra queries.

object false
child @venues => :venues do
  extends "api/v1/venues/show"
end
Owner

nesquena commented Sep 3, 2013

Ok thanks for testing and responding so quickly, will try to ensure that the , :root => "venues" option works the same as @venues => :venues

HassanJ commented Sep 3, 2013

No problem, thanks for the Gem ;)

Owner

nesquena commented Dec 6, 2013

I believe this is fixed. Create a new issue if there's any problems remaining.

@nesquena nesquena closed this Dec 6, 2013

@DouweM DouweM added a commit to DouweM/rabl that referenced this issue Oct 8, 2014

@DouweM DouweM Better resource resolution for AR Scopes #142 c74540f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment