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

Add Foreign Key support #1421

Closed
wants to merge 1 commit into from
Closed

Add Foreign Key support #1421

wants to merge 1 commit into from

Conversation

lifeiscontent
Copy link

@mcmire I couldn't find an API in rails to check if a foreign key exists, I'm guessing this is why you guys previously didn't support it.

If you've got some notes on how I could make the PR better, feel free to share, otherwise now that I know what I know, I'd also be happy to close this PR and issue.

it 'accepts a column that is has a foreign_key with options' do
expect(with_table(:author_id, :integer, foreign_key: { to_table: :users })).
to have_db_column(:author_id).with_options(foreign_key: { to_table: :users })
end
Copy link
Collaborator

@vsppedro vsppedro Feb 23, 2021

Choose a reason for hiding this comment

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

Hi there, thanks for your contribution!

Would you mind adding a test when the foreign_key does not exist?

Imagine a developer creating a test first just to see the failure and then adding the missing foreign_key.

Just to check that this is also working.

Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@vsppedro is that even possible? based on my research into rails it doesn't look like they provide an API to look at the foreign keys from the model that are in the schema.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may be possible to check, but not through the Column class. It looks like you might be able to get a list of all foreign keys through ActiveRecord::Base.connection: https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L581.

Copy link
Author

Choose a reason for hiding this comment

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

@mcmire I tried that but in the test it just returns an empty array

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh, hmm, I'm wondering if this is limited to Postgres and MySQL. The tests run using SQLite as a default but you can switch to Postgres by prepending the test command with DATABASE_ADAPTER=postgresql. Maybe see how you fare with that?

Copy link
Author

Choose a reason for hiding this comment

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

@mcmire does it make sense to add this feature considering it has that as an edge case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid that the way it is will give some false positive.

Examples:

    it 'why isn\'t it failing?' do
      # The column is not a foreign_key.
      expect(with_table(:value, :integer, foreign_key: false)).
        to have_db_column(:value).with_options(foreign_key: true)
    end

    it 'why isn\'t it failing?' do
      # The column is not a foreign key.
      expect(with_table(:value, :integer, {})).
        to have_db_column(:value).with_options(foreign_key: true)
    end

    it 'why isn\'t it failing?' do
      # The column is not a foreign key.
      expect(with_table(:value, :integer, invalid_option: false)).
        to have_db_column(:value).with_options(foreign_key: true)
    end

    it 'why isn\'t it failing?' do
      # Using a string
      expect(with_table(:value, :string, this_is_not_a_foreing_key: false)).
        to have_db_column(:value).with_options(foreign_key: true)
    end

All of these tests pass, correct me if I'm wrong, they shouldn't, because they don't have the correct value - foreign_key = true. I will try to understand the problem a little more here to perhaps suggest a solution.

Copy link
Author

Choose a reason for hiding this comment

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

@vsppedro that's fine, I'll close and remove foreign keys from my test upstream, thanks everyone for the amazing feedback and support on this, really appreciate it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok! Well I was going to say it's up to you since you requested this feature on whether have_db_column should support foreign keys, but if you find you do need this then don't hesitate to submit another PR and we'll try to help you some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants