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

Rails/Arel union query with mysql2 driver fails but works with sqlite3 #16978

Closed
maletor opened this Issue Sep 19, 2014 · 18 comments

Comments

Projects
None yet
4 participants
@maletor
Contributor

maletor commented Sep 19, 2014

I have produced a Gist that proves what I am talking about. The failing line is within Rails, but I believe the actual bug may be in the mysql2 driver. That being said, I'm not completely sure who's responsibility this bug is so I have also posted the same bug on mysql2.

https://gist.github.com/maletor/af1aae5086ba10ed7cad

This is the failing line where we have a block trying to call reverse on nil: https://github.com/rails/rails/blob/v4.1.5/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L14

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 19, 2014

Member

First thing, arel_table is private API of Rails and should not be used in applications.

Talking about this specific issue you are getting this error because your query is expecting binds for the prepared statement and you are not providing.

When you use arel it is your responsibility to provide the correct binds.

Member

rafaelfranca commented Sep 19, 2014

First thing, arel_table is private API of Rails and should not be used in applications.

Talking about this specific issue you are getting this error because your query is expecting binds for the prepared statement and you are not providing.

When you use arel it is your responsibility to provide the correct binds.

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Sep 19, 2014

Contributor

Hey, is this caused by mysql2 not supporting prepared statements yet? If so, it is on the agenda for mysql2 0.4.0, and there are outstanding PRs to implement prepared statements in mysql2 that will be picked up again soon.

Contributor

sodabrew commented Sep 19, 2014

Hey, is this caused by mysql2 not supporting prepared statements yet? If so, it is on the agenda for mysql2 0.4.0, and there are outstanding PRs to implement prepared statements in mysql2 that will be picked up again soon.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 19, 2014

Member

oops, I closed by mistake.

Member

rafaelfranca commented Sep 19, 2014

oops, I closed by mistake.

@rafaelfranca rafaelfranca reopened this Sep 19, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 19, 2014

Member

@sodabrew this is a good question. As it is using mysql2 it should not be using prepared statement at all. I believe this is the reason for this bug.

Member

rafaelfranca commented Sep 19, 2014

@sodabrew this is a good question. As it is using mysql2 it should not be using prepared statement at all. I believe this is the reason for this bug.

@maletor maletor referenced this issue in brianmario/mysql2 Sep 19, 2014

Closed

Union fails with mysql2 driver #546

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Sep 19, 2014

Contributor

@rafaelfranca, ignore the arel_table thing, that's not the issue. The real issue is union.to_sql.

I've updated the gist to make this more clear.

Contributor

maletor commented Sep 19, 2014

@rafaelfranca, ignore the arel_table thing, that's not the issue. The real issue is union.to_sql.

I've updated the gist to make this more clear.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Sep 22, 2014

Contributor

@sodabrew, I'm pretty sure this has to do with prepared statements. I can work around the bug by not passing object.associations and passing Object.joins(:associations).where(id: self). Less than awesome, but helped narrow this down.

It looks like this is definitely a bug (lack of feature?) on mysql2.

Contributor

maletor commented Sep 22, 2014

@sodabrew, I'm pretty sure this has to do with prepared statements. I can work around the bug by not passing object.associations and passing Object.joins(:associations).where(id: self). Less than awesome, but helped narrow this down.

It looks like this is definitely a bug (lack of feature?) on mysql2.

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Sep 22, 2014

Contributor

Lack of feature :) But as @rafaelfranca points out, the AR driver should try to use a prepared statement knowing that the underlying driver doesn't support it. Are youmanually coding the SQL with ? binds are you? If AR is generating that code for you, then it's an AR bug.

Contributor

sodabrew commented Sep 22, 2014

Lack of feature :) But as @rafaelfranca points out, the AR driver should try to use a prepared statement knowing that the underlying driver doesn't support it. Are youmanually coding the SQL with ? binds are you? If AR is generating that code for you, then it's an AR bug.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Sep 22, 2014

Contributor

@sodabrew, refer to the test case -- it's generated by AR.

Contributor

maletor commented Sep 22, 2014

@sodabrew, refer to the test case -- it's generated by AR.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Sep 23, 2014

Contributor

@sodabrew, so we should close the bug on mysql2 or...? Still confused who's responsibility this bug is.

Contributor

maletor commented Sep 23, 2014

@sodabrew, so we should close the bug on mysql2 or...? Still confused who's responsibility this bug is.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Oct 7, 2014

Contributor

First thing, arel_table is private API of Rails and should not be used in applications.

@rafaelfranca, I don't think people know or care that arel_table should be internal to Rails. If that is true why has it been made public?

I mean, there are whole talks and blog posts about how to leverage the power of Arel in your application.

Am I misunderstanding what you said or are people using it wrong en masse?

Contributor

maletor commented Oct 7, 2014

First thing, arel_table is private API of Rails and should not be used in applications.

@rafaelfranca, I don't think people know or care that arel_table should be internal to Rails. If that is true why has it been made public?

I mean, there are whole talks and blog posts about how to leverage the power of Arel in your application.

Am I misunderstanding what you said or are people using it wrong en masse?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 7, 2014

Member

People are using wrong.

There is a difference between the method being public and the method being
public api. Public api is documented and present in the api website. A
method can have public visibility and being private api.
On Oct 7, 2014 5:15 PM, "Ellis Berner" notifications@github.com wrote:

First thing, arel_table is private API of Rails and should not be used in
applications.

@rafaelfranca https://github.com/rafaelfranca, I don't think people
know or care that arel_table should be internal to Rails. If that is
true why has it been made public?

I mean, there are whole talks
https://danshultz.github.io/talks/mastering_activerecord_arel/#/19 and blog
posts http://robots.thoughtbot.com/using-arel-to-compose-sql-queries
about how to leverage the power of Arel in your application.

Am I misunderstanding what you said or are people using it wrong en masse?


Reply to this email directly or view it on GitHub
#16978 (comment).

Member

rafaelfranca commented Oct 7, 2014

People are using wrong.

There is a difference between the method being public and the method being
public api. Public api is documented and present in the api website. A
method can have public visibility and being private api.
On Oct 7, 2014 5:15 PM, "Ellis Berner" notifications@github.com wrote:

First thing, arel_table is private API of Rails and should not be used in
applications.

@rafaelfranca https://github.com/rafaelfranca, I don't think people
know or care that arel_table should be internal to Rails. If that is
true why has it been made public?

I mean, there are whole talks
https://danshultz.github.io/talks/mastering_activerecord_arel/#/19 and blog
posts http://robots.thoughtbot.com/using-arel-to-compose-sql-queries
about how to leverage the power of Arel in your application.

Am I misunderstanding what you said or are people using it wrong en masse?


Reply to this email directly or view it on GitHub
#16978 (comment).

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Oct 7, 2014

Contributor

So what would you say to them? Fine, it's a publicly private method, but how do you propose people get access to the API, because the points about its advantages are valid.

Are you ok with people explicitly marking Arel as a dependency (versus solely as a dependency of activerecord) and calling Arel::Table.new(:users) versus User.arel_table?

Contributor

maletor commented Oct 7, 2014

So what would you say to them? Fine, it's a publicly private method, but how do you propose people get access to the API, because the points about its advantages are valid.

Are you ok with people explicitly marking Arel as a dependency (versus solely as a dependency of activerecord) and calling Arel::Table.new(:users) versus User.arel_table?

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Oct 7, 2014

Contributor

@rafaelfranca, I just looked it up and it turns out it is documented and examples are given as if within the application domain. Does this change anything or does the method need to be marked :nodoc:?

Contributor

maletor commented Oct 7, 2014

@rafaelfranca, I just looked it up and it turns out it is documented and examples are given as if within the application domain. Does this change anything or does the method need to be marked :nodoc:?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 7, 2014

Member

So what would you say to them? Fine, it's a publicly private method, but how do you propose people get access to the API, because the points about its advantages are valid.

The correct way to get access to the API is exposing it through Active Record.

Are you ok with people explicitly marking Arel as a dependency (versus solely as a dependency of activerecord) and calling Arel::Table.new(:users) versus User.arel_table?

About this, both have the same problem. Even if you use arel as explicity dependency we don't promise that all Active Record APIs will work with plain arel.

Does this change anything or does the method need to be marked :nodoc:?

It is nodoc'ed already. The documentation and examples are only for internal propose. It is also an example in our documentation about method visibility.

Member

rafaelfranca commented Oct 7, 2014

So what would you say to them? Fine, it's a publicly private method, but how do you propose people get access to the API, because the points about its advantages are valid.

The correct way to get access to the API is exposing it through Active Record.

Are you ok with people explicitly marking Arel as a dependency (versus solely as a dependency of activerecord) and calling Arel::Table.new(:users) versus User.arel_table?

About this, both have the same problem. Even if you use arel as explicity dependency we don't promise that all Active Record APIs will work with plain arel.

Does this change anything or does the method need to be marked :nodoc:?

It is nodoc'ed already. The documentation and examples are only for internal propose. It is also an example in our documentation about method visibility.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Oct 7, 2014

Contributor

The correct way to get access to the API is exposing it through Active Record.

There are a plethora of things Active Record does not do that Arel is capable of doing. How about support for OR? How about fixing the issue of ambiguous column names when doing a join and an order?

Using a String seems like more of a regression to me than using an API like the one offered by Arel.

Contributor

maletor commented Oct 7, 2014

The correct way to get access to the API is exposing it through Active Record.

There are a plethora of things Active Record does not do that Arel is capable of doing. How about support for OR? How about fixing the issue of ambiguous column names when doing a join and an order?

Using a String seems like more of a regression to me than using an API like the one offered by Arel.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 7, 2014

Member

There are a plethora of things Active Record does not do that Arel is capable of doing.

Yes. This is why we should add support to these things.

How about support for OR?

#16052

How about fixing the issue of ambiguous column names when doing a join and an order?

We don't have an API for this but if it is a common need we should implement it.

Member

rafaelfranca commented Oct 7, 2014

There are a plethora of things Active Record does not do that Arel is capable of doing.

Yes. This is why we should add support to these things.

How about support for OR?

#16052

How about fixing the issue of ambiguous column names when doing a join and an order?

We don't have an API for this but if it is a common need we should implement it.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Oct 7, 2014

Contributor

Ok, we are on the same page now. Thanks for the quick responses. I may look into that last one separately but we have seriously digressed from the original issue here now.

Contributor

maletor commented Oct 7, 2014

Ok, we are on the same page now. Thanks for the quick responses. I may look into that last one separately but we have seriously digressed from the original issue here now.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jan 10, 2015

Member

I'm closing this as a dup of the (later, notabug) #18379 (comment)

Member

matthewd commented Jan 10, 2015

I'm closing this as a dup of the (later, notabug) #18379 (comment)

@matthewd matthewd closed this Jan 10, 2015

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