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

Fix a bug where virtual attribute are included when aliasing joined table column names #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

finalwharf
Copy link

@finalwharf finalwharf commented Oct 25, 2018

Issue

ActiveRecord 4.2 includes virtual attributes in selects with joins. This raises an error because virtual attributes are not persisted and there's no column with the name to the virtual attribute.

ActiveRecord::StatementInvalid: SQLite3::SQLException: no such column: people.county_plaintext ...

Background:

ActiveRecord 4.2 uses a slightly different implementation of Attributes API than in 5.0+. Before ActiveRecord 5, this API was considered private (although technically it wan't). In ActiveRecord 5+, the issue was fixed (by accident) because of the significant refactoring that took place in ActiveRecord and ActiveModel.

Some of the issues with virtual attributes in AR 4.2 were fixed with the addition of a new property called persistable_attribute_names. rails/rails@5383f22bb83 When checking for dirty attributes for example, they use that instead together with changed model attributes (which include the virtual ones as well).

Proposed Solution

For now we can override the column_names method of JoinPart, which the JoinDependency uses to generate aliases for the joined tables.

@FundingCircle/gdpr-engineering 👀

@finalwharf finalwharf force-pushed the fix_rails_4.2_join_aliases_bug branch 2 times, most recently from e36c4e0 to 0757f78 Compare October 25, 2018 12:36
@h-lame
Copy link

h-lame commented Oct 25, 2018

Seems fine based on everything you mentioned at standup - removing the names of the virtual attributes from the list of columns for the model sounds exactly like the sort of solution we need. That said, I doubt I'd have the context to understand the fix if I was looking at this PR tomorrow or next week 'cos by then I'll have forgotten the details of our standup discussion. To help future investigators can we include some more details of what the problem is and how it manifests itself in the commit message or PR description. I think you mentioned that it's a bug with rails 4.x attribute API that was fixed in rails 5.x, so it would be great to include a link to that fix too if we can.

@finalwharf finalwharf force-pushed the fix_rails_4.2_join_aliases_bug branch from 0757f78 to d7e4bd7 Compare October 25, 2018 13:17
@finalwharf finalwharf force-pushed the fix_rails_4.2_join_aliases_bug branch from d7e4bd7 to 78acc92 Compare October 25, 2018 14:08
Copy link

@AlexVPopov AlexVPopov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I echo @h-lame's notes that a reference to the relevant bug/PR/issue in Rails would good, other than that the PR LGTM.

P.S. 💯 for CreateProblems migration 😆

@finalwharf
Copy link
Author

I've updated the PR description with a proper explanation of the issue.
I've also changed the implementation a bit (see L206)

Copy link

@h-lame h-lame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on the new description; it's much clearer what's going on and will really help us if we need to come back to this.

@finalwharf finalwharf force-pushed the fix_rails_4.2_join_aliases_bug branch 4 times, most recently from b10fb95 to b065151 Compare October 26, 2018 15:32
@h-lame
Copy link

h-lame commented Oct 29, 2018

Would it make sense to only include these fixes in AR 4.2? E.g put a if ActiveRecord.version < Gem::Version("5") guard around the fix in the first commit, and around the whole file in the 2nd commit?

I know we have the guard on the presence of persistable_attribute_names but in the case of the 2nd commit we're still introducing our column_names method in rails 5 which doesn't need it at all.

@finalwharf finalwharf force-pushed the fix_rails_4.2_join_aliases_bug branch from b065151 to a04cf4a Compare October 29, 2018 13:51
@finalwharf
Copy link
Author

@h-lame I've addressed your comment even though it will still work with AR 5+. The column_names method is delegated to base_klass, and delegate actually does module_eval(method_def, ...), so no matter if we include my patch in AR patch the method will still exist. But I understand you concern!

@finalwharf
Copy link
Author

@elenatanasoiu please 👀

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