-
-
Notifications
You must be signed in to change notification settings - Fork 730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start upgrade to Rails 4 #3709
Start upgrade to Rails 4 #3709
Conversation
f64e2cd
to
e0fd5ff
Compare
@luisramos0 I updated the description because Github thought that you were saying that the issue is closed by this PR. It doesn't understand negation as in "it doesn't close #1001110". |
@@ -53,7 +53,7 @@ def unauthorized | |||
def active_distributors_not_ready_for_checkout | |||
ocs = OrderCycle.managed_by(spree_current_user).active | |||
distributors = ocs.map(&:distributors).flatten.uniq | |||
Enterprise.where('id IN (?)', distributors).not_ready_for_checkout | |||
Enterprise.where('enterprises.id IN (?)', distributors).not_ready_for_checkout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the table? AR infers it from the class you are calling to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of the scope not_ready_for_checkout the query errors out with "ambiguous field id". this change fixes that problem in 2-1-stable.
@@ -47,7 +47,7 @@ class OrderCycle < ActiveRecord::Base | |||
if user.has_spree_role?('admin') | |||
scoped | |||
else | |||
where('coordinator_id IN (?)', user.enterprises) | |||
where('coordinator_id IN (?)', user.enterprises.map(&:id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we checked we are not affecting performance here? When passing the association I guess AR calls enterprises_ids
which will do clever things, whereas with map we load the entire association and then iterate it.
I'd like to understand what's the actual change we are doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error we are fixing is that we can't have enterprises matching coordinator_id, we need enterprises ids. it just fails in rails 4.
can we use user.enterprises_ids? instead of user.enterprises.map(&:id)?
we could use select or map, but this case is better with map because the user.enterprises is already lazy loaded, with select we would trigger another db query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with select we would trigger another db query
Have you seen that happening? All my experiments resulted in a sub-query. I'm pretty sure about that.
Now it's a trade-off between running the sub-query vs transferring a list of ids. My hunch is that there is no big difference for small lists. For big lists the database is probably more efficient in handling the data than Ruby compiling the list and transferring it through a TCP connection.
I'm happy to leave it as is in this case. Probably leaning towards select as good style in the future. But I would be happy to learn in which cases this statement isn't true and AR issues another query as well. Why would it do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this work. It feels like progress.
I also like that Rails 4 is becoming less smart. It makes the code more explicit and easier to understand. It also gives use more choices though. We need to find best practices for these. Did you know about the use of sub queries when using relations?
Enterprise.where("id not in (?)", user.enterprises.select('enterprises.id')).to_sql
"SELECT \"enterprises\".* FROM \"enterprises\" WHERE (id not in (2346))"
@@ -47,7 +47,7 @@ class OrderCycle < ActiveRecord::Base | |||
if user.has_spree_role?('admin') | |||
scoped | |||
else | |||
where('coordinator_id IN (?)', user.enterprises) | |||
where('coordinator_id IN (?)', user.enterprises.map(&:id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So your guess is that enterprises are loaded already and we don't need to start a special query here? Otherwise this would be more efficient:
where('coordinator_id IN (?)', user.enterprises.select("enterprises.id"))
It's difficult to judge in a scope though. We don't know who will be using the scope and what else they are doing.
Uh, I just realised that we are using the result in a sub-query here. So there is no additional query with select
. It will just be inserted, right?
WHERE coordinator_id IN (SELECT enterprises.id FROM "enterprises" INNER JOIN ...)
In that case we potentially avoid querying the enterprises. In case of a big enterprise list, the sub query could also be much smaller than a long list of ids. At least, it would scale better.
Your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, user.enterprises are lazy loaded already and map is better. but seeing @sauloperez comment above made me realize user.enterprises_ids would be better. is user.enterprises_ids valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, calling *_ids
multiple times triggers a SQL query each time it is called while the association is not lazy loaded. But if the association is already lazy loaded, it uses the cache.
> user.recipe_ids ; nil
(0.7ms) SELECT "recipes".id FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipe_ids ; nil
(0.7ms) SELECT "recipes".id FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipes.to_a ; nil
Recipe Load (0.9ms) SELECT "recipes".* FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipe_ids
=> [1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite uncomfortable that the scope is now making assumptions about whether or not the enterprises
association is already lazy loaded. But yeah, happy about the way you do it here if the associations are already lazy-loaded anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe user.enterprise_ids would be better than map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Yeah actually @luisramos0, sounds good to me.
@@ -222,7 +222,7 @@ def exchanges_carrying(variant, distributor) | |||
end | |||
|
|||
def exchanges_supplying(order) | |||
exchanges.supplying_to(order.distributor).with_any_variant(order.variants) | |||
exchanges.supplying_to(order.distributor).with_any_variant(order.variants.map(&:id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. I'm not sure what performs better at the moment but I would guess that select
scales better than map
, especially because we use the result in sub queries and the database can handle it very efficiently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would not be a subquery, order.variants are already loaded at this point. so map is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, if order.variants is already loaded, AR will not make it a subquery. and using select on an already loaded order.variants will produce an extra query. probably because of the intense chaining of scopes...
e0fd5ff
to
366ebf4
Compare
@mkllnk "Did you know about the use of sub queries when using relations?" yes, but I dont think your example is always valid. it will not always be a subquery. I think that in the app user.enterprises will already be loaded and map will avoid an extra query. |
@mkllnk @sauloperez can you guys pls have a second look. I am pretty confident with the use of map instead of select... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very happy with this pull request. In this case I find it okay to ignore Code Climate as well, just to ignore merge conflicts on this long lasting pull request. It's big enough and it's not responsible for the violations, it just touched them.
e191077
to
b0a6bd0
Compare
b0a6bd0
to
de7618e
Compare
rebased to resolve conflicts with #3842 |
a26a975
to
0f46106
Compare
fixed rubocop issues, ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luisramos0 Please see my comment about select("DISTINCT enterprises.*")
and let me know what you think. I'm happy with the rest of the PR.
@@ -47,7 +47,7 @@ class OrderCycle < ActiveRecord::Base | |||
if user.has_spree_role?('admin') | |||
scoped | |||
else | |||
where('coordinator_id IN (?)', user.enterprises) | |||
where('coordinator_id IN (?)', user.enterprises.map(&:id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, calling *_ids
multiple times triggers a SQL query each time it is called while the association is not lazy loaded. But if the association is already lazy loaded, it uses the cache.
> user.recipe_ids ; nil
(0.7ms) SELECT "recipes".id FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipe_ids ; nil
(0.7ms) SELECT "recipes".id FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipes.to_a ; nil
Recipe Load (0.9ms) SELECT "recipes".* FROM "recipes" WHERE "recipes"."user_id" = $1 [["user_id", 1]]
=> nil
> user.recipe_ids
=> [1]
@@ -47,7 +47,7 @@ class OrderCycle < ActiveRecord::Base | |||
if user.has_spree_role?('admin') | |||
scoped | |||
else | |||
where('coordinator_id IN (?)', user.enterprises) | |||
where('coordinator_id IN (?)', user.enterprises.map(&:id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite uncomfortable that the scope is now making assumptions about whether or not the enterprises
association is already lazy loaded. But yeah, happy about the way you do it here if the associations are already lazy-loaded anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🚀
Conflicts @luisramos0 |
Adapt use of enterprise scope not_ready_for_checkout to rails 4 by adding enterprises table alias to selected field id
6091b1c
to
78f8a2a
Compare
yeah, thanks Sigmund! |
…prises and not ids again and by making not_ready_for_checkout select the id field from the ready_for_checkout scope
78f8a2a
to
1da18d3
Compare
With product cache active, I've tested:
Everything looks good. |
🎉 |
What? Why?
Starts #3708
This makes some of our code in models compatible with rails 4 by making scopes select the appropriate field when using the IN operator.
This PR does not close the issue, there are quite a few more places where we have to do this, this PR serves as a sample of what we need to do so we can validate the approach and do the same for the rest of the codebase.
map, select or pluck
The trick here is to know when to use map, select or pluck. With this PR I have learned a few things including how CollectionProxy works!
Relations have the methods select and pluck. Arrays have the map method.
A scope returns a relation but an association returns a CollectionProxy that has both methods from a relation select and pluck plus all the array methods including map. So we can use map on an association but not on a scope.
The challenge is that select returns a relation while pluck returns an array. So, when we use pluck we can't really chain relations and we create an extra query just for that pluck statement.
SO, when we are handling relations that will be chained or used in other queries, we need to use select.
What should we test?
This 2-0 build should be green.
This code can also be seen in the 2-1-stable where it fixes a few specs.
I think we will be ok with a sanity check. Maybe we can test adding variants to an order cycle and removing them and make sure the shopfront reacts correctly. Making sure the products cache is active.
Release notes
Changelog Category: Changed
Started adapting our codebase to Rails 4.
How is this related to the Spree upgrade?
Upgrading our code to rails 4 is part of the upgrade to spree 2-1.