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_exists?` method. #18662

Merged
merged 2 commits into from Feb 16, 2015
Merged

Add `foreign_key_exists?` method. #18662

merged 2 commits into from Feb 16, 2015

Conversation

@estum
Copy link
Contributor

estum commented Jan 23, 2015

No description provided.

@senny
Copy link
Member

senny commented Jan 24, 2015

@estum thank you for your work. Would you mind sharing some use-cases where you can see this being useful?

@senny senny added the activerecord label Jan 24, 2015
@senny senny added this to the 5.0.0 milestone Jan 24, 2015
@estum
Copy link
Contributor Author

estum commented Jan 24, 2015

@senny I was missing a method like index_exists? (which useful for me too) while reorganizing migrations to avoid add_foreign_key``remove_foreign_key methods raise when a key is already created\removed. I was surprised it yet hasn't been implemented.

# foreign_key_exists?(:accounts, name: "special_fk_name")
#
def foreign_key_exists?(from_table, options_or_to_table = {})
return unless supports_foreign_keys?

This comment has been minimized.

@ianks

ianks Jan 28, 2015 Contributor

We should probably return false instead of nil here to maintain convention with most Ruby .exist? functions.

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

@ianks that's not necessary. Rails does not make the promise of returning real booleans from predicate methods.

This comment has been minimized.

@ianks

ianks Jan 29, 2015 Contributor

Is there a particular reason for now? I would consider it an anti-pattern to return nil from a predicate method as it makes it harder to reason about the function.

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

@ianks there is a section on booleans in our API documentation guidelines.

# # Check a foreign key with a custom name exists
# foreign_key_exists?(:accounts, name: "special_fk_name")
#
def foreign_key_exists?(from_table, options_or_to_table = {})

This comment has been minimized.

@ianks

ianks Jan 28, 2015 Contributor

I would consider renaming to foreign_key_exist? to avoid any confusion. For example File.exists? in Ruby is deprecated in favor of File.exist?. Should we follow that convention?

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

I'd rather stick to the current naming:

  • index_exists?
  • table_exists?

This comment has been minimized.

@estum

estum Jan 29, 2015 Author Contributor

@ianks I've been guided by the naming of similar methods (from @senny's note).

@@ -671,6 +671,29 @@ def remove_reference(table_name, ref_name, options = {})
def foreign_keys(table_name)
raise NotImplementedError, "foreign_keys is not implemented"
end

# Checks to see if a foreign key exists on a table for a given foreign key definition.

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

@estum can you move the method below remove_foreign_key?

options = options_or_to_table
else
options = { column: foreign_key_column_for(options_or_to_table) }
end

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

This part is exactly the same in remove_foreign_key. I think it's worth to extract a method to obtain a foreign key for from_table, options_or_to_table.

This comment has been minimized.

@estum

estum Jan 29, 2015 Author Contributor

@senny May be just make remove_foreign_key to use foreign_key_exists?, and return foreign key's name when it exists? Or add foreign_key_presence method to keep boolean result in ..exists??

This comment has been minimized.

@senny

senny Jan 29, 2015 Member

I think it'd be good to have the method to access the ForeignKeyDefinition based on the user arguments. There are multiple ways how you can specify a key:

  1. from_table + to_table
  2. from_table + column
  3. from_table + name

Would be nice to have that bundled up so that higher level methods don't have to bother with these details.

@senny senny merged commit f0ae503 into rails:master Feb 16, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
senny added a commit that referenced this pull request Feb 16, 2015
Add `foreign_key_exists?` method.
@senny
Copy link
Member

senny commented Feb 16, 2015

@estum great work! 💛

I added a CHANGELOG entry and made some minor changes in the merge commit (removed trailing whitespace, replaced all?{|assoc|} with all? {|key, value|} and got rid of try(:name).

The extraction of foreign_key_for already paid off. Merging this I found a bug in a recent patch I committed (8980da8).

@@ -650,6 +658,10 @@ def remove_references(*args)
def foreign_key(*args) # :nodoc:
@base.add_foreign_key(name, *args)
end

def foreign_key_exists?(*args) # :nodoc:

This comment has been minimized.

@claudiob

claudiob Feb 20, 2015 Member

@senny Is this # :nodoc: intentional? Or should we make this method publicly documented?

This comment has been minimized.

@senny

senny Feb 23, 2015 Member

@claudiob I think it was due to the fact that foreign_key had a no-doc as well. It was previously only used by references. Should be fine to document them though.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.