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

Tests failing on Rails 5.2.0.rc1 #164

Closed
shioyama opened this issue Feb 7, 2018 · 7 comments
Closed

Tests failing on Rails 5.2.0.rc1 #164

shioyama opened this issue Feb 7, 2018 · 7 comments

Comments

@shioyama
Copy link
Owner

shioyama commented Feb 7, 2018

They were passing on beta, but some seem to be failing now on rc1. Need to investigate.

@shioyama
Copy link
Owner Author

shioyama commented Feb 7, 2018

If anyone is reading, the issues are all about the dirty plugin, as usual. Nothing else seems to be affected.

@shioyama
Copy link
Owner Author

shioyama commented Feb 7, 2018

For reference, I think the commit that broke things was rails/rails@a19e91f0fab

@shioyama
Copy link
Owner Author

shioyama commented Feb 7, 2018

Arg. This time Rails has really made a muck of things for Mobility, or any gem that wants to have pseudo-attributes on an AR model.

I don't quite know how to get around these changes without really digging deep into AR/AM internals, which seems like a bad idea. The basic problem is that Mobility tracks changes to localized attributes like title_en, but now with these changes AR/AM will not include these attributes in changes, so it looks like they haven't changed.

For now, dirty tracking in AR 5.2 is broken. I wish I could just hack out a fix but this one will take more time I'm afraid. I wonder if other gem maintainers may also be having an issue with this... I think it's a bad direction for Rails, but I confess that I don't know the internals well enough to make a detailed criticism.

@shioyama
Copy link
Owner Author

shioyama commented Feb 7, 2018

The key issue is that in ActiveModel, the mutation tracker is initialized with a set of attributes where the default falls through to a hash with an automatic default value:

https://github.com/rails/rails/blob/6a97a17f195a925959866edda2e951e20d7b1e76/activemodel/lib/active_model/dirty.rb#L252-L256

@attributes = AttributeSet.new(
  Hash.new { |h, attr|
    h[attr] = Attribute.with_cast_value(attr, _clone_attribute(attr), Type.default_value)
  }
)

For AR models, the attributes hash does not have a default value like this.

I think the best way to tackle this is probably to somehow change the mutation tracker on AR models to be aware of virtual translated attributes. I've tried actually modifying the attributes hash itself but this seems much more dangerous. Maybe we could subclass the mutation tracker and swap it out or something... no really good options here.

I hope we don't have to give up on the dirty plugin for AR models, but AR is certainly not making it easy to build this type of functionality.

@shioyama
Copy link
Owner Author

For now in 27585cc I've locked tests to 5.2.0.beta2, which passes. They fail in rc1.

@shioyama
Copy link
Owner Author

shioyama commented Mar 7, 2018

Wow good news: the main change that broke things in AR rc1 has been reverted, since it "makes assumptions that I do not want in the code base" according to @sgrif. I'm glad since I agree it did seem to make some quite sweeping assumptions.

That said, a smaller number of specs (I see six with postgres) are still failing, so will need to pinpoint why. In #166 I've changed the Gemfile to look at the master branch until the next release candidate is available.

@shioyama
Copy link
Owner Author

Fixed in #166, and I'll release 0.5.1 with the fix now.

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

No branches or pull requests

1 participant