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.

Sign up for GitHub

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

Jump to bottom

Fix bug, when ':dependent => :destroy' violates foreign key constraints #12450

Merged
merged 1 commit into from Jun 27, 2014
Merged

Fix bug, when ':dependent => :destroy' violates foreign key constraints #12450

merged 1 commit into from Jun 27, 2014

Conversation

iantropov
Copy link
Contributor

Fix issue, when ':dependent => :destroy' option violates foreign key constraints, issue #12380

@iantropov iantropov mentioned this pull request Oct 5, 2013
Closed
pftg
pftg reviewed Oct 5, 2013
View reviewed changes
activerecord/CHANGELOG.md
@@ -752,4 +752,11 @@

*Slava Markevich*

* Fixed a bug in `ActiveRecord::Associations::Builder` for case when `:dependent => :destroy' option
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move Changelog entry to the first line

@pftg
Copy link
Contributor

pftg commented Oct 5, 2013

Please use code style from: http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#write-your-code

pftg
pftg reviewed Oct 5, 2013
View reviewed changes
activerecord/test/cases/associations/belongs_to_associations_test.rb
errors = []
begin
player.destroy!
rescue ActiveRecord::InvalidForeignKey => e
Copy link
Contributor

Choose a reason for hiding this comment

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

You should try assert_nothing_raised

@iantropov
Copy link
Contributor Author

Updated pull request according code review marks.

pftg
pftg reviewed Oct 5, 2013
View reviewed changes
activerecord/test/cases/associations/belongs_to_associations_test.rb
team = Team.create!
player = Player.create! :id => 1, :team_id => team.id

assert_nothing_raised { player.destroy! }
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but I think better to remove assert_nothing_raised (it is redudant) then your test will fail if destroy! is broken

@pftg
Copy link
Contributor

pftg commented Oct 5, 2013

Please use 1.9 hash style and re-indent private methods, which you have added/updated

@iantropov
Copy link
Contributor Author

@pftg
Sorry, but I can't found usage of new hash-style. Also, as I see, all my private methods have a indent between private and name of method.

So, I think it would be useful to have some style checker and run it before commiting

pftg
pftg reviewed Oct 5, 2013
View reviewed changes
activerecord/test/cases/associations/belongs_to_associations_test.rb

def teardown
ActiveRecord::Schema.define do
drop_table :players, :if_exists => true
Copy link
Contributor

Choose a reason for hiding this comment

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

will be great: drop_table :players, if_exists: true

@iantropov
Copy link
Contributor Author

Reindent methods and change hash-style

@al2o3cr
Copy link
Contributor

al2o3cr commented Oct 6, 2013

The CHANGELOG on this could be clearer - this PR moves dependent: :destroy handling for belongs_to from before_destroy to after_destroy.

@iantropov
Copy link
Contributor Author

Update CHANGELOG.md

@indrekj
Copy link
Contributor

indrekj commented Oct 10, 2013

👍

@iantropov
Copy link
Contributor Author

@rafaelfranca @tenderlove please review this request, because now, I have to do monkey patches on my production code for avoidance of this issue.

@iantropov iantropov mentioned this pull request Oct 10, 2013
Closed
robin850
robin850 reviewed Oct 10, 2013
View reviewed changes
activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -831,3 +831,39 @@ def test_reflect_the_most_recent_change
assert_equal post.author_id, author2.id
end
end

require 'models/team'
require 'models/player'
Copy link
Member

Choose a reason for hiding this comment

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

It's a nitpick but could you please move the require statements to the very top of the file to follow the style of the file ? Nice fix though, thank you! ❤️

@iantropov
Copy link
Contributor Author

@robin850
Update test file according your comment.

@iantropov
Copy link
Contributor Author

@robin850 @pftg
Please review changes after your comments

pftg
pftg reviewed Oct 11, 2013
View reviewed changes
activerecord/test/models/player.rb
@@ -0,0 +1,3 @@
class Player < ActiveRecord::Base
belongs_to :team, :dependent => :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be belongs_to :team, dependent: :destroy

@pftg
Copy link
Contributor

pftg commented Oct 11, 2013

👍

@iantropov
Copy link
Contributor Author

Updated test model for new Rails hash syntax

senny
senny reviewed Oct 14, 2013
View reviewed changes
activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -831,3 +833,36 @@ def test_reflect_the_most_recent_change
assert_equal post.author_id, author2.id
end
end

class BelongsToWithForeignKeyTest < ActiveRecord::TestCase

Copy link
Member

Choose a reason for hiding this comment

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

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@senny
Sorry, but I don`t understand what you want 😄 Do you offer to pick out this class to separate file ?

Copy link
Member

Choose a reason for hiding this comment

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

We often use the ✂️ to mark blank lines that should be removed:

class BelongsToWithForeignKeyTest < ActiveRecord::TestCase
  def setup
...

@iantropov
Copy link
Contributor Author

@senny
Fixed new lines

@senny
Copy link
Member

senny commented Oct 21, 2013

look good to me. @rafaelfranca can you take a look?

@rafaelfranca
Copy link
Member

This look good but I'd like to avoid add new models to the test suire. @iantropov could you change the tests to reuse the models already there.

If it is not possible let me know. Thanks

@iantropov
Copy link
Contributor Author

@rafaelfranca please take a look. I've get rid of additional test models (Player and Team) and have reused existed (Author and AuthorAddress).

@lostinpatterns
Copy link

what's the status of this?

@iantropov
Copy link
Contributor Author

We are waiting @rafaelfranca

@rafaelfranca rafaelfranca self-assigned this Feb 4, 2014
@bliaxiong
Copy link

any news?

@iantropov
Copy link
Contributor Author

We are still waiting @rafaelfranca to accept this PR.

@spastorino
Copy link
Contributor

@iantropov you would need to rebase your commit, it doesn't apply anymore

@rafaelfranca rafaelfranca modified the milestones: 4.0.5, 4.0.4, 4.0.6 Mar 10, 2014
@ncolgan
Copy link

ncolgan commented Jun 27, 2014

👍

@rails rails locked and limited conversation to collaborators Jun 27, 2014
@rafaelfranca rafaelfranca merged commit 87d1aba into rails:master Jun 27, 2014
rafaelfranca added a commit that referenced this pull request Jun 27, 2014
@rafaelfranca
Merge pull request #12450 from iantropov/master
b22edc6 
Fix bug, when ':dependent => :destroy' violates foreign key constraints

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/associations/builder/association.rb
	activerecord/lib/active_record/associations/builder/has_one.rb
@rafaelfranca
Copy link
Member

I don't feel confident enough to backport this commit to stable versions but it is on master. Since we have foreign keys support built in on rails now make sense to this behaviour just work.

@rafaelfranca rafaelfranca removed this from the 4.0.7 milestone Jun 27, 2014
ncolgan pushed a commit to ncolgan/rails that referenced this pull request Jun 27, 2014
@rafaelfranca
Merge pull request rails#12450 from iantropov/master
f7947a8 
Fix bug, when ':dependent => :destroy' violates foreign key constraints

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/associations/builder/association.rb
	activerecord/lib/active_record/associations/builder/has_one.rb

Conflicts:
	activerecord/CHANGELOG.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@rafaelfranca rafaelfranca

Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet