to_sql in Rails 4.2 returns parameterized queries instead of full SQL statements

[jfelchner]

I don't know if this belongs in Arel or in ActiveRecord so I'll move it if necessary.

As of Rails 4.2, to_sql's behavior has changed. Previously when I had an ActiveRelation and I called to_sql on it, I would get a SQL statement that I could copy and paste directly into Postgres' console or use in a find_by_sql call.

Currently in Rails 4.2 I get back a SQL statement that is identical except that it parameterizes it by removing all of the variable bits.

Here's a real life example (with the names stripped and aligned for your viewing pleasure):

# to_sql in Rails 4.1
SELECT "table_0".* FROM "table_0" WHERE "table_0"."id" = 'ed7b67eb-2b16-48a7-a23c-7656cdf7be5e' UNION SELECT "some_column" FROM "table_1" INNER JOIN "table_3" ON "table_3"."id" = "table_1"."table_3_id" INNER JOIN "table_4" ON "table_4"."table_3_id" = "table_3"."id" WHERE "table_3"."name" = 'Model' AND "table_1"."status" = 'active' AND "table_1"."id" = 'ed7b67eb-2b16-48a7-a23c-7656cdf7be5e' )

# to_sql in Rails 4.2
SELECT "table_0".* FROM "table_0" WHERE "table_0"."id" = $1                                     UNION SELECT "some_column" FROM "table_1" INNER JOIN "table_3" ON "table_3"."id" = "table_1"."table_3_id" INNER JOIN "table_4" ON "table_4"."table_3_id" = "table_3"."id" WHERE "table_3"."name" = $2      AND "table_1"."status" = $3       AND "table_1"."id" = $4 )

Effectively here's the code that's running:

def self.get_my_big_ole_query(some_id)
    TableZeroClass.
    where(id: some_id).
    union(
      my_other_active_relation.
      where(id: some_id),
    ).
    to_sql
end

def self.my_other_active_relation
    TableOneClass.
    models.
    active.
    select(
      :some_column,
    )
end

As a minor version release shouldn't break backward compatibility I see this as a fairly large problem.

[jfelchner]

BTW, I'm using Postgres 9.3.5 if that matters.

[AMHOL]

Guessing this is down to the merge of AdequateRecord and the query caching

[rafaelfranca]

As a minor version release shouldn't break backward compatibility I see this as a fairly large problem.

This is not what our maintenance policy say: http://guides.rubyonrails.org/maintenance_policy.html

Minor Y

New features, may contain API changes (Serve as major versions of Semver). Breaking changes are paired with deprecation notices in the previous minor or major release.

About to_sql it still returns what is documented. It was never made to return queries without bind parameters. In fact it returns bind parameters since 3.2 for what I can tell, but not in all queries. Not it is returning in all queries.

If you don't want this behavior you can:

1. disable prepared statement globally
2. disable prepared statement before calling to_sql with unprepared_statement.

def self.get_my_big_ole_query(some_id)
  TableZeroClass.unprepared_statement do
    TableZeroClass.
    where(id: some_id).
    union(
      my_other_active_relation.
      where(id: some_id),
    ).
    to_sql
  end
end

[matthewd]

As of 4.2, it's actually expected to never contain bind variables -- though as @rafaelfranca said, it has historically done so, and we've made no stronger claim that it won't any more.

So, while this isn't a bug / breaking change by support policy, we'd probably still like to understand what's going on, and possibly change it.

[rafaelfranca]

👍 maybe I have missed this change to never return bind parameters.

On Wed, Jan 7, 2015, 08:18 Matthew Draper notifications@github.com wrote:

Reopened #18379 #18379.

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

[rafaelfranca]

Right. It is not intended to return with binds

rails/activerecord/lib/active_record/relation.rb

Line 564 in 9892d44

collect.substitute_binds(binds).join

.

[jfelchner]

First and most importantly, what do you all need from me to help you all track this bug down? 🐛

And in response to your comments:

@rafaelfranca I'd thought I'd heard that Rails had started using SemVer. Evidently it's using MarketingVer where the first digit means nothing other than "this is how many times we've released Rails at RailsConf" 😄

@rafaelfranca @matthewd to_sql is, in fact, documented to return an unparameterized query:

If it was not the case then it would be:

User.where(name: 'Oscar').to_sql
# => SELECT "users".* FROM "users"  WHERE "users"."name" = $1

If you mean it's not explicitly documented to return only an unparameterized query and therefore it's ok to make it do so, I think that's a poor argument. I'm sure there are a lot of things in the documentation which are not explicit but which are expected by most developers.

I think most developers would be surprised that a method called to_sql doesn't return a full and executable SQL statement.

[jfelchner]

@rafaelfranca your proposed solution:

def self.get_my_big_ole_query(some_id)
  TableZeroClass.unprepared_statement do
    TableZeroClass.
    where(id: some_id).
    union(
      my_other_active_relation.
      where(id: some_id),
    ).
    to_sql
  end
end

results in NoMethodError: undefined method 'unprepared_statement' for #<Class:0x007fe0bd0835b8>

unprepared_statement is actually defined on the connection. However doing:

def self.get_my_big_ole_query(some_id)
  TableZeroClass.connection.unprepared_statement do
    TableZeroClass.
    where(id: some_id).
    union(
      my_other_active_relation.
      where(id: some_id),
    ).
    to_sql
  end
end

Results in a LocalJumpError.

Even if I pare it down to:

def self.get_my_big_ole_query(some_id)
  TableZeroClass.connection.unprepared_statement do
    TableZeroClass.
    where(id: some_id).
    to_sql
  end
end

The LocalJumpError persists.

[jfelchner]

But getting back to the original problem which is that to_sql returns parameterized queries in the first place: it appears that it may be the union call that's the culprit as both:

TableOneClass.
    models.
    active.
    select(
      :some_column,
    ).to_sql

and

TableZeroClass.
where(id: some_id).
to_sql

Each result in an unparameterized query.

[tenderlove]

First and most importantly, what do you all need from me to help you all track this bug down? 🐛

A self-contained reproduction script would be a great start.

I'd thought I'd heard that Rails had started using SemVer. Evidently it's using MarketingVer where the first digit means nothing other than "this is how many times we've released Rails at RailsConf"

This is the type of stuff that wears out OSS maintainers. We should stick to fixing the bug, please.

it appears that it may be the union call

When did Rails start supporting union? I can't seem to find the implementation. Are you using a third party library?

Each result in an unparameterized query.

So it's behaving as the docs say.

[matthewd]

First and most importantly, what do you all need from me to help you all track this bug down?

@jfelchner the most productive path to getting this issue solved would've been for you to skip this comment entirely, I think.

Sarcastic mea culpa aside, if you can find any official source that claims Rails uses SemVer, please point it out so we can fix it.

I think my meaning was pretty clear: "it has historically done so, and we've made no stronger claim that it won't any more". to_sql has always returned strings containing bind variables, in a variety of largely unpredictable situations. But that's irrelevant, because:

I think most developers won't expect an instance of one class to conform to the documented API of a different class, from a different library.

And you're getting that instance because you're calling a method that's not part of AR's documented API.

[jfelchner]

@tenderlove @matthewd the SemVer comment was a joke... hence the smiley. Evidently a poor one from your perspective. I should have understood in the context of what you all have to deal with every day maintaining this huge project that it might come across the wrong way. My apologies. ❤️

The rest of the comment however I absolutely meant, but it may have come across with the wrong tone due to the way you took the "joke". There was no blame, anger or incredulity intended, but simply an explanation of why I feel that this is a bug and should not be considered an appropriate API change.

Now, back to the problem at hand:

@tenderlove

When did Rails start supporting union? I can't seem to find the implementation. Are you using a third party library?

I'm not using anything other than what Rails provides. I know that, when I was reaching for a way to do a union on two Relation objects, the first thing I tried was to call union. Similar to how most of the other SQL statements are represented as methods on Relation. My assumption was that, although not documented, it was simply an accidental omission, but was still intended to work as the other SQL-like calls do. If this is not the case, then perhaps a warning is necessary?

So it's behaving as the docs say.

If you mean that insofar as certain Relation objects return unparameterized queries, then yes.

A self-contained reproduction script would be a great start.

I'll definitely see about getting a repo up that you can test.

@matthewd

And you're getting that instance because you're calling a method that's not part of AR's documented API.

If that's the case, then I think having a Relation object respond to union but without it working like other SQL-like methods on Relation may not be the best course of action seeing as how it seems likely that a dev would use it without even looking it up to verify. A "Hey I wonder if this works?" kind of situation.

Thanks all.

[jfelchner]

@tenderlove I did one more quick dig through and it appears that maybe union on Relation is delegating to Arel. What I get back after the union is an Arel::Nodes::Union class and not a Relation. Therefore the to_sql call isn't even a call to Relation.

This was transparent to me before because it was coincidental that the to_sql on Arel just happened to behave the same way as the to_sql on Relation.

Personally I think it would be desirable for union to work properly in the context of Relation or at the very least to throw a warning for the user.

Just let me know what you all decide to do (if anything) and I'll be happy submit a PR to make it so.

[sgrif]

It's worth noting that the union method on Relation is not public API

[jfelchner]

@sgrif sweet yeah, I think what we've come to in this issue is whether @tenderlove, @matthewd and/or @rafaelfranca have come to a decision on whether or not:

• they want it to be added to the public API for Relation
• a warning about using it for people who just guess it should work
• nothing at all and leave it how it is

[sgrif]

union definitely ins't ready to be public API, since it returns an Arel node. We have tons of internal methods you might think will "just work". Should we warn for all of those, as well? I don't think there's anything actionable to do here.

[jfelchner]

@sgrif I know it's not ready to be a public API, I was mentioning that I think it should be. And that I'd be willing to do the work.

So you're saying that you don't think that union would be a valuable addition to the Relation API?

If so, that's fine, I just want to make sure we're on the same page.

[matthewd]

@jfelchner we might eventually grow some union behaviour beyond #16052, which I'll be returning to shortly now that 4.2 is out -- I'm initially making it very restrictive, so we have more room to make it DWIM later.

If you're unioning two relations that are Quite Different Indeed, though, that seems beyond the realm of what Relation can support: if you have a Relation, we're fairly obliged to be sure it corresponds to rows of the matching AR::Base class's table. (That doesn't quite hold when group gets involved, but it's not a coincidence that that causes unexpected behaviour in a bunch of places.)

Given that you're returning raw SQL, maybe talking to Arel would in fact be more in line with your goal? Insert disclaimer about Arel's comparatively unstable API here... as you've just encountered 😉. Perhaps there's room for us to make the AR::Relation -> Arel transition smoother, wrt bind params?

There is an actionable point here, though I wouldn't consider it a direct offshoot of this issue: active_record/relation/delegation.rb#L94-L96 should be 🔥; if there's anything that should legitimately delegate, it should be explicit.