Skip to content
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

[3.2] active_record: Fix regression in predicate builder for join aliases. #9265

Conversation

dylanahsmith
Copy link
Contributor

Fixes #9257

Backports pull #9262.

The predicate builder was causing an exception to be raised when trying to
get the column for quoting integers when compared to an attribute on an
aliased table.

This commit takes the same approach as Arel, checking if the table exists,
and otherwise passing a nil column to quote.
@steveklabnik
Copy link
Member

/cc @carlosantoniodasilva @ernie

@ernie
Copy link
Contributor

ernie commented Feb 25, 2013

Yeah, same comment from the other PR applies here. I really don't like how this implementation gives the finger to Demeter. But it should work.

@rafaelfranca
Copy link
Member

@ernie any idea how can we improve it? Your input is always valuable

@ernie
Copy link
Contributor

ernie commented Feb 25, 2013

@rafaelfranca One thing I mentioned in the other PR is that we could consider making the column_for method in ARel public. It's basically doing the same thing already. Of course, that increases dependence on ARel being coupled to the DB connection, which is something we've talked about trying to reduce.

Unfortunately, I've been busy lately with work and prepping for talks, and unable to spend time on AR/ARel work. Based on the regression needing a fix, and lack of time to work something cleaner out, I'm not really opposed to merging this -- just think we need to keep in mind it's a band-aid.

@dylanahsmith
Copy link
Contributor Author

Yes, this is a band-aid until rails/arel#162 gets merged in Arel, after which we can remove the when Integer, ActiveSupport::Duration case that this if fixing an issue with.

@steveklabnik
Copy link
Member

Now that we've reverted that commit, this shouldn't be needed anymore. Thank you for the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants