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

Basic support for adding and removing foreign keys #15606

Merged
merged 18 commits into from Jun 26, 2014

Conversation

Projects
None yet
@senny
Member

senny commented Jun 10, 2014

We had this as a GSoC idea but there was no accepted student to work on it. Because I was really looking forward to see this in rails/rails I made a first draft.

This patch adds basic support for adding and removing foreign keys using the migration DSL:

add_foreign_key :from, :to # will add the key to `to_id`
add_foreign_key :from, :to, column: "fk_id" # with specific column

add_foreign_key :from, :to, on_delete: :restrict # to specify ON DELETE RESTRICT
add_foreign_key :from, :to, on_delete: :cascade # to specify ON DELETE CASCADE
add_foreign_key :from, :to, on_delete: :nullify # to specify ON DELETE SET NULL

add_foreign_key :from, :to, on_update: :cascade # to specify ON UPDATE CASCADE
add_foreign_key :from, :to, on_update: :nullify # to specify ON UPDATE SET NULL

add_foreign_key :from, :to, name: "my_fk" # with specific name
add_foreign_key :from, :to, primary_key: "special_pk" # will reference `special_pk` from `:to`

Removing a foreign key works mostly the same:

remove_foreign_key :from, :to # will remove foreign key on `to_id`
remove_foreign_key :from, column: "fk_id" # will remove foreign key on `fk_id`
remove_foreign_key :from, name: "my_fk" # will remove foreign key named `my_fk`

Foreign keys are dumped after the indexes to schema.rb. The options name and column are always dumped. dependent only when set.

add_foreign_key is reversible, remove_foreign_key is not.

There is a connection#supports_foreign_keys? method to determine wether the adapter does support the API.

TODO:

  • Write documentation
  • Decide wether add-/remove_foreign_key should be no-op when supports_foreign_keys? is false
  • Make sure it works with automatic test schema maintenance (#15394)
  • Schema dumping must ensure the tables are created in the right order
  • Handle long identifiers.
  • rename dependent to on_delete and introduce on_update
  • rename when a table / column is renamed. (because of digest names, this is no longer needed)

Future Work (not this PR):

  • Support foreign keys when creating a table (This would allow SQLite to use FK's as well)

This patch was hugely inspired by foreigner. Credits to @matthuhiggins ❤️

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 10, 2014

Member

Thank you. One thing to make work is the automatic test schema could you check if it is working now? I know it doesn't with foreigner

Member

rafaelfranca commented Jun 10, 2014

Thank you. One thing to make work is the automatic test schema could you check if it is working now? I know it doesn't with foreigner

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 10, 2014

Member

I have not checked the patch but something in the example code caught my attention.

Foreign keys have ON DELETE CASCADE, and ON UPDATE CASCADE. While :dependent resonates to associations, I see a few drawbacks:

  • It is suspicious that there's only one possible value (other than nil).
  • It doesn't generalize to ON UPDATE CASCADE.

What about two explicit flags on_delete_cascade: true, and on_update_cascade?

Coincidentally I have heavily used ON UPDATE CASCADE recently in a database merge where I had to move IDs.

Member

fxn commented Jun 10, 2014

I have not checked the patch but something in the example code caught my attention.

Foreign keys have ON DELETE CASCADE, and ON UPDATE CASCADE. While :dependent resonates to associations, I see a few drawbacks:

  • It is suspicious that there's only one possible value (other than nil).
  • It doesn't generalize to ON UPDATE CASCADE.

What about two explicit flags on_delete_cascade: true, and on_update_cascade?

Coincidentally I have heavily used ON UPDATE CASCADE recently in a database merge where I had to move IDs.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 10, 2014

Member

@fxn the current implementation does not allow for ON UPDATE specifications. The ON DELETE case is broader than the examples:

          def dependency_sql(dependency)
            case dependency
              when :nullify then "ON DELETE SET NULL"
              when :delete  then "ON DELETE CASCADE"
              when :restrict then "ON DELETE RESTRICT"
              else ""
            end
          end

A simple boolean is not enough. This also varies per adapter. PostgreSQL for example has a default of NO ACTION.

Member

senny commented Jun 10, 2014

@fxn the current implementation does not allow for ON UPDATE specifications. The ON DELETE case is broader than the examples:

          def dependency_sql(dependency)
            case dependency
              when :nullify then "ON DELETE SET NULL"
              when :delete  then "ON DELETE CASCADE"
              when :restrict then "ON DELETE RESTRICT"
              else ""
            end
          end

A simple boolean is not enough. This also varies per adapter. PostgreSQL for example has a default of NO ACTION.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 10, 2014

Member

Ah, I see didn't remember that.

Why isn't ON UPDATE supported?

Member

fxn commented Jun 10, 2014

Ah, I see didn't remember that.

Why isn't ON UPDATE supported?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 10, 2014

Member

@fxn because I did not implement it yet 😅

Member

senny commented Jun 10, 2014

@fxn because I did not implement it yet 😅

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 10, 2014

Member

Haha :).

So yes, at the database schema level on_update and on_delete seem to be a better vocabulary to me than :dependent. Or you could have both, but if the DSL has only :dependent I see no equivalent choice for ON UPDATE.

Member

fxn commented Jun 10, 2014

Haha :).

So yes, at the database schema level on_update and on_delete seem to be a better vocabulary to me than :dependent. Or you could have both, but if the DSL has only :dependent I see no equivalent choice for ON UPDATE.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 10, 2014

Member

@fxn agreed. Will update accordingly.

Member

senny commented Jun 10, 2014

@fxn agreed. Will update accordingly.

@matthuhiggins

This comment has been minimized.

Show comment
Hide comment
@matthuhiggins

matthuhiggins Jun 10, 2014

I might as well chime in!

  • I regret using :dependent as the option name. Since the option was named after has_many's option of the same name, it created expectations for users that it would work the same. :on_delete is much better.
  • I always turned down any support for on_update because I viewed foreign keys as a database constraint rather than a way to change the behavior of the application. foreigner did support this through :options: https://github.com/matthuhiggins/foreigner#database-specific-options
  • Similar to above, there will always be database specific constraint types, so you'll probably need the :options option regardless. (Example: matthuhiggins/foreigner#136)
  • This is your first and last time to decide on the default constraint name when no name is provided. "foreign_key_#{from_name}_on_#{column_names * '_'}" better mirrors the naming convention used by add_index.
  • Since add_foreign_key supplants the use of add_index in many cases, users will expect the foreign key methods to work much like the index migration methods. This goes for things such as creating a foreign key during create_table, along with renaming foreign keys.
  • Related to above, it's important to note that adding a foreign key to a column without an index will introduce a new index to that column. This results in some annoyances:
change_table :widgets do |t|
  t.foreign_key :factories
end
# widgets.factory_id now has a foreign key and an index.

change_table :widgets do |t|
  t.remove_foreign_key :factories
end
# widgets still has an index on factory_id
  • The whole sqlite thing is probably worth addressing as a different beast. I have not checked recently, but I believe that the only way to add a foreign key to an existing sqlite table is to drop and recreate the table. I'm sure that this could be done automatically, but it would probably convoluted.

Anyways, those are the things I learned over the last 5 years of trying to keep foreigner as simple as possible. You can get a general gist of problems that you'll need to address or say no to by looking at https://github.com/matthuhiggins/foreigner/issues.

matthuhiggins commented Jun 10, 2014

I might as well chime in!

  • I regret using :dependent as the option name. Since the option was named after has_many's option of the same name, it created expectations for users that it would work the same. :on_delete is much better.
  • I always turned down any support for on_update because I viewed foreign keys as a database constraint rather than a way to change the behavior of the application. foreigner did support this through :options: https://github.com/matthuhiggins/foreigner#database-specific-options
  • Similar to above, there will always be database specific constraint types, so you'll probably need the :options option regardless. (Example: matthuhiggins/foreigner#136)
  • This is your first and last time to decide on the default constraint name when no name is provided. "foreign_key_#{from_name}_on_#{column_names * '_'}" better mirrors the naming convention used by add_index.
  • Since add_foreign_key supplants the use of add_index in many cases, users will expect the foreign key methods to work much like the index migration methods. This goes for things such as creating a foreign key during create_table, along with renaming foreign keys.
  • Related to above, it's important to note that adding a foreign key to a column without an index will introduce a new index to that column. This results in some annoyances:
change_table :widgets do |t|
  t.foreign_key :factories
end
# widgets.factory_id now has a foreign key and an index.

change_table :widgets do |t|
  t.remove_foreign_key :factories
end
# widgets still has an index on factory_id
  • The whole sqlite thing is probably worth addressing as a different beast. I have not checked recently, but I believe that the only way to add a foreign key to an existing sqlite table is to drop and recreate the table. I'm sure that this could be done automatically, but it would probably convoluted.

Anyways, those are the things I learned over the last 5 years of trying to keep foreigner as simple as possible. You can get a general gist of problems that you'll need to address or say no to by looking at https://github.com/matthuhiggins/foreigner/issues.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 10, 2014

Member

@matthuhiggins thanks so much for sharing your insight. Will investigate further to provide a sound starting point. I mostly agree with your points, I'm skeptical about the fk naming though. We had many issues because identifiers that got too long. I'd like to stick to a short naming when possible. Any thoughts on that?

Member

senny commented Jun 10, 2014

@matthuhiggins thanks so much for sharing your insight. Will investigate further to provide a sound starting point. I mostly agree with your points, I'm skeptical about the fk naming though. We had many issues because identifiers that got too long. I'd like to stick to a short naming when possible. Any thoughts on that?

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 10, 2014

Member

Regarding ON UPDATE my take is that databases are who define what are FK constraints for. It's their field rather than ours.

FKs protect you from orphan records in the common case, but also allow you to define what to do when something happens to the parent record. In the use case I mentioned before, I need to issue dozens of statements like this:

UPDATE foos SET id = id + delta;

and ON UPDATE CASCADE is the blessed way to keep the FKs in children tables in sync automatically.

I believe that if we add support for FKs then we need to adapt to whatever FKs mean to database writers.

Member

fxn commented Jun 10, 2014

Regarding ON UPDATE my take is that databases are who define what are FK constraints for. It's their field rather than ours.

FKs protect you from orphan records in the common case, but also allow you to define what to do when something happens to the parent record. In the use case I mentioned before, I need to issue dozens of statements like this:

UPDATE foos SET id = id + delta;

and ON UPDATE CASCADE is the blessed way to keep the FKs in children tables in sync automatically.

I believe that if we add support for FKs then we need to adapt to whatever FKs mean to database writers.

@matthuhiggins

This comment has been minimized.

Show comment
Hide comment
@matthuhiggins

matthuhiggins Jun 10, 2014

Your current naming is completely fine too. Yes, I am aware of the naming limits, in particular with postgres. It is less likely to occur with foreign keys, since it is on only one column rather than multiple. Your choice is completely fine (hey now, it was my choice too). There is also fk_#{table_name}_on_#{column_name}.

Great work btw. I hope that some form of this makes it in.

matthuhiggins commented Jun 10, 2014

Your current naming is completely fine too. Yes, I am aware of the naming limits, in particular with postgres. It is less likely to occur with foreign keys, since it is on only one column rather than multiple. Your choice is completely fine (hey now, it was my choice too). There is also fk_#{table_name}_on_#{column_name}.

Great work btw. I hope that some form of this makes it in.

@matthuhiggins

This comment has been minimized.

Show comment
Hide comment
@matthuhiggins

matthuhiggins Jun 10, 2014

@fxn That is completely reasonable. I was stating where I personally drew the line, and therefore did not support. Similarly, I never supported composite foreign keys. Per your philosophy, should composite foreign keys be supported?

@senny Make sure that this issue does not exist with your new implementation: matthuhiggins/foreigner#110

matthuhiggins commented Jun 10, 2014

@fxn That is completely reasonable. I was stating where I personally drew the line, and therefore did not support. Similarly, I never supported composite foreign keys. Per your philosophy, should composite foreign keys be supported?

@senny Make sure that this issue does not exist with your new implementation: matthuhiggins/foreigner#110

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 10, 2014

Member

@matthuhiggins thanks for the issue reference. I think this is the behavior @rafaelfranca was talking about. As we now have automatic schema maintenance for test runs, this is even more important. I haven't settled on a solution yet but will investigate dropping the database and not the tables.

Member

senny commented Jun 10, 2014

@matthuhiggins thanks for the issue reference. I think this is the behavior @rafaelfranca was talking about. As we now have automatic schema maintenance for test runs, this is even more important. I haven't settled on a solution yet but will investigate dropping the database and not the tables.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 10, 2014

Member

@matthuhiggins Oh absolutely, regading your feedback. Just to be clear I was also talking from the point of view of a team member thinking about a new feature in Rails, without implying anything about your choices in foreigner, since that is your baby and I totally respect your choices there, couldn't be otherwise :).

Regarding composite FKs, I have never used them. In principle I'd say yes from the point of view of Active Record. If there's support for FKs I'd expect support for multiple columns. But then it also depends on practical matters, does it complicate the interface, is it worthwhile? So I'd say yes unless there are practical trade-offs that suggest not to support it.

In the case of ON UPDATE what I see is that it is totally analogous to ON DELETE. I imagine the implementation should be similar, and indeed treating both as equal (as they are in SQL) may even influence naming/interface.

Member

fxn commented Jun 10, 2014

@matthuhiggins Oh absolutely, regading your feedback. Just to be clear I was also talking from the point of view of a team member thinking about a new feature in Rails, without implying anything about your choices in foreigner, since that is your baby and I totally respect your choices there, couldn't be otherwise :).

Regarding composite FKs, I have never used them. In principle I'd say yes from the point of view of Active Record. If there's support for FKs I'd expect support for multiple columns. But then it also depends on practical matters, does it complicate the interface, is it worthwhile? So I'd say yes unless there are practical trade-offs that suggest not to support it.

In the case of ON UPDATE what I see is that it is totally analogous to ON DELETE. I imagine the implementation should be similar, and indeed treating both as equal (as they are in SQL) may even influence naming/interface.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jun 11, 2014

Contributor

we already use foreigner in our code. is this patch fully compatible with existing code that uses foreigner's syntax?

Contributor

egilburg commented Jun 11, 2014

we already use foreigner in our code. is this patch fully compatible with existing code that uses foreigner's syntax?

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jun 11, 2014

Contributor

add_foreign_key is reversible, remove_foreign_key is not.

why not? remove_column is reversible in rails 4 provided it was defined with the params that add_column would usually require

Contributor

egilburg commented Jun 11, 2014

add_foreign_key is reversible, remove_foreign_key is not.

why not? remove_column is reversible in rails 4 provided it was defined with the params that add_column would usually require

Show outdated Hide outdated ...cord/lib/active_record/connection_adapters/abstract/schema_statements.rb
options[:column] ||= foreign_key_column_for(to_table)
primary_key = options.fetch(:primary_key, "id")
options = {

This comment has been minimized.

@egilburg

egilburg Jun 11, 2014

Contributor

foreigner accepts arbitrary options like add_foreign_key(:comments, :posts, options: 'ON UPDATE DEFERRED'). WDYT of supporting that?

@egilburg

egilburg Jun 11, 2014

Contributor

foreigner accepts arbitrary options like add_foreign_key(:comments, :posts, options: 'ON UPDATE DEFERRED'). WDYT of supporting that?

This comment has been minimized.

@senny

senny Jun 11, 2014

Member

I will probably add something of that sort but for now, I'd like to get the basic functionality working and merged.

@senny

senny Jun 11, 2014

Member

I will probably add something of that sort but for now, I'd like to get the basic functionality working and merged.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jun 11, 2014

Contributor

And of course, much ❤️ for proposing this to core Rails.

I always thought that "getting started quickly" and "convention over configuration" doesn't mean "throw basic sane db design out the window". Not having db-level foreign key constraints and only relying on validations/assocs just means a world of hurt when you have to deal with production maintenance, changing application code, data migrations, etc.

So IMO this is a much needed patch and should also have docs that encourage it's use to the same tune as indexes and null constraints.

Contributor

egilburg commented Jun 11, 2014

And of course, much ❤️ for proposing this to core Rails.

I always thought that "getting started quickly" and "convention over configuration" doesn't mean "throw basic sane db design out the window". Not having db-level foreign key constraints and only relying on validations/assocs just means a world of hurt when you have to deal with production maintenance, changing application code, data migrations, etc.

So IMO this is a much needed patch and should also have docs that encourage it's use to the same tune as indexes and null constraints.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jun 11, 2014

Contributor

Also, one thing that bit me with foreigner is that rename_table, rename_column and friends doesn't rename existing foreign key names. This is especially painful when a db refactor results in a completely different table being named what another table was before. Is this something to consider tracking?

Contributor

egilburg commented Jun 11, 2014

Also, one thing that bit me with foreigner is that rename_table, rename_column and friends doesn't rename existing foreign key names. This is especially painful when a db refactor results in a completely different table being named what another table was before. Is this something to consider tracking?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 11, 2014

Member

@fxn Why do you think we need to provide a complete solution for FK's? When I look at other methods from the migration DSL it becomes obvious that we only support a small subset of what would be possible. You can always drop down to execute.

I'm not opposed to build to a more complete solution but I think we should be shipping this as soon as we hit 80 / 20.

Member

senny commented Jun 11, 2014

@fxn Why do you think we need to provide a complete solution for FK's? When I look at other methods from the migration DSL it becomes obvious that we only support a small subset of what would be possible. You can always drop down to execute.

I'm not opposed to build to a more complete solution but I think we should be shipping this as soon as we hit 80 / 20.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 11, 2014

Member

@senny note that my answer had some ifs :).

Member

fxn commented Jun 11, 2014

@senny note that my answer had some ifs :).

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 11, 2014

Member

@egilburg there is basic compatibility between the current implementation and foreigner. The name of the options might vary (currently we have :on_delete and not :dependent). You should be able to migrate over though:

1.) remove foreigner
2.) remove legacy migrations
3.) dump a new schema.rb to have the correct syntax reflected

I will look into the renaming stuff. I know we do this for indexes.

Member

senny commented Jun 11, 2014

@egilburg there is basic compatibility between the current implementation and foreigner. The name of the options might vary (currently we have :on_delete and not :dependent). You should be able to migrate over though:

1.) remove foreigner
2.) remove legacy migrations
3.) dump a new schema.rb to have the correct syntax reflected

I will look into the renaming stuff. I know we do this for indexes.

@senny senny changed the title from WIP: Basic support for adding and removing foreign keys to Basic support for adding and removing foreign keys Jun 18, 2014

Show outdated Hide outdated ...record/lib/active_record/connection_adapters/abstract/schema_creation.rb
when :nullify then "ON #{action} SET NULL"
when :cascade then "ON #{action} CASCADE"
when :restrict then "ON #{action} RESTRICT"
end

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

else ArgumentError ?

@matthewd

matthewd Jun 19, 2014

Member

else ArgumentError ?

Show outdated Hide outdated ...record/lib/active_record/connection_adapters/abstract/schema_creation.rb
@@ -18,11 +18,28 @@ def visit_AddColumn(o)
add_column_options!(sql, column_options(o))
end
def visit_AddForeignKey(o)
sql = <<-SQL
ADD CONSTRAINT #{o.name}

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

We should probably quote this

@matthewd

matthewd Jun 19, 2014

Member

We should probably quote this

Show outdated Hide outdated ...rd/lib/active_record/connection_adapters/postgresql/schema_statements.rb
WHERE c.contype = 'f'
AND t1.relname = #{quote(table_name)}
AND t3.nspname = ANY (current_schemas(false))
ORDER BY c.conname

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

If we're not going to support multi-column FKs, would it be more polite to skip them entirely, rather than dumping a half-complete depiction?

@matthewd

matthewd Jun 19, 2014

Member

If we're not going to support multi-column FKs, would it be more polite to skip them entirely, rather than dumping a half-complete depiction?

Show outdated Hide outdated activerecord/lib/active_record/schema_dumper.rb
@@ -102,6 +103,10 @@ def tables(stream)
end
table(tbl, stream)
end
sorted_tables.each do |tbl|

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

Is it worth a comment here to clarify why the FKs can't follow their respective tables, like indexes?

@matthewd

matthewd Jun 19, 2014

Member

Is it worth a comment here to clarify why the FKs can't follow their respective tables, like indexes?

This comment has been minimized.

@senny

senny Jun 20, 2014

Member

Whom would that comment target? Do you see a situation where the user cares about that?

@senny

senny Jun 20, 2014

Member

Whom would that comment target? Do you see a situation where the user cares about that?

Show outdated Hide outdated activerecord/lib/active_record/schema_dumper.rb
'primary_key: ' + foreign_key.primary_key.inspect,
'name: ' + foreign_key.name.inspect
]
parts << ('on_delete: ' + foreign_key.on_delete.inspect) if foreign_key.on_delete

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

Missing on_update?

@matthewd

matthewd Jun 19, 2014

Member

Missing on_update?

Show outdated Hide outdated activerecord/lib/active_record/schema_dumper.rb
'add_foreign_key ' + remove_prefix_and_suffix(foreign_key.from_table).inspect,
remove_prefix_and_suffix(foreign_key.to_table).inspect,
'column: ' + foreign_key.column.inspect,
'primary_key: ' + foreign_key.primary_key.inspect,

This comment has been minimized.

@matthewd

matthewd Jun 19, 2014

Member

Do we really need to always include this?

@matthewd

matthewd Jun 19, 2014

Member

Do we really need to always include this?

@VanTanev

This comment has been minimized.

Show comment
Hide comment
@VanTanev

VanTanev Jun 23, 2014

One thing possibly worth considering: composite key foreign keys.

VanTanev commented Jun 23, 2014

One thing possibly worth considering: composite key foreign keys.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 23, 2014

Member

@VanTanev currently Active Record does not have proper support for composite primary keys. I'm willing to look into composite foreign keys but that needs to be tackled first.

Member

senny commented Jun 23, 2014

@VanTanev currently Active Record does not have proper support for composite primary keys. I'm willing to look into composite foreign keys but that needs to be tackled first.

@VanTanev

This comment has been minimized.

Show comment
Hide comment
@VanTanev

VanTanev Jun 23, 2014

Indeed, it would be best if AR had proper support for composite keys, but I'm currently running into an issue where Foreigner will create the wrong schema dumps for manually created composite keys. At least a support on the level of "Don't break if composite keys have been manually created" would be nice to have.

VanTanev commented Jun 23, 2014

Indeed, it would be best if AR had proper support for composite keys, but I'm currently running into an issue where Foreigner will create the wrong schema dumps for manually created composite keys. At least a support on the level of "Don't break if composite keys have been manually created" would be nice to have.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 23, 2014

Member

@VanTanev I can see where you are coming from but schema.rb breaks the same for composite primary keys. With the current implementation you need to use structure.sql and not schema.rb if you are using them.

Member

senny commented Jun 23, 2014

@VanTanev I can see where you are coming from but schema.rb breaks the same for composite primary keys. With the current implementation you need to use structure.sql and not schema.rb if you are using them.

@VanTanev

This comment has been minimized.

Show comment
Hide comment
@VanTanev

VanTanev Jun 23, 2014

Ah, you are of course right. Did not know about structure.sql. Thanks.

VanTanev commented Jun 23, 2014

Ah, you are of course right. Did not know about structure.sql. Thanks.

Show outdated Hide outdated ...record/lib/active_record/connection_adapters/abstract/schema_creation.rb
@@ -18,11 +18,28 @@ def visit_AddColumn(o)
add_column_options!(sql, column_options(o))
end
def visit_AddForeignKey(o)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Usually the name of the visitor method match the name of the node it is visiting. In this case it is visiting ForeignKeyDefinition. I'd change to visit_ForeignKeyDefinition and the visit_DropForeignKey I would not name with visit_ prefix and use a normal method name since it is not visiting any node.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Usually the name of the visitor method match the name of the node it is visiting. In this case it is visiting ForeignKeyDefinition. I'd change to visit_ForeignKeyDefinition and the visit_DropForeignKey I would not name with visit_ prefix and use a normal method name since it is not visiting any node.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

These methods can be private right?

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

These methods can be private right?

This comment has been minimized.

Show outdated Hide outdated ...record/lib/active_record/connection_adapters/abstract/schema_creation.rb
private
def visit_AlterTable(o)
sql = "ALTER TABLE #{quote_table_name(o.name)} "
sql << o.adds.map { |col| visit_AddColumn col }.join(' ')
sql << o.foreign_key_adds.map { |fk| visit_AddForeignKey fk }.join(' ')

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

If we match the method name with the node name we can call accept fk here.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

If we match the method name with the node name we can call accept fk here.

Show outdated Hide outdated ...record/lib/active_record/connection_adapters/abstract/schema_creation.rb
@@ -18,11 +18,28 @@ def visit_AddColumn(o)
add_column_options!(sql, column_options(o))
end
def visit_AddForeignKey(o)
sql = <<-SQL
ADD CONSTRAINT #{quote_column_name(o.name)}

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Can we use a proper indentation? 😄

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Can we use a proper indentation? 😄

This comment has been minimized.

@senny

senny Jun 24, 2014

Member

We can but it will be reflected in the String...

@senny

senny Jun 24, 2014

Member

We can but it will be reflected in the String...

This comment has been minimized.

@sgrif

sgrif Jun 24, 2014

Member

strip_heredoc?

@sgrif

sgrif Jun 24, 2014

Member

strip_heredoc?

This comment has been minimized.

@senny

senny Jun 24, 2014

Member

👍

@senny

senny Jun 24, 2014

Member

👍

Show outdated Hide outdated ...cord/lib/active_record/connection_adapters/abstract/schema_statements.rb
end
# Adds a new foreign key to the table. +from_table+ is the table with the key,
# +to_table+ containes the referenced primary key.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

contains the referenced...

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

contains the referenced...

@@ -23,6 +23,8 @@ def visit_AddColumn(o)
def visit_AlterTable(o)
sql = "ALTER TABLE #{quote_table_name(o.name)} "
sql << o.adds.map { |col| visit_AddColumn col }.join(' ')
sql << o.foreign_key_adds.map { |fk| visit_AddForeignKey fk }.join(' ')
sql << o.foreign_key_drops.map { |fk| visit_DropForeignKey fk }.join(' ')

This comment has been minimized.

@senny

senny Jun 24, 2014

Member

@rafaelfranca I like the synergy of AddForeignKey and DropForeignKey. Renaming em to visit_ForeignKeyDefinition would make it less expressive.

@senny

senny Jun 24, 2014

Member

@rafaelfranca I like the synergy of AddForeignKey and DropForeignKey. Renaming em to visit_ForeignKeyDefinition would make it less expressive.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

I see. I agree with you but I think maybe we should remove the visit_ since we are not using the visitor pattern anymore.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

I see. I agree with you but I think maybe we should remove the visit_ since we are not using the visitor pattern anymore.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Well, addColumn is not even but we are using visit_, lets keep them

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Well, addColumn is not even but we are using visit_, lets keep them

This comment has been minimized.

@senny

senny Jun 24, 2014

Member

was thinking the same. Either change all three or none.

@senny

senny Jun 24, 2014

Member

was thinking the same. Either change all three or none.

Show outdated Hide outdated ...cord/lib/active_record/connection_adapters/abstract/schema_statements.rb
def foreign_key_name(table_name, options) # :nodoc:
options.fetch(:name) do
"fk_rails_#{SecureRandom.hex(5)}"

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Maybe we should be explicit the default is to use a digested name on the documentation.

@rafaelfranca

rafaelfranca Jun 24, 2014

Member

Maybe we should be explicit the default is to use a digested name on the documentation.

This comment has been minimized.

@senny

senny Jun 24, 2014

Member

👍 good catch. The current docs are wrong.

@senny

senny Jun 24, 2014

Member

👍 good catch. The current docs are wrong.

@rafaelfranca rafaelfranca merged commit a5b3f37 into rails:master Jun 26, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

rafaelfranca added a commit that referenced this pull request Jun 26, 2014

Merge pull request #15606 from senny/ar/foreign_key_support
Basic support for adding and removing foreign keys
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 26, 2014

Member

:shipit:!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Really awesome work as usual 👏 👏 👏 👏

Member

rafaelfranca commented Jun 26, 2014

:shipit:!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Really awesome work as usual 👏 👏 👏 👏

@fnando

This comment has been minimized.

Show comment
Hide comment
@fnando

fnando Jun 26, 2014

Contributor

<3 <3 <3

Thanks a lot, @senny!

Contributor

fnando commented Jun 26, 2014

<3 <3 <3

Thanks a lot, @senny!

@fabianoleittes

This comment has been minimized.

Show comment
Hide comment
@fabianoleittes

fabianoleittes Jun 26, 2014

Good work @senny, tks! 👍

fabianoleittes commented Jun 26, 2014

Good work @senny, tks! 👍

@shamanime

This comment has been minimized.

Show comment
Hide comment
@shamanime

shamanime Jun 26, 2014

Contributor

Nicely done! 👍 :shipit:

Contributor

shamanime commented Jun 26, 2014

Nicely done! 👍 :shipit:

@chancancode chancancode added the JRuby label Jun 26, 2014

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 26, 2014

Member

thanks guys ❤️

Member

senny commented Jun 26, 2014

thanks guys ❤️

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Jun 27, 2014

Contributor

Nice, really useful. Thanks from Switzerland @senny!

Contributor

thibaudgg commented Jun 27, 2014

Nice, really useful. Thanks from Switzerland @senny!

@tilsammans

This comment has been minimized.

Show comment
Hide comment
@tilsammans

tilsammans Jun 27, 2014

Contributor

Well done, love this. If anything can we make sure all the work by @matthuhiggins is being incorporated. I use Foreigner in all apps, would be best to have a smooth transition between the two.

Contributor

tilsammans commented Jun 27, 2014

Well done, love this. If anything can we make sure all the work by @matthuhiggins is being incorporated. I use Foreigner in all apps, would be best to have a smooth transition between the two.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 27, 2014

Member

@tilsammans as described above you can transition to this. There will be small differences in the API and especially in the naming of foreign keys.

A lot of this PR is based on the work of @matthuhiggins and his work on foreigner. Hopefully we can improve this first draft down the line. ❤️

Member

senny commented Jun 27, 2014

@tilsammans as described above you can transition to this. There will be small differences in the API and especially in the naming of foreign keys.

A lot of this PR is based on the work of @matthuhiggins and his work on foreigner. Hopefully we can improve this first draft down the line. ❤️

@dosire

This comment has been minimized.

Show comment
Hide comment
@dosire

dosire Jun 27, 2014

Contributor

Thanks for getting this into rails Yves! <3

Contributor

dosire commented Jun 27, 2014

Thanks for getting this into rails Yves! <3

@robin850 robin850 added PostgreSQL and removed JRuby labels Jun 27, 2014

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Jun 27, 2014

Member

Awesome work here! ❤️

Member

robin850 commented Jun 27, 2014

Awesome work here! ❤️

@matthuhiggins

This comment has been minimized.

Show comment
Hide comment
@matthuhiggins

matthuhiggins Jun 27, 2014

Great stuff. I gladly relinquish my technical debt to Rails!

matthuhiggins commented Jun 27, 2014

Great stuff. I gladly relinquish my technical debt to Rails!

@Krule

This comment has been minimized.

Show comment
Hide comment
@Krule

Krule Jul 12, 2014

Finally! Thank you. ❤️

Krule commented Jul 12, 2014

Finally! Thank you. ❤️

@senny senny deleted the senny:ar/foreign_key_support branch Jan 27, 2015

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jun 22, 2015

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