search

brendon / acts_as_list

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

Silence deprecation warning in rails 6 #363

Closed h-lame closed 4 years ago

h-lame commented 5 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>.unscoped`.
(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 to use the full unscoped method instead of the restricted unscope(:select, :where).

No other tests seem to break with this change, so I assume fully unscoping isn't going to cause any problems elsewhere in how acts_as_list works.

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.

brendon commented 5 years ago

Hi @h-lame, thanks for this PR :)

We added restricted scoping a while back to counter problems in user-end code (which obviously has no tests as you've demonstrated :D).

The issue is that fully unscoping can break expectations around ordering if my memory serves me rightly. These seem to be the related issues:

Is there another way around this issue? :)

h-lame commented 5 years ago

Hi @brendon,

On our app it seems like we only got the deprecation warning when we're using find_or_create_by on one of our acts_as_list models. I'll dig into find_or_create_by and see if there's a different hook for hanging acts_as_list off.

I think what the dep warning is telling us is that in rails 6.1 you will effectively be getting a "free" unscoped call in these cases whether you want it or not. But the major problem is that last time I checked, rails master didn't have the new implementation that would break instead of raising a dep warnings, so it's hard to check what might break once it's not deprecated anymore.

I'll see if I can whip up some more test cases.

brendon commented 5 years ago

Thanks @h-lame. Depending on what you find, it might be a case of raising this with the Rails team also if they've not thought of an edge case like this :)

ojab commented 4 years ago

I've created https://github.com/rails/rails/issues/38303 about it.

brendon commented 4 years ago

Can someone give me an update on this one?

h-lame commented 4 years ago

@brendon - looks to me that the rails issue mentioned by @kamipo has been merged, so for rails 6.1 acts_as_list should use default_scoped to avoid the dep warning. I could redo this PR to achieve that. The default_scoped method exists (albeit as an undocumented "private" API) at least as far back as rails 5.0. In theory we only need it to use it for rails 6.x to avoid the dep warning so we could do a version check and use default_scoped if we're on rails 6.x but remain with unscope(:select, :where) for other rails version. However, as it has existed for a while, and the implementation seems pretty constant throughough its lifespan we could avoid doing any version checks and just use it regardless of the rails version.

Thoughts?

h-lame commented 4 years ago

Looks like it's also present in rails 4.2, and that's currently the minor version of AR that acts_as_list requires so we could use it and not change the supported versions of rails.

I guess the question is the same as the one for the original version of the PR, how confident would we be in the test suite telling us if this change broke anything?

brendon commented 4 years ago

Thanks @h-lame :) I wouldn't be super confident that the test suite will catch new problems around the unscoping (though I can't remember offhand how much we're testing this).

Your idea sounds good provided the implementation of default_scoped hasn't changed from 4.2 until now. Did that look to be the case?

Testing-wise, we're just wanting to make sure other things like order aren't stripped when unscoping :)

sudara commented 4 years ago

I'd love to help resolve.

Your idea sounds good provided the implementation of default_scoped hasn't changed from 4.2 until now. Did that look to be the case?

default_scoped has changed a couple times over the years, most recently to fix some issues with subclasses.

Here are some comparisons of the method that builds it:

https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/scoping/default.rb#L97-L109 https://github.com/rails/rails/blob/5-0-stable/activerecord/lib/active_record/scoping/default.rb#L104-L127 https://github.com/rails/rails/blob/master/activerecord/lib/active_record/scoping/default.rb#L102-L124

Testing-wise, we're just wanting to make sure other things like order aren't stripped when unscoping :)

It sounds like the next steps are to use default_scoped.unscope and add a test or two for unscoping so all the change feels safe on all supported rails versions. @h-lame would you want to proceed with that on this PR? I'm also happy to open a new PR.

h-lame commented 4 years ago

@sudara - looks like the fork I made this PR on has been deleted, but it should be possible to resurrect it. I can easily add the default_scoped call. Not entirely sure about the version check on default_scoped, does the gem already do some rails/ruby version toggling? What's the best approach here? Also not entirely sure what testing is missing.

If you're motivated to get this fixed, I have no objections to you picking these changes up and making a new PR, don't feel like you'd be stepping on my toes. I freely admit to being a bit lost on how best to support this change with decent tests that give us confidence that the change isn't breaking anything, which is why this has lingered for .... a year 🤦.

brendon commented 4 years ago

We already do some version checking so this wouldn't be too much of a problem. If it's more pragmatic to version check for 6+ then let's do that. We unscope in a few places from memory so that could potentially be abstracted in to a method to keep the checking code in one place?

h-lame commented 4 years ago

Turns out, having deleted the fork for I couldn't update this PR even after I recreated the fork. I've created a new PR to incorporate the changes suggested by @kamipo in https://github.com/brendon/acts_as_list/pull/363#discussion_r371060761 and the version check stuff suggested by @brendhon in https://github.com/brendon/acts_as_list/pull/363#issuecomment-695841514.

See #384 for the new version, sorry for the noise! Closing this in favour of the new one.