Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate using class level querying methods if the receiver scope regarded as leaked #35280

Merged
merged 2 commits into from Feb 15, 2019

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Feb 15, 2019

This deprecates using class level querying methods if the receiver scope
regarded as leaked, since #32380 and #35186 may cause that silently
leaking information when people upgrade the app.

We need deprecation first before making those.

…on_relation_create"

This reverts commit b67d5c6, reversing
changes made to 2e01836.

Reason: rails#35186 may cause that silently leaking information when people
upgrade the app.

We need deprecation first before making this.
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricky situation. Great fix!

@matthewd
Copy link
Member

What are the non-"leak" ways a scope can be set after this? Is it only explicit .scoping { .. }?

@kamipo
Copy link
Member Author

kamipo commented Feb 15, 2019

Yes, since .scoping { .. } is public API, I leaved the way as to inject the scope at global.

@kamipo
Copy link
Member Author

kamipo commented Feb 15, 2019

Maybe I don't get the point of "What are the non-"leak" ways a scope can be set after this?".

Can you expand about your concerned situation? @matthewd

ActiveSupport::Deprecation.warn(<<~MSG.squish)
Class level methods will no longer inherit scoping from the scoped relation 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 `#{name}.unscoped`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded.

@kamipo kamipo force-pushed the deprecate_leaking_scope branch 2 times, most recently from 18f0ca7 to 8a9e02c Compare February 15, 2019 08:30
…garded as leaked

This deprecates using class level querying methods if the receiver scope
regarded as leaked, since rails#32380 and rails#35186 may cause that silently
leaking information when people upgrade the app.

We need deprecation first before making those.
@kamipo kamipo merged commit b414ca3 into rails:master Feb 15, 2019
@kamipo kamipo deleted the deprecate_leaking_scope branch February 15, 2019 09:52
@kamipo kamipo restored the deprecate_leaking_scope branch February 15, 2019 09:52
@kamipo kamipo deleted the deprecate_leaking_scope branch February 15, 2019 09:52
@kamipo kamipo restored the deprecate_leaking_scope branch February 15, 2019 09:52
kamipo added a commit to kamipo/rails that referenced this pull request Oct 4, 2019
Follow up of rails#35280, I missed that `AssociationRelation` is using
`scoping` and not use `super`.
kamipo added a commit to kamipo/rails that referenced this pull request Oct 21, 2019
@sgrif
Copy link
Contributor

sgrif commented Nov 12, 2019

The code from the documentation for find_or_create_by now triggers this deprecation warning:

User.create_with(last_name: 'Johansson').find_or_create_by(first_name: 'Scarlett')

It seems like this deprecation is either too aggressive, or create_with is now useless and should be deprecated?

@kamipo
Copy link
Member Author

kamipo commented Nov 13, 2019

We have some tests for find_or_create_by, but it won't trigger any deprecation warning.

def test_find_or_create_by
assert_nil Bird.find_by(name: "bob")
bird = Bird.find_or_create_by(name: "bob")
assert_predicate bird, :persisted?
assert_equal bird, Bird.find_or_create_by(name: "bob")
end

def test_find_or_create_by_with_create_with
assert_nil Bird.find_by(name: "bob")
bird = Bird.create_with(color: "green").find_or_create_by(name: "bob")
assert_predicate bird, :persisted?
assert_equal "green", bird.color
assert_equal bird, Bird.create_with(color: "blue").find_or_create_by(name: "bob")
end

Let me know more executable information I can take a closer look at an issue around this deprecation.

@nbulaj
Copy link

nbulaj commented Nov 14, 2019

It looks really aggressive 🤔 We have a models that generates slug on creation using such concern:

module Sluggable
  extend ActiveSupport::Concern

  included do
    before_create :set_slug

    def generate_slug(payload)
      slug = payload.to_slug

      if self.class.where(slug: slug).exists? # <------ ¯\_(ツ)_/¯
        "#{slug}_#{SecureRandom.hex(6)}"
      else
        slug
      end
    end

    def sluggable_value
      name
    end

    private

    def set_slug
      self.slug = generate_slug(sluggable_value) if slug.blank?
    end
  end
end

And then I got a lot of deprecation warnings on the line marked above with a comment. Don't sure which scoping self.class.where().exists? may use, BTW it would be great to hear your feedback @kamipo . When adding .unscoped to self.class - it resolves the issue, but breaks our business that tied to paranoid behavior. See the script that tries to reproduce the issue

NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 3, 2020
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 3, 2020
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 3, 2020
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 7, 2020
Fixes the following deprecation warning in MiqGroup:

  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 `MiqGroup.default_scoped`.

  (called from next_sequence at /Users/nicklamuro/code/redhat/manageiq/app/models/miq_group.rb:72)

There was a change coming that will prevent leaking scopes in Rails 6.1,
and causes a deprecation warning as part of Rails 6.0:

- rails/rails#35280
- rails/rails#37727

Since our use case probably benefited from the leaking scope, we needed
to support that functionality when it make sense, otherwise default to
the .default_scope (aka: self) when appropriate.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 7, 2020
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 7, 2020
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 7, 2020
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Fixes the following deprecation warning in MiqGroup:

  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 `MiqGroup.default_scoped`.

  (called from next_sequence at /Users/nicklamuro/code/redhat/manageiq/app/models/miq_group.rb:72)

There was a change coming that will prevent leaking scopes in Rails 6.1,
and causes a deprecation warning as part of Rails 6.0:

- rails/rails#35280
- rails/rails#37727

Since our use case probably benefited from the leaking scope, we needed
to support that functionality when it make sense, otherwise default to
the .default_scope (aka: self) when appropriate.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 8, 2020
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 10, 2020
Fixes the following deprecation warning in MiqGroup:

  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 `MiqGroup.default_scoped`.

  (called from next_sequence at /Users/nicklamuro/code/redhat/manageiq/app/models/miq_group.rb:72)

There was a change coming that will prevent leaking scopes in Rails 6.1,
and causes a deprecation warning as part of Rails 6.0:

- rails/rails#35280
- rails/rails#37727

Since our use case probably benefited from the leaking scope, we needed
to support that functionality when it make sense, otherwise default to
the .default_scope (aka: self) when appropriate.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 10, 2020
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 10, 2020
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Dec 10, 2020
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jan 5, 2021
Fixes the following deprecation warning in MiqGroup:

  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 `MiqGroup.default_scoped`.

  (called from next_sequence at /Users/nicklamuro/code/redhat/manageiq/app/models/miq_group.rb:72)

There was a change coming that will prevent leaking scopes in Rails 6.1,
and causes a deprecation warning as part of Rails 6.0:

- rails/rails#35280
- rails/rails#37727

Since our use case probably benefited from the leaking scope, we needed
to support that functionality when it make sense, otherwise default to
the .default_scope (aka: self) when appropriate.
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jan 5, 2021
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jan 5, 2021
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jan 5, 2021
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
kbrock pushed a commit to kbrock/manageiq that referenced this pull request Jan 27, 2021
Fixes the following deprecation warning in MiqGroup:

  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 `MiqGroup.default_scoped`.

  (called from next_sequence at /Users/nicklamuro/code/redhat/manageiq/app/models/miq_group.rb:72)

There was a change coming that will prevent leaking scopes in Rails 6.1,
and causes a deprecation warning as part of Rails 6.0:

- rails/rails#35280
- rails/rails#37727

Since our use case probably benefited from the leaking scope, we needed
to support that functionality when it make sense, otherwise default to
the .default_scope (aka: self) when appropriate.
kbrock pushed a commit to kbrock/manageiq that referenced this pull request Jan 27, 2021
Adds a fix for the following deprecation error:

  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 `MiqGroup.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/lib/unique_within_region_validator.rb:25)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
kbrock pushed a commit to kbrock/manageiq that referenced this pull request Jan 27, 2021
Similar to previous commits, this fixes the following deprecation error
that was propagating when running `task_helpers/imports/tags_spec.rb`

  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 `Classification.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/app/models/classification.rb:512)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727
kbrock pushed a commit to kbrock/manageiq that referenced this pull request Jan 27, 2021
Adds a fix for the following deprecation error:

  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 `Vm.default_scoped`.

  (called from validate_each at /Users/nicklamuro/code/redhat/manageiq/spec/lib/rbac/filterer_spec.rb:2086)

This was added to fix "leaking scopes" in Rails 6.1, but was added as a
deprecation warning first:

- rails/rails#35280
- rails/rails#37727

This specifically was just a fake `Vm` model specifically used for the Rbac
specs, so this isn't something that needs to be fixed in the app codebase
proper.
alexdunae added a commit to culturecode/spatial_features that referenced this pull request Jun 3, 2021
alexdunae added a commit to culturecode/spatial_features that referenced this pull request Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants