Rails 4 prep #4667

Matt-Yorkley · 2020-01-12T12:06:06Z

What? Why?

Pulls across some commits for upgrading to Rails 4. Mostly minor changes in syntax.

What should we test?

I think a green build will be enough to validate these changes.

Release notes

Adapted some syntax in preparation for Rails 4.

Changelog Category: Changed

luisramos0

nice!

app/controllers/spree/admin/base_controller.rb

mkllnk

Nice work.

Can you link me to some information about the issue with subqueries in Rails 4? Subqueries can be much more efficient, right?

mkllnk · 2020-01-16T01:25:01Z

app/models/enterprise.rb

@@ -117,7 +117,7 @@ class Enterprise < ActiveRecord::Base
    # checkout.
    ready_enterprises = Enterprise.ready_for_checkout.select('enterprises.id')
    if ready_enterprises.present?
-      where("enterprises.id NOT IN (?)", ready_enterprises)
+      where("enterprises.id NOT IN (?)", ready_enterprises.map(&:id))


I think that we can use pluck(:id) here to be more efficient. The select isn't needed then. But this could have a performance impact, the array can be big.

pluck could create an extra query, right?
I think this should be select :-)
#3709
I think we should agree on some rules for using map vs select vs pluck :-)

I think we should agree on some rules for using map vs select vs pluck :-)

That sounds good! And we could add it to the wiki

Thank you for the clarification. I now think that I know how it works.

In this particular case, there was a select that would lead to a sub-query. The present? call creates a separate query just looking if anything is there without loading the enterprises. Then applying map would first load all the enterprise objects and then copy the ids into an array. A pluck call would do that without creating the enterprise objects (less Ruby work).

If we used the enterprise objects somewhere else, we should use map to re-use the same array and not load the enterprises again. But I don't think we do it here, so pluck would be better than map.

Matt, you explained that select * is a problem with sub-queries in Rails 4. That's logical. We should definitely avoid that. In this case, we do select('enterprises.id') in line 118 which should allow us to do an efficient sub-query. That should be no problem with Rails 4, right?

With my current knowledge, we don't need map here and should use a sub-query. We could move the select from line 118 down to the 120 to make it clearer that we are selecting the ids for the IN query part.

awesome Maikel 👌

Aha! Yeah it looks like it's mostly the prepared statements in our scopes that are causing issues. In the #managed_by scope in enterprise_fee.rb this line throws a "subquery has too many columns" error:

where('enterprise_id IN (?)', user.enterprises)

but this does not:

where(enterprise_id: user.enterprises)

nice investigation Matt!!
is there a reason you are not satisfied with how we have been fixing things so far with:
where('enterprise_id IN (?)', user.enterprises.select(&:id))

I need to redo the other subquery PR and check some things in Rails 4. It looks like it will be better if we avoid prepared statements and favour the hash syntax, passing an ActiveRecord::Relation object (as mentioned above). And in that case I think we generally don't need the select, as ActiveRecord should in theory figure it out and build the subquery correctly from the hash parameters?

I am not sure I agree.
Prepared statements are a good thing in general. I dont think we should plan to just avoid them.
I dont think we should let active record control things, we should control things and we should name the exact fields we want in the query.
These are general rules I have seen used in other projects: as much prepared statements as possible and lists of fields in a query as specific as possible.

In general, the new Rails 4 way where.not(id: ready_enterprises) sounds better than what we have at the moment. But for compatibility with Rails 3 and 4, I would prefer where("enterprises.id NOT IN (?)", ready_enterprises.select("enterprises.id")) for now. The reason I prefer this version is that it's very explicit what's happening, it's compatible with many versions and avoids the Rails 3 bug where multiple where(id: collection) statements override each other.

Instead of optimising the way we use the ActiveRecord interface further, I would also suggest looking at the underlying problem and changing our logic here. This negation is really expensive. It would be better to find criteria for enterprises that make them not being ready for checkout and using those in a direct query.

One problem is that there can be several reasons why an enterprise is not ready. And we are querying them all together. Instead, we could actually query those individually and change the UI to say:

These enterprises are missing a shipping method: ...

These enterprises are missing a payment method: ...

...

@Matt-Yorkley I noticed that your most recent version still uses pluck. Does select('enterprises.id') still raise the too-many-column error? I don't understand that because enterprises.id is only one column, isn't it? Or are there many more in JOIN clauses that count as well?

Matt-Yorkley · 2020-01-16T13:20:06Z

Can you link me to some information about the issue with subqueries in Rails 4?

It's difficult to find anything concrete for this specific error. Basically when a subquery is not specifying which columns to fetch and defaulting to SELECT *, it throws a fatal error. It seems to be specific to Postgres and Rails 4. I guess it enforces more efficient queries by forcing you to select only the columns you will actually use in the subquery.

luisramos0 · 2020-01-16T13:28:45Z

there's this issue for it:
#3708

…s 4.

has_and_belongs_to_many relationships now require a join_table

sauloperez · 2020-01-24T10:38:41Z

app/controllers/spree/admin/base_controller.rb

-        return if distributors.empty? || flash[:notice].present?
-
-        flash[:notice] = active_distributors_not_ready_for_checkout_message(distributors)
+        warning = OrderCycleWarning.new(spree_current_user).call


With this abstraction the intent is much clearer now 👌

kristinalim

Nice work, @Matt-Yorkley! 😃

kristinalim · 2020-01-30T02:01:24Z

@mkllnk Are you happy with this PR already? 🙂

mkllnk · 2020-01-30T04:35:23Z

I'm still concerned about the performance of pluck in that one example. If it's really needed to be compatible with Rails 4, okay. But I got mixed answers here, nothing explaining why we can't use select in this example, selecting only once column in the subquery.

Matt-Yorkley · 2020-02-03T23:05:37Z

I'm still concerned about the performance of pluck in that one example.

I'll try it again in Rails 4

Matt-Yorkley · 2020-02-04T00:07:58Z

Aha! The resulting query here looked like this in Rails 4:

...NOT IN (SELECT DISTINCT enterprises.*, enterprises.id FROM "enterprises" JOIN...

The scope that's being negated here uses .select('DISCINCT enterpises.*) at the end. I've used #except here to override it, and it now works in Rails 4 without #pluck.

mkllnk · 2020-02-04T00:49:23Z

Great work @Matt-Yorkley! Only Code Climate is the usual grinch. 😉

sauloperez · 2020-02-04T14:20:18Z

All green so merging!

Matt-Yorkley self-assigned this Jan 12, 2020

Matt-Yorkley force-pushed the rails-4-prep branch 3 times, most recently from 2cd052c to 6ed7af9 Compare January 12, 2020 13:59

Matt-Yorkley mentioned this pull request Jan 12, 2020

3.0 Initial fixes #4641

Merged

luisramos0 approved these changes Jan 12, 2020

View reviewed changes

app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved

Matt-Yorkley force-pushed the rails-4-prep branch from f213f86 to 04b55cc Compare January 13, 2020 16:20

mkllnk reviewed Jan 16, 2020

View reviewed changes

Matt-Yorkley force-pushed the rails-4-prep branch from 04b55cc to 8b70165 Compare January 18, 2020 22:28

Matt-Yorkley added 4 commits January 22, 2020 12:17

Fix issue with each_serializer not being called in some cases in Rail…

57ca1d5

…s 4.

Fix subquery errors triggered by #warn_invalid_order_cycles

59ebfb9

Add join_table to enterprise groups relation

a02c58e

has_and_belongs_to_many relationships now require a join_table

Refactor BaseController

5f3abbf

sigmundpetersen added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 22, 2020

Matt-Yorkley removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 22, 2020

sauloperez approved these changes Jan 24, 2020

View reviewed changes

kristinalim approved these changes Jan 30, 2020

View reviewed changes

Matt-Yorkley force-pushed the rails-4-prep branch 2 times, most recently from 6c7979e to 2df09e3 Compare February 4, 2020 00:07

Matt-Yorkley added 2 commits February 4, 2020 11:10

Remove use of #pluck and ensure subquery does not include all columns

285c78a

Add frozen_string_literal comment to new class

7d71f21

Matt-Yorkley force-pushed the rails-4-prep branch from 2df09e3 to 7d71f21 Compare February 4, 2020 10:11

sauloperez merged commit 246235b into openfoodfoundation:master Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 4 prep #4667

Rails 4 prep #4667

Matt-Yorkley commented Jan 12, 2020

luisramos0 left a comment

mkllnk left a comment

mkllnk Jan 16, 2020

luisramos0 Jan 16, 2020

Matt-Yorkley Jan 16, 2020

mkllnk Jan 16, 2020

luisramos0 Jan 16, 2020

Matt-Yorkley Jan 18, 2020

luisramos0 Jan 18, 2020

Matt-Yorkley Jan 19, 2020 •

edited

luisramos0 Jan 19, 2020

mkllnk Jan 21, 2020

Matt-Yorkley commented Jan 16, 2020 •

edited

luisramos0 commented Jan 16, 2020

sauloperez Jan 24, 2020

kristinalim left a comment

kristinalim commented Jan 30, 2020

mkllnk commented Jan 30, 2020

Matt-Yorkley commented Feb 3, 2020

Matt-Yorkley commented Feb 4, 2020

mkllnk commented Feb 4, 2020

sauloperez commented Feb 4, 2020

Rails 4 prep #4667

Rails 4 prep #4667

Conversation

Matt-Yorkley commented Jan 12, 2020

What? Why?

What should we test?

Release notes

luisramos0 left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Jan 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley commented Jan 16, 2020 • edited

luisramos0 commented Jan 16, 2020

Choose a reason for hiding this comment

kristinalim left a comment

Choose a reason for hiding this comment

kristinalim commented Jan 30, 2020

mkllnk commented Jan 30, 2020

Matt-Yorkley commented Feb 3, 2020

Matt-Yorkley commented Feb 4, 2020

mkllnk commented Feb 4, 2020

sauloperez commented Feb 4, 2020

Matt-Yorkley Jan 19, 2020 •

edited

Matt-Yorkley commented Jan 16, 2020 •

edited