Show reason for referential integrity failure

[danini-the-panini]

Summary

Currently, if there is a referential integrity error in your fixtures, it will throw an error. However, the original error message from postgres is swallowed in all_foreign_keys_valid? and returns false. This gives the developer very little information on where to start debugging.

My proposed solution is to raise the original exception from the underlying adapter for postgres, and raise a similar error from the SQLite adapter.

Other Information

Stack overflow explaining the issue: https://stackoverflow.com/questions/71054865/tips-for-debugging-fixture-foreign-key-errors-in-rails

[ghiculescu]

@danini-the-panini thanks for the PR and the stack overflow link. It's good feedback on the feature.

I think we could consider including the error message from the database in the error message that gets raised, rather than logging. This way it works for all databases and it's harder to miss the message.

To do this all_foreign_keys_valid? would need to return a string (and probably be renamed) and then that would get interpolated in here.

Is this something you'd be interested in working on? If not I am happy to make a PR.

[danini-the-panini]

@ghiculescu thank you for your feedback. I think I'm catching your drift here.

Is this something you'd be interested in working on? If not I am happy to make a PR.

Let me play around in this PR and see if I can get it into a place we can align on.

[danini-the-panini]

@ghiculescu I've changed it to bubble up the original error. Not sure what the best way would be to include the original hint to the developer? What do you think if we did the following in activerecord/lib/active_record/fixtures.rb?

if ActiveRecord.verify_foreign_keys_for_fixtures
  begin
    conn.check_all_foreign_keys_valid
  rescue ActiveRecord::StatementInvalid => error
    raise "Foreign key violations found in your fixture data. Ensure you aren't referring to labels that don't exist on associations\nCaused by\n#{error.message}"
  end
end

A more advanced approach would be to investigate the actual error from PG and give the developer the exact fixture files to look at.

[ghiculescu]

I like that for Postgres. Will the same approach work for SQLite?

[danini-the-panini]

Will have to look into the other adapters and see what's possible. I don't see why it wouldn't though.

[yahonda]

Would you address CI failures?

[danini-the-panini]

I like that for Postgres. Will the same approach work for SQLite?

I've managed to amend the SQLite adapter to raise a similar error.

[danini-the-panini]

Would you address CI failures?

I fixed the errors pertaining to my code changes but it now appears to be failing for reasons completely unrelated. Is it maybe since I rebased off main?

[ghiculescu]

Could you update the changelog?

[yahonda]

The current implementation looks cleaner, however it depends on the all_foreign_keys_valid? which is marked as duplicate.
Therefore it raises deprecation warnings and all_foreign_keys_valid? cannot be removed in the future versions of Rails.

Would you check_all_foreign_keys_valid not depending on all_foreign_keys_valid??

[danini-the-panini]

I don't see a way of doing this without breaking 3rd party adapters. An alternative would be to create a custom deprecator that we can "turn off" during the check_all_foreign_keys_valid method?

[danini-the-panini]

I've removed the dependency. This will mean that fixture integrity checks in 3rd party adapters that implement all_foreign_keys_valid? will no longer work until they override check_all_foreign_keys_valid instead.

[yahonda]

I'm not sure if I understand your concern yet, How about creating another private method _all_foreign_keys_valid??

def all_foreign_keys_valid?
    # Just call the private `_all_foreign_keys_valid` method
    _all_foreign_keys_valid?
end
deprecate :all_foreign_keys_valid?

def check_all_foreign_keys_valid
    # Making use of the private `_all_foreign_keys_valid?` method
    # so that it does not depend on the deprecated public `all_foreign_keys_valid?` method
end

private 
def _all_foreign_keys_valid?
    # implement the current `all_foreign_keys_valid?` method logic here
end

Then we can remove the public all_foreign_keys_valid? method in future.

[danini-the-panini]

Aha, good idea, thank you 🙏🏻

[danini-the-panini]

Wait, that makes no difference since there isn't any real "logic" in the abstract adapter, and this doesn't solve the issue with 3rd party abstract adapters, since they will still be overriding all_foreign_keys_valid?, which will no longer be used by the fixtures integrity check. Unless we just don't deprecate it until we find a way to remove it safely.

How do you deprecate a method for "overriding"? Unless we do some reflection to see if the current adapter has overridden all_foreign_keys_valid? and not check_all_foreign_keys_valid and raise a deprecation warning if that's the case. For example, something like this:

def all_foreign_keys_valid?
  true
end
deprecate :all_foreign_keys_valid?

def check_all_foreign_keys_valid
  return if self.class.method(:all_foreign_keys_valid?).owner == AbstractAdapter

  raise unless all_foreign_keys_valid?
end

[yahonda]

I am also a 3rd party adapter maintainer and there is no way for every third-party adapter to call super so we do not have to do something like return if self.class.method(:all_foreign_keys_valid?).owner == AbstractAdapter .
Here is one example rsim/oracle-enhanced#1770 Also we can notify by adding mentions to some of 3rd party adapter maintainers if necessary.

[danini-the-panini]

Okay, in that case the current implementation should be sufficient

[dorianmariecom]

Maybe you don't need to deprecate all_foreign_keys_valid??

[s-mage]

Looks like this PR is abandoned, so I'll try to bring some attention to it with this comment.

So, the issue it tries to solve is still very much relevant. I was adding https://github.com/djezzzl/database_consistency this gem to a project I work on. It generated a hundred new migrations, and referential integrity checks stoped passing because our fixtures were inconsistent.

I solved this with putting a debugger in where all_foreign_keys_valid? rescues and swallows the error, but that wasn't an ideal experience. Ideally it should have said what it didn't like, not a generic "Foreign key violations found in your fixture data". Passing postgres error would do.

If you don't want to break existing adapters, which is understandable, I suggest adding logging there and moving on, that's a lot better than nothing.

Another approach, not the cleanest one, would be not swallowing this error at all. The only place you use this method is the place where you raise when it returns false. This would break the "method ends with ? -> it returns a bool" intuition, but would improve user experience here.

I know nobody asked by advice here, but can't rephrase it so it sounds nicer, sorry. Feel free to ignore it.

Also, rails maintainers, thanks for the work you're doing for all those years. And @danini-the-panini thank you for the patch.

[ghiculescu]

I've pulled the ideas from this PR and #47780 into a single PR, #47990, which I think should be good to merge. @yahonda could you take a look?