`includes` uses SQL parsing when String joins are involved. #14226

Merged
merged 1 commit into from Feb 28, 2014

Conversation

Projects
None yet
4 participants
Owner

senny commented Feb 28, 2014

Closes #14109

This is a partial revert of 22b3481.
The current implementation of references_eager_loaded_tables? needs to know
every table involved in the query. With the current API this is not possible
without SQL parsing.

While a2dab46 deprecated SQL parsing for includes.
It did not issue deprecation warnings when String joins are involved. This resulted
in a breaking change after the deprecated behavior was removed (22b3481).

We will need to rethink the usage of includes, preload and eager_load but for now,
this brings back the old working behavior.

@senny senny `includes` uses SQL parsing when String joins are involved.
This is a partial revert of 22b3481.
The current implementation of `references_eager_loaded_tables?` needs to know
every table involved in the query. With the current API this is not possible
without SQL parsing.

While a2dab46 deprecated SQL parsing for `includes`.
It did not issue deprecation warnings when String joins are involved. This resulted
in a breaking change after the deprecated behavior was removed (22b3481).

We will need to rethink the usage of `includes`, `preload` and `eager_load` but for now,
this brings back the old *working* behavior.
d1e7cd1
Owner

senny commented Feb 28, 2014

@tenderlove the partial revert is enough to resolve the issue described in #14109. This patch does not yet deprecate the usage of SQL parsing when String joins are involved. Let me know if prefer the full revert.

senny added the activerecord label Feb 28, 2014

senny added this to the 4.1.0 milestone Feb 28, 2014

tenderlove was assigned by senny Feb 28, 2014

Owner

tenderlove commented Feb 28, 2014

Nope, this seems great. Thanks for the work!

tenderlove merged commit 422906d into rails:master Feb 28, 2014

1 check failed

default The Travis CI build failed
Details
Contributor

exviva commented Mar 2, 2014

@senny how about backporting to 4-1-stable?

Owner

senny commented Mar 2, 2014

@exviva this patch was specifically written for 4.1 not sure if there is a reason why @tenderlove did not yet backport.

Owner

chancancode commented Mar 2, 2014

In fact - do we need this on master at all? Is it going away in 4.2?—
Sent from Mailbox for iPhone

On Sun, Mar 2, 2014 at 10:01 AM, Yves Senn notifications@github.com
wrote:

@exviva this patch was specifically written for 4.1 not sure if there is a reason why @tenderlove did not yet backport.

Reply to this email directly or view it on GitHub:
#14226 (comment)

Owner

tenderlove commented Mar 2, 2014

Sorry, I haven't back ported yet because I forgot to do it. I will do it now.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Mar 2, 2014, at 10:01 AM, Yves Senn notifications@github.com wrote:

@exviva this patch was specifically written for 4.1 not sure if there is a reason why @tenderlove did not yet backport.


Reply to this email directly or view it on GitHub.

Owner

senny commented Mar 2, 2014

@chancancode it's better if it's on master for the moment. We need to take a closer look how to proceed before removing anything.

Owner

chancancode commented Mar 2, 2014

👍
Sent from Mailbox for iPhone

On Sun, Mar 2, 2014 at 11:28 AM, Yves Senn notifications@github.com
wrote:

@chancancode it's better if it's on master for the moment. We need to take a closer look how to proceed before removing anything.

Reply to this email directly or view it on GitHub:
#14226 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment