Silence deprecation warning in rails 6 [redo]

[h-lame]

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:

• Silence deprecation warning in rails 6 #363 (comment) (use default_scoped)
• Silence deprecation warning in rails 6 #363 (comment) (do a version check on using default_scoped.unscoped)

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

[sudara]

Thanks for re-opening!

I went through the links to prior issues @brendon listed here: #363 (comment)

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]

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

• acts_as_list_class.where(scope_condition) - I get failures from the test suite - both functional about elements being in the wrong order or missing, and the non-functional ones I added in this PR about deprecation warnings.
• acts_as_list_class.default_scoped.where(scope_condition) - I get the functional failures but not the deprecation warning ones.
• acts_as_list_class.unscoped - I don't get any failures.
• acts_as_list_class.unscope(:select, :where).where(scope_conditions) - (e.g. original implementation) I get the deprecation warning failures.
• acts_as_list_class.unscoped.default_scoped - I get the functional failures only (which, as I think about it, doesn't surprise me as Model.default_scoped and Model.unscoped.default_scoped should be equivalent pieces of code).

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]

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]

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]

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]

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]

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: rails/rails#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]

What about:

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

?

[h-lame]

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]

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]

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]

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]

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]

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]

@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]

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

[h-lame]

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]

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

[fschwahn]

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

[brendon]

Merry Christmas @fschwahn :) 🎄

[fschwahn]

awesome, thanks!