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

Opting out of strict loading on a per-record base #41181

Merged
merged 1 commit into from Jan 21, 2021

Conversation

ayrton
Copy link
Contributor

@ayrton ayrton commented Jan 19, 2021

Summary

Provide a way to opt out of strict loader on a per-record base. This is useful when strict loading is enabled application wide or on model level.

How:

Similar to defining the value of strict_loading on relations we accept an optional value on a record.

This was initially reported in #41171.

@ayrton
Copy link
Contributor Author

ayrton commented Jan 19, 2021

Would love to hear if we should ship with lazy loaded records, when serialization happens through globalid, to avoid having to write:

record.strict_loading!(false)

in your jobs:

module MyApplication
  class Application < Rails::Application
    config.active_record.strict_loading_by_default = true
  end
end

class Subscription < ApplicationRecord
  belongs_to :user
end

class User < ApplicationRecord
  has_many :subscriptions, inverse_of: :user
end

class ArchiveSubscriptionJob < ApplicationJob
  def perform(subscription)
    subscription.strict_loading!(false) # <-- should this be done out-of-the-box?

    user = subscription.user
    # [...]
  end
end

If so I am happy to draft up the code (either in this PR or a different one). Something along the lines of:

# activejob/lib/active_job/arguments.rb
def deserialize_global_id(hash)
  GlobalID::Locator.locate(hash[GLOBALID_KEY]).tap do |record|
    record.strict_loading!(false) if record.respond_to?(:strict_loading!)
  end
end

@ayrton ayrton marked this pull request as ready for review January 20, 2021 08:38
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Can you also add a changelog entry?

I'm not sure about the global id issue. You can open a separate PR for discussion after this one is merged.

@@ -24,6 +24,9 @@ def test_strict_loading!
assert_raises ActiveRecord::StrictLoadingViolationError do
developer.audit_logs.to_a
end

developer.strict_loading!(false)
assert_not_predicate developer, :strict_loading?
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an assertion that developer.audit_logs doesn't raise? I think we should also have a test showing that it also works if the strict loading is defined in the relation. See strict_loading_audit_logs below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8223023

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Can you squash your commits into 1? Sorry I should have mentioned when I reviewed the first time!

This is useful when strict loading is enabled application wide or on a
model level.
@ayrton ayrton force-pushed the activerecord-strict-loading-optout branch from 8223023 to 2041279 Compare January 20, 2021 14:42
@ayrton
Copy link
Contributor Author

ayrton commented Jan 20, 2021

Can you squash your commits into 1? Sorry I should have mentioned when I reviewed the first time!

No problem, just finished rebasing and squashing the commits into one

@eileencodes eileencodes merged commit 32e5d55 into rails:main Jan 21, 2021
@eileencodes
Copy link
Member

Thanks for working on this!

@ayrton ayrton deleted the activerecord-strict-loading-optout branch January 21, 2021 19:47
# => ActiveRecord::StrictLoadingViolationError

user = User.first
user.stict_loading!(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

user.strict_loading!(false)

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

3 participants