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

Fix delete all to not produce sql in statement #14546

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ def delete(*records)
dependent = _options[:dependent] || options[:dependent]

if records.first == :all
if loaded? || dependent == :destroy
if (loaded? || dependent == :destroy) && dependent != :delete_all
Copy link
Member

Choose a reason for hiding this comment

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

This conditional is weird. Should not it be:

if (loaded? && dependent != :delete_all) || dependent == :destroy

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. @rafaelfranca the both read weird to me. 😅 If you have strong feelings about the conditional, we can change.

Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings, it is just because if dependent is :destroy it is clearly != :delete_all. But we can merge as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can go either way, I don' have strong feelings towards either.

delete_or_destroy(load_target, dependent)
else
delete_records(:all, dependent)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
require 'models/categorization'
require 'models/minivan'
require 'models/speedometer'
require 'models/reference'
require 'models/job'

class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCase
fixtures :authors, :posts, :comments
Expand All @@ -39,7 +41,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
fixtures :accounts, :categories, :companies, :developers, :projects,
:developers_projects, :topics, :authors, :comments,
:people, :posts, :readers, :taggings, :cars, :essays,
:categorizations
:categorizations, :jobs

def setup
Client.destroyed_client_ids.clear
Expand Down Expand Up @@ -107,6 +109,19 @@ def test_do_not_call_callbacks_for_delete_all
assert_equal 0, Bulb.count, "bulbs should have been deleted using :delete_all strategy"
end

def test_delete_all_on_association_is_the_same_as_not_loaded
author = authors :david
author.thinking_posts.create!(:body => "test")
author.reload
expected_sql = capture_sql { author.thinking_posts.delete_all }

author.thinking_posts.create!(:body => "test")
author.reload
author.thinking_posts.inspect
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the inspect?

Copy link
Member

Choose a reason for hiding this comment

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

@rafaelfranca we need either the inspect or a to_a. The problem occurs when using irb, so I had @eileencodes use inspect.

Copy link
Member

Choose a reason for hiding this comment

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

👍

loaded_sql = capture_sql { author.thinking_posts.delete_all }
assert_equal(expected_sql, loaded_sql)
end

def test_building_the_associated_object_with_implicit_sti_base_class
firm = DependentFirm.new
company = firm.companies.build
Expand Down
6 changes: 6 additions & 0 deletions activerecord/test/cases/test_case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ def assert_date_from_db(expected, actual, message = nil)
end
end

def capture_sql
Copy link
Member

Choose a reason for hiding this comment

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

Can we change assert_sql to use this new method?

Copy link
Member

Choose a reason for hiding this comment

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

We should, but this PR is just to fix a bug. We should have a different PR for refactoring. :)

Copy link
Member

Choose a reason for hiding this comment

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

If it is in a different commit I guess it is fine. Either way is fine to me

Copy link
Member

Choose a reason for hiding this comment

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

@eileencodes can you refactor assert_sql to use capture_sql and send another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing! Actually threw it in there right before the merge request and a bunch of tests failed which I thought was odd, so I need to look at it some more.

I'll get everything working and send another PR.

SQLCounter.clear_log
yield
SQLCounter.log_all.dup
end

def assert_sql(*patterns_to_match)
SQLCounter.clear_log
yield
Expand Down