Invalid SQL generated when using `includes`, `joins`, `order` and `first` #14109

Closed
exviva opened this Issue Feb 19, 2014 · 11 comments

Comments

Projects
None yet
7 participants
Contributor

exviva commented Feb 19, 2014

I have a rather complex query, which fails in Rails 4.1.0.rc1.

User.includes(:profile_pictures).joins('LEFT JOIN profile_views ON profile_views.user_id = users.id').order('profile_views.created_at NULLS FIRST').first

It generetes the following (invalid) SQL:

SELECT  DISTINCT "users"."id", profile_views.created_at NULLS FIRST AS alias_0 FROM "users" LEFT OUTER JOIN "profile_pictures" ON "profile_pictures"."user_id" = "users"."id" LEFT JOIN profile_views ON profile_views.user_id = users.id  ORDER BY profile_views.created_at NULLS FIRST LIMIT 1

With Rails 4.0.3, I'm getting the correct query:

SELECT "users".* FROM "users" LEFT JOIN profile_views ON profile_views.user_id = users.id ORDER BY profile_views.created_at NULLS FIRST LIMIT 1

The weird thing is that I need all 4 methods (includes, joins, order with a namespaced.field and first)chained, removing any of them makes the query work again.

Member

arthurnn commented Feb 20, 2014

this is a gist that simulates the issue: https://gist.github.com/arthurnn/9117192
After git bisect, I found the the commit that introduced the regression is 22b3481 . Not sure yet what the problem is though. cc @senny

Contributor

exviva commented Feb 20, 2014

@arthurnn thanks for the investigation! I'd be glad to spend some time fixing this, if I only knew what was going on :(.

Owner

tenderlove commented Feb 20, 2014

@exviva did you get any warnings with this code when you ran it on 4.0?

Contributor

exviva commented Feb 21, 2014

@tenderlove no, I didn't get any Rails warnings, nor Ruby warnings (when run with -w). By the way, the original query is way more complex, this contrived example just shows the minimal isolated cause of error.

Member

senny commented Feb 26, 2014

This should have issued the deprecation warning added in a2dab46. Looking closer the deprecation code did only cover one branch of deprecated behavior.

The goal was to get rid of the SQL parsing method tables_in_string. It was used here and here.

The first linked occurrence was used to detect joined tables from String joins. This functionality did not issue the warning and was removed as well with 22b3481

@tenderlove A possible solution would be to bring tables_in_string back to only parse String joins. We can issue a new deprecation warning for that scenario and possibly remove it for 4.2.

Member

jonleighton commented Feb 26, 2014

Darn, sorry about that. I guess we have to bring it back for string joins then.

Member

senny commented Feb 26, 2014

@jonleighton Let me know if I should write the patch to bring it back or if you are working on it.

Member

jonleighton commented Feb 26, 2014

@senny if you're up for doing it please go ahead. thanks.

Owner

tenderlove commented Feb 26, 2014

Ideally, we would have added the deprecation code to the tables_in_string method so that we could have tracked and deprecated all branches that called that method.

@senny yes, please make the patch.

@exviva After @senny writes the patch, I think your code will work again. I suspect that adding a select() to your code will make it work now (but I haven't had a chance to test).

@tenderlove tenderlove closed this in #14226 Feb 28, 2014

Contributor

exviva commented Feb 28, 2014

Thanks for fixing this guys, I'll give it a spin over the weekend!

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