search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Silence deprecation warning in rails 6 [redo] #384

Closed h-lame closed 3 years ago

h-lame commented 4 years ago

In rails 6 we get the following deprecation warning:

DEPRECATION WARNING: Class level methods will no longer inherit
scoping from `create` in Rails 6.1. To continue using the scoped
relation, pass it into the block directly. To instead access the full
set of models, as Rails 6.1 will, use `<model name>.default_scoped`.
(called from acts_as_list_list at /path/to/acts_as_list/lib/acts_as_list/active_record/acts/list.rb:229)

when we call find_or_create_by on a model with acts_as_list. To get rid of this deprecation warning, we need default_scoped.unscoped instead of the restricted unscope(:select, :where).

In the first commit I've also tweaked the Gemfile and Appraisals files to work out of the box now that rails 6 is available and it doesn't work with sqlite 1.3.

Note - this is a redo of #363 because I foolishly deleted the source repo and had to recreate it to update the PR. It also incorporates comments from that PR discussion:

Sorry for the noise. I'll close the other one.

sudara commented 4 years ago

Thanks for re-opening!

I went through the links to prior issues @brendon listed here: https://github.com/brendon/acts_as_list/pull/363#issuecomment-536344373

It does look like existing tests should cover this change https://github.com/brendon/acts_as_list/blob/master/test/test_list.rb#L331-L538

However I wasn't able to get those tests to fail, for example by changing the acts_as_list_list method to

          acts_as_list_class.unscoped.where(scope_condition)

So I'm not sure if I'm doing something wrong or those tests aren't doing what they think they are? Maybe someone else could verify. I was testing with bundle exec --gemfile gemfiles/rails_6_0.gemfile rake test test/test_list.rb

h-lame commented 4 years ago

Great question @sudara. I tried a bunch of scenarios:

I guess the question is, do we need the default_scoped bit in the default_scoped.unscoped chain - it was @kamipo who suggested that, in reference to a rails issue, so maybe they can shed some light?

Maybe there aren't enough tests to capture what we'd be missing out on by excluding default_scoped, or maybe it's something that won't fail in rails 6.0, but would fail in rails 6.1?

h-lame commented 4 years ago

Oh, and I'm getting these results by running just bundle exec rake test, but the --gemfile variant you used should be broadly equivalent (my Gemfile.lock and gemfiles/rails_6_0.gemfile.lock are almost identical - just slightly newer versions of rails 6 in my one compared to the gemfiles one).

brendon commented 4 years ago

The main goal of the selective unscoping was to preserve any special select and order scopes. The key idea (from memory) was that order does matter when we're running some of these queries (e.g. to prevent deadlocks).

mijoharas commented 3 years ago

Hey @brendon any update on this? I keep coming back and checking this PR from time to time to see if it's been merged. Is there anything I can do to help? can you clarify exactly what the issue/danger with this PR is? Is there a test for whatever the issue might be (that'd help me dig in to try and help ensure we mitigate whatever it might be)?

h-lame commented 3 years ago

FWIW: I locally added the rail 6.1.0.rc1 gem to a new appraisal and ran the tests under various versions of the code to see what would happen. The dev warning explicitly says rails 6.1 will change the behaviour so I hoped this would tell us something. Unfortunately:

  1. If we include my change to list.rb to swap unscope(:select, :where) with default_scoped.unscoped for rails 6+, the tests pass for rails 6.1, 6.0, and 5.2
  2. If we do not include my change to list.rb to swap unscope(:select, :where) with default_scoped.unscoped the tests pass for rails 6.1 and 5.2, but fail for 6.0 - the tests that fail are the ones I added about not raising a dep warning.
  3. If we include my change to list.rb to swap unscope(:select, :where) with default_scoped.unscoped but change it to apply to every rails version, the tests pass for rails 6.1, 6.0, and 5.2

Note: I couldn't get the appraisal for 4.2 to install locally - suspect I need a specific old ruby + pg for compiling - I suspect something would break in at least scenario 3 for 4.2.

If we're not confident that replacing unscope(:select, :where) with default_scoped.unscoped is actually a safe thing to do for acts_as_list it seems we could just do nothing and say "you're gonna get dep warnings on rails 6, they'll go away with rails 6.1". Of course, something has changed in rails with scoping, so we can't really say that by doing nothing we've resolved the warning the dep warning is there to tell us about.

There are two scenarios that seem likely to me:

  1. The behavioural change introduced by rails 6.1 doesn't actually affect acts_as_list
  2. The behavioural change introduced by rails 6.1 does affect acts_as_list, but we don't have test coverage that tells us about that change

I can't really say that I feel super confident either way TBH. My main concern was just getting rid of the dep warning from my console, and it seems it will go away when rails 6.1 is released and we upgrade our app so ... 🤷

brendon commented 3 years ago

Thanks @h-lame, that's super interesting. I had a suspicion that it may be a false warning. There is some more discussion on this here: https://github.com/rails/rails/issues/38881

Just to clarify my understanding: your code is calling find_or_create_by() and that is kicking off a callback from acts_as_list that is doing some potential shuffling that causes this deprecation warning?

brendon commented 3 years ago

What about:

acts_as_list_class.default_scoped.unscope(:select, :where)

?

h-lame commented 3 years ago

What about:

acts_as_list_class.default_scoped.unscope(:select, :where)

?

It's baffling that I didn't think to try this in the first place! Thanks for suggesting.

If it does that then rails 6-1, 6-0, 5-2, and 5-0 appraisals all pass. So maybe that's the solution (again, not entirely sure that introducing the default_scoped doesn't change something else, but it's pretty promising).

I'll push up a commit that does that so we can see what 4-2 does 'cos I can't get that running locally.

h-lame commented 3 years ago

Just to clarify my understanding: your code is calling find_or_create_by() and that is kicking off a callback from acts_as_list that is doing some potential shuffling that causes this deprecation warning?

Yes - turns out it is this situation. Relevant bits of the callstack:

.activerecord-6.0.3.4/lib/active_record/relation.rb:169:in `find_or_create_by'
# lines of activerecord internals
activerecord-6.0.3.4/lib/active_record/relation.rb:100:in `create'
# lines of activerecord internals for creating records leading to
activesupport-6.0.3.4/lib/active_support/callbacks.rb:825:in `_run_create_callbacks'
# lines of activerecord internals about running callbacks until we get to the acts_as_list callbacks
acts_as_list-1.0.0/lib/acts_as_list/active_record/acts/list.rb:254:in `add_to_list_bottom'
acts_as_list-1.0.0/lib/acts_as_list/active_record/acts/list.rb:309:in `increment_positions_on_lower_items'
acts_as_list/active_record/acts/list.rb:229:in `acts_as_list_list'
activerecord-6.0.3.4/lib/active_record/querying.rb:21:in `unscope'
# lines of activesupport internals for raising a deprecation

That rails issue isn't exactly conclusive, but it feels like it might not be what anyone expected.

brendon commented 3 years ago

Haha! It only occurred to me after reading through what default_scoped is for. It's for getting a scope that includes whatever is set as default_scope on the model, straight from the model class. From there you have a scope and so can unscope or do whatever you want. It's probably equivalent to calling all?

h-lame commented 3 years ago

Looks like travis and the appraisals are happy with those changes. Always use unscope(:select, :where) and for rails 6+ add a default_scoped in first to avoid the dep warning by explicitly opting in to the rails 6.1+ behaviour. I can tidy up the commits, for sure, but what do we reckon on this - mergeable as a solution?

brendon commented 3 years ago

Sounds good to me @h-lame, how far back goes default_scoped go? If it goes to Rails 4.2 then perhaps we could apply this across the board? Actually I just had a look and it goes back to 4.1. Let's have a go at applying it unconditionally and see how the test suite works.

brendon commented 3 years ago

Man Travis is quite slow at running tests now! I suppose it's free though. Ping me back if you notice it's done and green :)

h-lame commented 3 years ago

@brendon looks like it went fully green. So seems we can use default_scoped.unscope(:select, :where) everywhere. If you're happy with this, I'll tidy up the commits ready for a merge. Thanks for putting up with all the back and forth on this!

brendon commented 3 years ago

I'm happy with that. Glad to finally have figured this one out :D

h-lame commented 3 years ago

Ok, sorry for leaving this for a few days @brendon, but I've smooshed those WIP commits down and left it as the two appraisals update commits and one commit talking about using default_scoped before unscope. Thanks for the patience.

brendon commented 3 years ago

Perfect! Thanks for that @h-lame, great to get this one out of the gate!

fschwahn commented 3 years ago

@brendon is there a timeline for when this will be released as a gem?

brendon commented 3 years ago

Merry Christmas @fschwahn :) 🎄

fschwahn commented 3 years ago

awesome, thanks!