Fix dup with multiple mounters or not cached #2645
Conversation
BrianHawley
force-pushed
the
fixes_1962
branch
from
e6e0628
to
e3625f0
Compare
|
Thanks for the PR. Where's the fix for this point?
|
spec/orm/activerecord_spec.rb
Outdated
| t.column :textfile, :string | ||
| t.column :textfiles, :json | ||
| t.column :foo, :string | ||
| describe CarrierWave::ActiveRecord, :aggregate_failures do |
There was a problem hiding this comment.
Could you remove this :aggregate_failures ? While it can be useful for local development, I don't see any benefit for permanently enabling this.
There was a problem hiding this comment.
I could remove it, but there's no harm in keeping it.
The benefit is that it's also useful in CI for future developers to debug future test failures. It gives better errors when things fail, and has no overhead in the success case. It really helps on multi-person teams where only one person has to know the feature exists, but everyone gets the benefit of better results.
Still, your call, so I removed it.
lib/carrierwave/orm/activerecord.rb
Outdated
| old_mounters.each do |column, mounter| | ||
| _mounter(:#{column}).cache(mounter.uploaders) | ||
| old_mounters&.each do |column, mounter| | ||
| _mounter(column).cache(mounter.uploaders) |
There was a problem hiding this comment.
Weirdly enough, these two lines have the side effect of fixing the method being defined in the wrong context.
You don't want this method defined unless you have at least one mount field in the model, so it should be added by this method builder somehow. The problem is, the method gets redefined for every mount field, and these two lines apply to all fields and you only want them to run once.
I could have tried to make another module for this and reload that only gets included once, and then check whether it's there before including it again. But, that would add a module name, because you'd need a name to see if it was included before, and that inclusion check is a little tricky to do compatibly across all the Ruby versions supported.
However, I noticed that if this method is defined and called repeatedly in the same inheritance chain, @_mounters is nil already for every later call, due to it having been assigned nil in the earlier call on line 97, so old_mounters will always be nil for later calls. When old_mounters is nil, old_mounters&.each doesn't do anything. This means that the later calls are safe to run because they don't do anything significant that can't be repeated. And by changing :#{column} to column, that makes the first method called do the work for all the defined fields properly.
This is the same trick that makes the reload method above safe to repeat, as it only assigns a nil to the same variable once per field. I don't know if that was intentional, but it works.
So with this trick, we don't have to make a new context, and can just live with these methods being repeated. It works well enough, doesn't have a lot of overhead, and I made sure that the tests verify it works (the be_a(@uploader) vs be_a(@uploader1) assertions).
If you prefer, we could make another named module and use ActiveSupport::Concern. That way the module could be included every time in the prepended module defined above and ActiveSupport::Concern would handle the multiple inclusion in a compatible way.
@mshibuya FYI.
|
@mshibuya I've used this fix in my app at work (to fix after updating carrierwave yesterday) and it solves the issues I ran into: NoMethodError on nil, fortunately caught by our test suite. I verified it there before making the PR here. |
BrianHawley
force-pushed
the
fixes_1962
branch
from
e3625f0
to
f7d282d
Compare
| private def foo=(value); raise; end | ||
| private | ||
|
|
||
| def foo=(value); raise; end |
There was a problem hiding this comment.
This was just to get a fully passing rubocop run.
lib/carrierwave/orm/activerecord.rb
Outdated
| old_mounters = @_mounters if instance_variable_defined?(:@_mounters) | ||
| @_mounters = nil |
There was a problem hiding this comment.
| old_mounters = @_mounters if instance_variable_defined?(:@_mounters) | |
| @_mounters = nil | |
| old_uploaders = _mounter(:"#{column}").uploaders | |
| @_mounters[:"#{column}"] = nil |
lib/carrierwave/orm/activerecord.rb
Outdated
| old_mounters&.each do |column, mounter| | ||
| _mounter(column).cache(mounter.uploaders) | ||
| end |
There was a problem hiding this comment.
| old_mounters&.each do |column, mounter| | |
| _mounter(column).cache(mounter.uploaders) | |
| end | |
| _mounter(:"#{column}").cache(old_uploaders) |
- Don't call `each` if `@_mounters` is unitialized or nil. It will be nil for calls to later mounters, or if not referenced or saved. - Don't interpolate `column` when it should be a local variable. - Don't define global functions in specs. Make them local to the block. - Cover more missing scenarios. - Fix some rubocop failures. [Fixes carrierwaveuploader#1962]
BrianHawley
force-pushed
the
fixes_1962
branch
from
f7d282d
to
e2af599
Compare
Sure, that will work. Updated with your changes, @mshibuya. |
|
Thank you! |
eachif@_mountersis unitialized or nil. It will be nil for calls to later mounters, or if not referenced or saved.columnwhen it should be a local variable.[Fixes #1962]