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 AM/AR Dirty plugin issues with AR 5.2 #116

Merged
merged 3 commits into from Nov 30, 2017

Conversation

Projects
None yet
2 participants
@shioyama
Owner

shioyama commented Nov 29, 2017

Fixes #115

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Nov 30, 2017

Owner

@pwim So I found a way to fix the problem while maintaining the current interface. Basically, I realized that any time we need to check if an attribute is a Mobility attribute in the context of attribute changes, we can do this quickly and easily if we track which attributes have changed by Mobility.

So I add a method mobility_changed_attributes which returns a set, and before calling attribute_will_change!, just add the attribute name to the set. Then we can override _read_attribute as before, except that rather than a tricky regex, we just check if this set contains the attribute, if it does then dispatch to __send__.

This is not beautiful, but it fixes all (currenty) dirty issues in AR 5.2, should have minimal performance impact (also since it uses def instead of define_method, and only includes the ReadAttribute method once), and of course only impacts users who use the dirty plugin.

I tried to think about how we could do things differently without fallthrough accessors, but the issue is that there is a case where the user (say) does not use locale accessors, and does not specify available_locales. In this case, we really don't know what locales we should be matching against. Fallthrough accessors allow this case to be handled seamlessly, although at a performance cost (which I will mention in the readme).

Also, regardless, any solution I can imagine will have to override _read_attribute, given how AR now overrides AM::Dirty methods. The only way around this would be to explicitly add attributes to the @attributes hash, which seems like it would be asking for yet more problems.

So... if specs pass (locally only remaining failures are with the generators), I'd like to merge this and release in 0.3.0 with the changes, so we're (mostly) prepared for the Rails 5.2 launch.

Owner

shioyama commented Nov 30, 2017

@pwim So I found a way to fix the problem while maintaining the current interface. Basically, I realized that any time we need to check if an attribute is a Mobility attribute in the context of attribute changes, we can do this quickly and easily if we track which attributes have changed by Mobility.

So I add a method mobility_changed_attributes which returns a set, and before calling attribute_will_change!, just add the attribute name to the set. Then we can override _read_attribute as before, except that rather than a tricky regex, we just check if this set contains the attribute, if it does then dispatch to __send__.

This is not beautiful, but it fixes all (currenty) dirty issues in AR 5.2, should have minimal performance impact (also since it uses def instead of define_method, and only includes the ReadAttribute method once), and of course only impacts users who use the dirty plugin.

I tried to think about how we could do things differently without fallthrough accessors, but the issue is that there is a case where the user (say) does not use locale accessors, and does not specify available_locales. In this case, we really don't know what locales we should be matching against. Fallthrough accessors allow this case to be handled seamlessly, although at a performance cost (which I will mention in the readme).

Also, regardless, any solution I can imagine will have to override _read_attribute, given how AR now overrides AM::Dirty methods. The only way around this would be to explicitly add attributes to the @attributes hash, which seems like it would be asking for yet more problems.

So... if specs pass (locally only remaining failures are with the generators), I'd like to merge this and release in 0.3.0 with the changes, so we're (mostly) prepared for the Rails 5.2 launch.

@shioyama shioyama changed the title from [WIP] Fix AM/AR Dirty plugin issues with AR 5.2 to Fix AM/AR Dirty plugin issues with AR 5.2 Nov 30, 2017

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Nov 30, 2017

Owner

Generator spec failures are from an issue in the specs, not the generators themselves. So basically this PR should fix all known issues with Rails 5.2.

Owner

shioyama commented Nov 30, 2017

Generator spec failures are from an issue in the specs, not the generators themselves. So basically this PR should fix all known issues with Rails 5.2.

shioyama added some commits Nov 29, 2017

Override _read_attribute in Dirty plugin
This is required due to a fix in AR 5.2:

rails/rails#28661

Previously, `__send__` was used to fetch the value of the attribute
before changes, which worked fine with locale accessors like
`title_en`. However, with the fix above, `@attributes` is now used,
which does not contain translated attributes (let alone locale
accessors). Since in general, locale accessors are an open-ended set, we
can't treat them the way normal attributes are treated, so we resort to
overriding `_read_attribute` to check if the attribute that has changed
is one that Mobility changed, and if so use `__send__`. This fixes the
issue, although overriding a private method is not ideal.

@shioyama shioyama merged commit 2da5d17 into master Nov 30, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Nov 30, 2017

Collaborator

Cool. Took a look at it, and nothing jumps out at me.

Collaborator

pwim commented Nov 30, 2017

Cool. Took a look at it, and nothing jumps out at me.

@shioyama shioyama deleted the fix_dirty_ar_5_2 branch Nov 30, 2017

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