Revert hm:t on unsaved collection for 3.2 #8895

Merged
merged 1 commit into from Jan 11, 2013

Conversation

Projects
None yet
7 participants
Contributor

ernie commented Jan 11, 2013

It would appear that #7661 had unintended consequences for the documented API. Until
we can sort those out, this should not be in 3.2.x, and wait for 4.0.0. To be clear, I still think it's "the right thing" but it wasn't really a point-release kind of right thing.

@steveklabnik, do you agree?

Owner

rafaelfranca commented Jan 11, 2013

I agree. Since it was already release the correct thing to do is to add a CHANGELOG entry for the revert instead of remove the entry for the previous change.

Owner

rafaelfranca commented Jan 11, 2013

And thank you so much to work on this.

Contributor

ernie commented Jan 11, 2013

OK, will update.

@ernie ernie Revert "Merge pull request #7661 from ernie/build-join-records-on-uns…
…aved-hmt"

This reverts commit ee43989.

It would appear that #7661 had unintended consequences to the API. Until
we can sort those out, this should not be in 3.2.x, and wait for 4.0.0.
18b9187
Contributor

ernie commented Jan 11, 2013

rafaelfranca merged commit 2c5e0ad into rails:3-2-stable Jan 11, 2013

Owner

rafaelfranca commented Jan 11, 2013

❤️ 💚 💙 💛 💜

ernie deleted the unknown repository branch Jan 11, 2013

@ernie Maybe I haven't seen the full background to this issue but I am concerned about your proposal for 4.0 with regards to this. As an example, let's assume we have User, Team and TeamUser models. The TeamUser has belongs_to relationship both a user and a team and both the user & team models are linked to each other through the team user model. When we create a new User we want to assign it to a number of teams so we do this:

user = User.new
user.teams << Team.find_by_name("Admin")
user.save

Your proposal means that the TeamUser object would be created at line two before the user exists which means the user_id column of the TeamUser object would be nil which is not valid in this scenario.

When assigning objects in this manner before the object is actually created, I believe the current behaviour of creating these "join" objects after the original object has been saved is correct.

In my specific use case, I often add a validation on the join model to ensure that both parts of the association belong to the same account (in a multi-tenanted system). For example:

class TeamUser < ActiveRecord::Base
  belongs_to :team
  belongs_to :user
  validate do
    if team.account_id != user.account_id
      errors.add :base, "The team & user must belong to the same account"
    end
  end
end

Do you have any thoughts on this and how this would be achieved if the model initialised, validated & saved without one of the foreign keys?

Also, when is 3.2.12 due out?

Would it, perhaps, be worth releasing this sooner rather than later to avoid confusion by people upgrading to 3.2.11 because of the recently security announcements? I reckon there will be quite a few people upgrading right now (and in the coming weeks) and wondering about these changes and becoming confused due to the lack of documentation.

tgaff commented Jan 14, 2013

+1 for 3.2.12 release with this change. Based on the quantity of reported issues and the sudden need to update presented by the security patch I suspect this affects many users.

I held off on updates from 3.2.8 after this change but the security patch has forced the issue. I'm moving to this commit and hoping for 3.2.12 soon.

Toady00 commented Feb 1, 2013

+1 for 3.2.12 release sooner rather than latter. We've been bitten by this same issue when forced to update for security reasons.

+1 this caused some issues on our end after the 3.2.11 upgrade also.

Owner

rafaelfranca commented Feb 1, 2013

You always can use the git version. http://gembundler.com/git.html

Contributor

dgm commented Feb 16, 2013

+1 for official release. This regression caused a number of forms to fail when we were forced to install security upgrade releases.

class PhoneNumber < ActiveRecord::Base
  has_many :personal_phone_numbers, :dependent => :destroy
  has_many :people, :through => :personal_phone_numbers
end

class Person < ActiveRecord::Base
  has_many :personal_phone_numbers, :dependent => :destroy
  has_many :phone_numbers, :through => :personal_phone_numbers
end

class PersonalPhoneNumber < ActiveRecord::Base
  belongs_to :person
  belongs_to :phone_number

  validates :person_id, :existence => true, :presence => true
  validates :phone_number_id, :existence => true, :presence => true
end

> p = Person.new 
> pn = PhoneNumber.new(:phone_number => '555-5555', :phone_type => 'Home')
> p.phone_numbers << pn
> p.valid?
=> false

I had several models set up like this that fail to save properly now. Current 3-2-stable branch works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment