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

Disable strict loading on ActiveStorage::Attached::Changes #41461

Closed
wants to merge 3 commits into from

Conversation

Roriz
Copy link
Contributor

@Roriz Roriz commented Feb 16, 2021

Summary

When we try to attach a new file with strict_loading_by_default = true on the model, will always raise an ActiveRecord::StrictLoadingViolationError because of validation of active storage (https://github.com/rails/rails/blob/main/activestorage/lib/active_storage/attached/changes/create_one.rb#L43).
This validation is trigger always without loading any association (https://github.com/rails/rails/blob/main/activestorage/lib/active_storage/attached/model.rb#L65)

In past, we already disable strict_loading on validations(#40775), but active_storage still keeps this validation. This PR disable strict_loading on save of Attached::Changes::CreateOne

This PR solves the issue: #41453

Setup

rails new sl-with-as --database=postgresql --no-skip-active-storage
cd sl-with-as/
rails active_storage:install
rails generate model Example name:string
rails db:create && rails db:migrate
echo 'class Example < ActiveRecord::Base; self.strict_loading_by_default = true; has_one_attached :file; end' > app/models/example.rb
rails console
example = Example.create(name: 'Lorem')
example.file.attach({ io: File.new("tmp/temp1.txt"), filename: 'tempfile1' }) # ActiveRecord::StrictLoadingViolationError

@Roriz Roriz changed the title Patch 1 Disable strict loading on ActiveStorage::Attached::Changes Feb 16, 2021
Copy link

@andersonmarcelino andersonmarcelino left a comment

Choose a reason for hiding this comment

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

LGTM

eileencodes added a commit to eileencodes/rails that referenced this pull request Mar 16, 2021
Fixes: rails#41453
Closes: rails#41461

Previously when a model had strict loading set to true and then had a
relation set strict_loading to false the false wasn't considered when
deciding whether to raise/warn about strict loading.

Code example:

```ruby
class Dog < ActiveRecord::Base
  self.strict_loading_by_default = true

  has_many :treats, strict_loading: false
end
```

In the example, `dog.treats` would still raise even though
`strict_loading` was set to false. This is a bug effecting more than
Active Storage which is why I made this PR superceeding rails#41461. We need
to fix this for all applications since the behavior is a little
surprising. I took the test from #rails#41461 and the code suggestion from rails#41453
with some additions.

Co-authored-by: Radamés Roriz <radamesroriz@gmail.com>
@eileencodes
Copy link
Member

Hi @Roriz thanks for the PR and the issue - I combined your test changes here into the suggestion on the issue in #41688. This was a bug with any relation not just active storage. I made sure to add your authorship so you get credit for the change.

@Roriz Roriz deleted the patch-1 branch March 16, 2021 21:12
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

5 participants