Revert introduction of forwardable
#107
Conversation
Mongoid 7.1.0 introduced a regression (see https://jira.mongodb.org/browse/MONGOID-4849) by switching to the builtin ruby forwardable method of delegation rather than relying on the ActiveSupport-provided delegation used before. mongoid-locker was made compatible with the regressed version of mongoid in mongodb/mongoid#4739 Mongoid then fixed this regression in 7.1.1+ with mongodb/mongoid@d078acd, which goes back to the active support way of delegating. The `forwardable` inclusion in mongoid-locker causes incompatibilities when you then include another gem that expects to be able to set up a delegator within a mongoid document class. For example the `mongoid-history` gem expects to be able to delegate using the active-support style here: https://github.com/mongoid/mongoid-history/blob/c8c4de1235c01252dfbdb9fe6c0abfc078ec1443/lib/mongoid/history/trackable.rb#L24-L25 This PR reverts the change to introduce `forwardable`. See mongoid/mongoid-history#238 (comment) for an example of how this interacts poorly with mongoid-history.
|
Thanks. Let's add a test that shows the problem? |
I couldn't think of a reasonable way to do this - can you point me in the direction you'd prefer? It seems strange and outside the scope of this gem to have a test that I could add a test dependency on mongoid-history and try to set up a mongoid document that fails if |
|
Maybe just add a simple integration test ala https://github.com/ruby-grape/grape/tree/master/spec/integration? |
spec/spec_helper.rb
Outdated
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| ENV['RACK_ENV'] = 'test' | |||
|
|
|||
| require 'bundler' | |||
| Bundler.require :default, :test | |||
There was a problem hiding this comment.
This is required for defined?(Mongoid::History) to be true in the integration spec.
Ok I took a stab at it. If you cherry-pick 9cb3e2d over to master the new spec will fail. |
| # This replicates the exception reported at https://github.com/mongoid/mongoid-history/issues/238#issuecomment-1063155193 | ||
| # when mongoid-locker required 'forwardable' instead of relying on the | ||
| # active support-provided delegation method | ||
| RSpec.describe 'MongoidHistory', if: defined?(Mongoid::History) do # rubocop:disable RSpec/DescribeClass |
There was a problem hiding this comment.
Is the if: needed? I expected for the test to be predictable.
There was a problem hiding this comment.
I was setting this up to be similar to what you linked to over at https://github.com/ruby-grape/grape/tree/master/spec/integration, where you have a suite of integration tests where not all gems are required all the time, but rather required conditionally for the various combination tests.
It would still work to remove this check or even remove appraisal entirely.
|
Sorry for the delay, CI has an issue. I have to approve it here in the upstream for first time contributors, but you can definitely see it in your fork when you push, so iterate to 🍏 . Thanks! |
scpike
force-pushed
the
revert-forwardable-inclusion
branch
from
9f4e509 to
a1e5086
Compare
scpike
force-pushed
the
revert-forwardable-inclusion
branch
from
e0b95ae to
794f081
Compare
|
Merging, thanks, will deal with the jruby failure in #108 |
|
If you want to do some more work here, the test matrix could use an upgrade to include more recent versions of rubies/MongoDBs. |
This PR attempts to fix an incompatibility between mongoid-locker >= 2.0.1 and mongoid-history. The issue was introduced by this PR. See mongoid/mongoid-history#238 (comment) for an example report.
Timeline
Here's a timeline of what I think happened:
delegatedoes, so mongoid-locker gets its expected behavior but other gems have theirdelegatechanged out from under them).Underlying issue
The
forwardableinclusion in mongoid-locker causes incompatibilities when you then include another gem that expects to be able to set up a delegator within a mongoid document class. For example themongoid-historygem expects to be able to delegate using the active-support style here: https://github.com/mongoid/mongoid-history/blob/c8c4de1235c01252dfbdb9fe6c0abfc078ec1443/lib/mongoid/history/trackable.rb#L24-L25Fix
This PR reverts the change to introduce
forwardableto go back to using the ActiveSupport-provideddelegate.