Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Unexpected behaviour of ActiveRecord unvalidated saves with associations #6659

Closed
josephlord opened this Issue · 13 comments

4 participants

Joseph Lord Steve Klabnik Matt Jones Rafael Mendonça França
Joseph Lord

I'm not certain that this is a bug as such but I certainly found the behaviour unexpected in two ways.

1) Errors not raised when I would have expected them.
2) valid? has side effects.

I have some tests that show the behaviour which although it is not my expectation that they will actually be pulled into the main project. See: https://github.com/josephlord/rails/pull/new/unvalidated_association_save_test_for_pull

Essentially the behaviour that I have found is that I get unexpected behaviour in the following scenario.

Steps to reproduce:
1) Assign object to belongs_to association
2) Assign invalid value to the _id of the association
4) Call save(validated: false) - No error is raised. Record is saved incorrectly

Step 3 is to call valid? on the object which prevents the issue and causes the ActiveRecord::InvalidForeignKey to be triggered in step 4.

I know that this is an edge case. I came across it while trying to develop tests to ensure that the database validation was occuring in addition to the Rails validations. It may also be the behaviour that is expected by those who know Rails better than I do.

This test passes but I think that this behaviour is wrong:

class House < ActiveRecord::Base
  has_many :doors
end

class Door < ActiveRecord::Base
  belongs_to :house
  validates :house, presence: true
end

#Foreign key constraints are declared in the schema. See https://github.com/josephlord/rails/pull/new/unvalidated_association_save_test_for_pull

  def test_save_validate_false_after_association_created_bad_behaviour
    # This passes but I think it shouldn't.  
    # The intention is to illustrate the current behaviour rather than desired behaviour
    door  = Door.new
    houseA = door.house = House.new # This makes the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    door.save(validate: false)
    door.reload
    houseA.reload
    assert_not_equal bad_house_id, door.house_id
    assert_equal houseA.id, door.house_id
    assert_raises(ActiveRecord::RecordNotFound) { House.find bad_house_id }
    assert_equal house_count + 1, House.count
  end
Joseph Lord

I should have noted that I have only tested this with Postgres but the issue is I think more on the ActiveRecord side so I would suspect that and DB supporting foreign key constraints to be affected.

Steve Klabnik
Collaborator

Call save(validated: false) - No error is raised. Record is saved incorrectly

This very much seems like a bug to me.

EDIT: Unless you mean call save without validations, in which case, I wouldn't expect an error.

/cc @jonleighton @tenderlove

Joseph Lord

@steveklabnik

It has been a while since I reported this. While it is saving without validations I expect the Foreign Key constraint in the database to throw an error (I actually came across the issue developing tests that were skipping Rails validation to ensure the DB constraints were also in place). It doesn't because "door.house_id = bad_house_id" is basically ignored until validation and the previously assigned.

The full set of tests in the pull request includes 5 tests. The first two pass (correctly I believe), the third as a minor modifications to the passing tests that make it fail (when I believe that they should pass), the fourth passes due to the side effect of the 'valid?' call (I think it should pass but the reason shouldn't be the side effect) and the fifth is the same as shown above with a comment indicating that it currently passes but should not. NOTE that I tested against the latest Rails at the time of bug submission but I haven't retested since.

https://github.com/josephlord/rails/pull/new/unvalidated_association_save_test_for_pull

Steve Klabnik
Collaborator

Ah, I don't ever use foreign key constraints in the DB, so that's what I'm missing here.

Matt Jones

This doesn't look like a FK issue at all - the real underlying issue appears nearly identical to #5744 and #5563, where belongs_to was preferring the object that it had over the directly-set _id column.

Joseph Lord

The FK was just the mechanism for testing the write to the DB really. You are probably right about the core bug though it does seem very similar but I don't know if they covered the unvalidated save case or the side effects of validation.

I'm not going to have chance to check if those other changes fix these situations for at least a few days. I am reassured that the behaviour is considered wrong though.

Rafael Mendonça França
Owner

I think this issue is fixed in master at 365b8b6. Could you investigate?

Joseph Lord

I don't believe it is fixed. I've merged from upstream Master and I'm still having: test_save_validate_false_after_association_created fail.

I wanted to follow up quickly while my branch is still up to date. I haven't yet looked at the details of the fix you reference but I suspect it my fix the issue for save but not for validation having a side effect. I will try to look at that in the next couple of hours.

I've created a branch in my repo that is currently a clean change to the current master. https://github.com/josephlord/rails/tree/fullUpdateClean

I'm still hitting errors

  def test_save_validate_false_after_association_created
    # This fails due to the association having been set before the id is set to something invalid
    door  = Door.new
    door.house = House.new # This makes the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }  ### DOES NOT RAISE
    assert_raise(ActiveRecord::RecordNotFound) { door.reload }
    assert_equal bad_house_id, door.house_id
    assert_raises(ActiveRecord::RecordNotFound) { House.find bad_house_id }
    assert_equal house_count, House.count
  end

And this test does pass but I think it shouldn't.

  def test_save_validate_false_after_association_created_bad_behaviour
    # This passes but I think it shouldn't.  
    # The intention is to illustrate the current behaviour rather than desired behaviour
    door  = Door.new
    houseA = door.house = House.new # This makes the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    door.save(validate: false)
    door.reload
    houseA.reload
    assert_not_equal bad_house_id, door.house_id
    assert_equal houseA.id, door.house_id
    assert_raises(ActiveRecord::RecordNotFound) { House.find bad_house_id }
    assert_equal house_count + 1, House.count
  end
Joseph Lord

Problem definitely still exists (assuming it is a problem - it is certainly behaviour that was unexpected to me). If the ID is changed the association is only updated when accessed (of which validation is a subset of the issue).

The unvalidated save somehow retrieves the association set before the id is changed.

I would have imagined that the safest way to fix this would be to nil the association reference when the id itself is set rather than relying on all retrieval accesses to do so correctly.

I try below to make clearer what is happening and the lines that are changing the results. I've also trimmed off the additional assertions which while still correct in my view are not needed to present this issue.

I'm going to try to step into the activerecord handling of id assignment and try to fix there unless anyone advises against.

def test_save_validate_false_after_valid_test
    # This passess
    door  = Door.new
    # door.house = House.new # This would make the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }
  end

  def test_save_validate_false_after_association_created
    # This fails due to the association having been set before the id is set to something invalid
    door  = Door.new
    door.house = House.new # This makes the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    # door.house # This would make it pass even with the above
    # door.valid? # This would also make it pass even with the above
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }  # Fails here

  end

  def test_save_validate_false_after_association_created_touched
    # This passes because the touch on door.house cleans the stale reference
    door  = Door.new
    door.house = House.new
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    door.house # This makes it pass
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }
  end

  def test_save_validate_false_after_valid_test_after_association_created
    # This passess as the valid? call has a useful side effect
    door  = Door.new
    door.house = House.new # This would make the test fail if it weren't for the valid test'
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    door.valid? # The valid? call means that the tests pass.
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }
  end
Joseph Lord

The updated id is overwritten in this function found in activerecord/lib/active_record/autosave_association.rb:

    def save_belongs_to_association(reflection)
      association = association_instance_get(reflection.name)
      record      = association && association.load_target
      if record && !record.destroyed?
        autosave = reflection.options[:autosave]

        if autosave && record.marked_for_destruction?
          self[reflection.foreign_key] = nil
          record.destroy
        elsif autosave != false
          saved = record.save(:validate => !autosave) if record.new_record? || (autosave && record.changed_for_autosave?)

          if association.updated?
            association_id = record.send(reflection.options[:primary_key] || :id)
            self[reflection.foreign_key] = association_id
            association.loaded!
          end

          saved if autosave
        end
      end
    end

Specifically the association is regarded as being updated which leads to this specific line being called:

self[reflection.foreign_key] = association_id

It appears that autosave triggers validated saves on associations even when the requested save is unvalidated. I don't know if that is another issue in itself. My next step will be to try to work backwards to work out the best place to intercept. I do note there are other save_[association_type]_association methods and they may all have the same issue so if it can be intercepted earlier it might be a smaller change to fix this.

Joseph Lord

All my attempts so far to fix this have caused other errors and failures throughout the test suite. I'm not going to spend further time trying but I believe that this issue is real (although minor).

The cleanest fix (although not the smallest and I haven't attempted it) might be for a specific setter to be generated for the _id of an association which then cleans the association cache and as appropriate marks the previous association for deletion etc.

Joseph Lord

Not sure why this was referenced from PR # 12480 as I don't see the relevance.

I've just checked whether the test code still gets the same results against Rails master and it does. Note that all my testing has been with Postgresql.

See: josephlord#1 for the test code I add or https://github.com/josephlord/rails/tree/updatedTestBranch for the fork of latest master with the testcode added. (Note that it includes one test `test_save_validate_false_after_association_created_bad_behaviour' that currently passes but should fail on correct behaviour. It is implemented to describe current rather than desired behavior so should be removed before any merge into the main tree.)

I will create a Pull request with my changes except for the test that describes the current (incorrect) behavior and link it to this issue. It will obviously add failures to the testing system.

The comments in the following code describe the behavior. This test fails currently and either of the two commented out lines will make it pass if enabled but that does not seem like correct behavior to me still.

  def test_save_validate_false_after_association_created
    # This fails due to the association having been set before the id is set to something invalid
    door  = Door.new
    door.house = House.new # This makes the test fail below!!!
    bad_house_id = House.order(:id).last.id + 10000
    house_count = House.count
    door.house_id = bad_house_id
    # door.house # This would make it pass even with the above
    # door.valid? # This would also make it pass even with the above
    assert_raise(ActiveRecord::InvalidForeignKey) { door.save(validate: false) }  # Fails here
    assert_raise(ActiveRecord::RecordNotFound) { door.reload }
    assert_equal bad_house_id, door.house_id
    assert_raises(ActiveRecord::RecordNotFound) { House.find bad_house_id }
    assert_equal house_count, House.count
  end
Rafael Mendonça França

Please see #12525 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.