Deferrable constraints in Migrations DSL

[Rodrigo Rosenfeld Rosas]

Although I don't use ActiveRecord as an ORM, I do use its migrations framework for evolving my database schema through standalone_migrations.

But I'm finding the current DSL limited with regards to deferrable constraints support.

Let me demonstrate with a common use-case, like trees implementation, with SQL written for PostgreSQL:

create table tree(parent_id integer not null, position integer not null, unique(parent_id, position));
insert into tree(parent_id, position) select generate_series(1, 3), 1;
insert into tree(parent_id, position) select generate_series(1, 3), 2;

update tree set position = position + 1;

ERROR:  duplicate key value violates unique constraint "tree_parent_id_position_key"

In PostgreSQL the constraint check is performed during the UPDATE, so its success will depend on what order PG will try to update the "position" field.

Since we want PG to defer the constraint check to the end of the UPDATE statement (it is also possible to specify constraints to be check on transaction commit, but I'll keep this short) we must create this "tree" table in PG like this:

create table tree(parent_id integer not null, position integer not null, unique(parent_id, position) deferrable);

But there is no DSL support in migrations to tell the database to use a deferrable constraint when supported by the database.

Also, I believe that deferrable should be the default and one should specify "deferrable: false" if this is not desired in case add_index would support a "deferrable" option. In such case, it should generate a statement like:

alter table :table_name add constraint :constraint_name unique (parent_id, position) deferrable

Another option would be to introduce some "add_constraint" DSL to Migrations. Anyway, there should be some way of stating this deferrable information in schema.rb.

Each approach would be fine to me as long as I don't need to do it myself using PG-specific SQL with "execute" method.

Thanks for considering,
Rodrigo.

[Gary Weaver]

afaik using foreigner is still the recommended way to handle fkeys in Rails. Here is the pull request in foreigner where they discuss deferrable a few years ago: https://github.com/matthuhiggins/foreigner/pull/32

Using Foreigner v0.9.1 or later and using structure.sql vs. schema.rb then can specify :options in foreigner's add_foreign_key, e.g.

add_foreign_key(:employees, :companies, :options => 'deferrable')

Related rails ticket which had no follow through, mostly related to deferred use in fixtures: https://github.com/rails/rails/issues/5523

Unrelated, but for those interested, sequel just added support for deferrable constraints in pg: https://github.com/jeremyevans/sequel/pull/583

Hope that helps.

[Gary Weaver]

For the create_table though, I'm not sure there is a way to specify options that go inside the parenthesis.

For example this fails because it puts deferrable on the outside of the parenths:

    create_table(:test3, :options => 'deferrable') do |t|
      t.string :some_id
    end

$ rake db:migrate
==  TestMigration: migrating ==================================================
-- create_table(:test3, {:options=>"deferrable"})
rake aborted!
An error has occurred, this and all later migrations canceled:

PG::Error: ERROR:  syntax error at or near "deferrable"
LINE 1: ...al primary key, "some_id" character varying(255)) deferrable

Maybe there should be a concept like column_options: 'deferrable'

    create_table(:test3, :column_options => 'deferrable') do |t|
      t.string :some_id
    end

?

[Gary Weaver]

So proposal would be in activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

You would add :column_options support, like:

      def create_table(table_name, options = {})
        td = table_definition
        td.primary_key(options[:primary_key] || Base.get_primary_key(table_name.to_s.singularize)) unless options[:id] == false

        yield td if block_given?

        if options[:force] && table_exists?(table_name)
          drop_table(table_name, options)
        end

        create_sql = "CREATE#{' TEMPORARY' if options[:temporary]} TABLE "
        create_sql << "#{quote_table_name(table_name)} ("
        create_sql << td.to_sql
        create_sql << "#{options[:column_options]}"
        create_sql << ") #{options[:options]}"
        execute create_sql
        td.indexes.each_pair { |c,o| add_index table_name, c, o }
      end

Not sure if would want to add similar option in other places to be consistent, though...

[Gary Weaver]

Forgot the preceding space, so that line should really be:

        create_sql << " #{options[:column_options]}" if options.has_key?(:column_options)

That way the outputted sql will be same as was before, even if not specifying that option.

[Gary Weaver]

Changed it to table_options because otherwise could conflict with create_join_table which already used column_options.

Pull request: https://github.com/rails/rails/pull/8353

[Jeremy Evans]

On Wed, Nov 28, 2012 at 9:45 AM, Gary Weaver <garys...@gmail.com> wrote:

So proposal would be in activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

You would add :column_options support, like:

      def create_table(table_name, options = {})
        td = table_definition
        td.primary_key(options[:primary_key] || Base.get_primary_key(table_name.to_s.singularize)) unless options[:id] == false

        yield td if block_given?

        if options[:force] && table_exists?(table_name)
          drop_table(table_name, options)
        end

        create_sql = "CREATE#{' TEMPORARY' if options[:temporary]} TABLE "
        create_sql << "#{quote_table_name(table_name)} ("
        create_sql << td.to_sql
        create_sql << "#{options[:column_options]}"
        create_sql << ") #{options[:options]}"
        execute create_sql
        td.indexes.each_pair { |c,o| add_index table_name, c, o }
      end

Not sure if would want to add similar option in other places to be consistent, though...

DEFERRABLE is a per-constraint setting, not a per-table setting, so the idea of using :column_options as a per-table option makes no sense.  It needs to be added as a per constraint setting if it's added at all.  I didn't think ActiveRecord supported creating constraints at all, other than unique constraints implicitly created via unique indexes (and I don't think you can mark those as deferrable using the CREATE UNIQUE INDEX syntax, at least in PostgreSQL), so talking about supporting deferrable constraints when you don't support constraints at all puts the cart before the horse.

Jeremy

[Gary Weaver]

Jeremy,

I looked at the postgres documentation and it looked like it is both column level and table level, but maybe I misread it- could you look?

http://www.postgresql.org/docs/9.1/static/sql-createtable.html

CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] table_name
    OF type_name [ (
  { column_name WITH OPTIONS [ column_constraint [ ... ] ]
    | table_constraint }
    [, ... ]
) ]
[ WITH ( storage_parameter [= value] [, ... ] ) | WITH OIDS | WITHOUT OIDS ]
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace ]

where column_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL |
  NULL |
  CHECK ( expression ) |
  DEFAULT default_expr |
  UNIQUE index_parameters |
  PRIMARY KEY index_parameters |
  REFERENCES reftable [ ( refcolumn ) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ]
    [ ON DELETE action ] [ ON UPDATE action ] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]

and table_constraint is:

[ CONSTRAINT constraint_name ]
{ CHECK ( expression ) |
  UNIQUE ( column_name [, ... ] ) index_parameters |
  PRIMARY KEY ( column_name [, ... ] ) index_parameters |
  EXCLUDE [ USING index_method ] ( exclude_element WITH operator [, ... ] ) index_parameters [ WHERE ( predicate ) ] |
  FOREIGN KEY ( column_name [, ... ] ) REFERENCES reftable [ ( refcolumn [, ... ] ) ]
    [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE action ] [ ON UPDATE action ] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]

[Jeremy Evans]

It's neither column-level nor table-level, it's constraint-level.  Tables can have multiple constraints, each with different DEFERRABLE values, and those constraints are not necessarily tied to specific columns.

Jeremy

[Rafael Mendonça França]

I don't think this is a good fit for the core.

You are able to use SQL statements inside your migrations, so we already support this.

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/6nltHuNceFMJ.

To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

[Gary Weaver]

Jeremy, you're right- my example was incorrect.

So, I'm unsure whether we should allow the user in the migration to specify additional sql for the constraints like this:

create_table(:foos, table_options: "FOREIGN KEY(column2) REFERENCES table1(column1) DEFERRABLE, FOREIGN KEY(column3) REFERENCES table2(column2) DEFERRABLE, ")

since people should maybe be doing that through foreigner. Should my patch gun be holstered?

[Gary Weaver]

Was thinking this was something that might allow easier specification of DEFERRABLE in table creation, but I misread. I agree that having yet another way to specify custom SQL for constraints is not a good idea.

[Rodrigo Rosenfeld Rosas]

I think I'm late at the party and lost a lot happening here yesterday...

At least it is good to know what core considers a good fit or not for AR Migrations. Since constraints are a fundamental piece in my projects I get the warning "you should avoid AR at all, including AR migrations". Currently it is not a big deal for me as I don't plan to support any other database vendor than PG so I can keep using "execute" in my migrations but I thought that maybe some OSS projects could benefit from some built-in support in AR migrations when using a popular plugin like this one:

https://github.com/collectiveidea/awesome_nested_set/blob/master/lib/awesome_nested_set/awesome_nested_set.rb#L655

I don't really believe the method above will always work in PG, for example, if some OSS project has created an index like:

add_index :some_table, [:parent_id, :lft], unique: true
add_index :some_table, [:parent_id, :rgt], unique: true

The Rails community still doesn't seem to be mature enough yet with regards to database constraints, which is unfortunate, so I believe most people using awesome_nested_set (I never used, just took some popular gem as an example) don't have the indices above in their migration. And if it happens for an OSS project to have an unique index like above it is likely that it won't always work in PG unless the OSS project uses a custom "add_unique_index" that would be portable among database vendors to include the deferrable constraint instead for databases like PG.

I just suggested that unique constraints/indices should be deferrable by default to avoid the problem above, which seems to be a very common use-case. Otherwise the message that comes to me is "if you're not doing MySql, get away from AR. we just pretend to support PG, but if you need first-class support, you should be really considering a more robust ORM, like Sequel".

[Gary Weaver]

Rodrigo,

We're using postgres with structure.sql and a legacy DB that has plenty of constraints (fkey, unique) and in the Rails app using that schema for new migrations we're using foreigner ( https://github.com/matthuhiggins/foreigner ) to add new fkeys, and iirc using add_index for some unique constraints (a la http://stackoverflow.com/a/3370333/178651 ). I've read mixed things about using deferrable for everything, but it might be appropriate in some apps.

AR supports using execute to add/remove constraints, etc. also, and foreigner supports deferrable via specifying new constraints like:

add_foreign_key(:employees, :companies, :options => 'deferrable')

Rails create_table I think may be the only place that doesn't allow custom SQL for constraint definition, and I got schooled on that yesterday when trying to allow a spot for custom SQL within the parenths of the generated SQL for create table. :)

I agree that Rails could be more constraint-friendly, but I think you can accomplish what you are trying to do with what is there?

Gary

[Rodrigo Rosenfeld Rosas]

Em 29-11-2012 09:21, Gary Weaver escreveu:

Rodrigo,

We're using postgres with structure.sql and a legacy DB that has plenty of constraints (fkey, unique) and in the Rails app using that schema for new migrations we're using foreigner ( https://github.com/matthuhiggins/foreigner ) to add new fkeys, and iirc using add_index for some unique constraints (a la http://stackoverflow.com/a/3370333/178651 ). I've read mixed things about using deferrable for everything, but it might be appropriate in some apps.

AR supports using execute to add/remove constraints, etc. also, and foreigner supports deferrable via specifying new constraints like:

add_foreign_key(:employees, :companies, :options => 'deferrable')

Rails create_table I think may be the only place that doesn't allow custom SQL for constraint definition, and I got schooled on that yesterday when trying to allow a spot for custom SQL within the parenths of the generated SQL for create table. :)

I agree that Rails could be more constraint-friendly, but I think you can accomplish what you are trying to do with what is there?

I really don't understand this argument. You could really just get rid of all DSL in migrations and keep only "execute" with no loss of functionality, right? You could as well just use SQL everywhere in your application instead of relying on any kind of ORM, right?

Here is another common usage example from another popular gem that would have problem when "add_index [:parent_id, :position], unique:true" is used:

acts_as_list scope: :parent (see meaning here: https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L20)
https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L179 (problematic implementation when add_index is used with unique: true)

I don't understand why AR migrations insists that foreign keys are not much important to be included by default. There have always been external gems to add support DSL to foreign keys but some of them are no longer maintained after a while. This is the main problem in my opinion. You rely on another gem, like foreigner, and then some years later when a new AR is released at some point you don't get more support from that gem. I don't recall the name right now but there was a quite popular gem in Rails 1 era to add foreign keys constraints to AR migrations DSL that is no longer maintained.

If the Rails community can't convince itself about the importance of basic things in ACID databases like transactions, foreign keys and other constraints than I think I'm out of luck with regards to deferrable constraints... :( (yes, when I talk about transactions I mean the huge amount of Rails applications out there running MySql with MyISAM engine, that used to be the default one until recently in 5.5 when the InnoDB is now the default one).

When developing open-source applications (like Redmine, Gitorious, etc) you usually want to support as many database vendors as possible. So, what happens if some application like this uses your proposed solution?

add_foreign_key(:employees, :companies, :options => 'deferrable')

What is the effect of the code above in such scenario if a user wants to run the code on MySql or MS SQL Server?

On Thursday, November 29, 2012 5:59:11 AM UTC-5, Rodrigo Rosenfeld Rosas wrote:

I think I'm late at the party and lost a lot happening here yesterday...

At least it is good to know what core considers a good fit or not for AR Migrations. Since constraints are a fundamental piece in my projects I get the warning "you should avoid AR at all, including AR migrations". Currently it is not a big deal for me as I don't plan to support any other database vendor than PG so I can keep using "execute" in my migrations but I thought that maybe some OSS projects could benefit from some built-in support in AR migrations when using a popular plugin like this one:

https://github.com/collectiveidea/awesome_nested_set/blob/master/lib/awesome_nested_set/awesome_nested_set.rb#L655

I don't really believe the method above will always work in PG, for example, if some OSS project has created an index like:

add_index :some_table, [:parent_id, :lft], unique: true
add_index :some_table, [:parent_id, :rgt], unique: true

The Rails community still doesn't seem to be mature enough yet with regards to database constraints, which is unfortunate, so I believe most people using awesome_nested_set (I never used, just took some popular gem as an example) don't have the indices above in their migration. And if it happens for an OSS project to have an unique index like above it is likely that it won't always work in PG unless the OSS project uses a custom "add_unique_index" that would be portable among database vendors to include the deferrable constraint instead for databases like PG.

I just suggested that unique constraints/indices should be deferrable by default to avoid the problem above, which seems to be a very common use-case. Otherwise the message that comes to me is "if you're not doing MySql, get away from AR. we just pretend to support PG, but if you need first-class support, you should be really considering a more robust ORM, like Sequel".

Em 28-11-2012 17:15, Rafael Mendonça França escreveu:

I don't think this is a good fit for the core.

You are able to use SQL statements inside your migrations, so we already support this.

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/YhSw7VXH1kEJ.

[Gary Weaver]

add_foreign_key(:employees, :companies, :options => 'deferrable')

What is the effect of the code above in such scenario if a user wants to run the code on MySql or MS SQL Server?

Good point. You should bring this up as a foreigner issue and do a pull request to add common options to its adapters like:
https://github.com/matthuhiggins/foreigner/blob/master/lib/foreigner/connection_adapters/postgresql_adapter.rb
and oracle and whatever else supports similar options.

However, I don't think you always would want to specify deferrable, and if you specify a deferrable option, and it isn't supported then should it just ignore it? Then to be DB agnostic you'd have to include all relevant options for each DB, and maybe some share the same name that you wouldn't want to use for others, so then you'd end up with something like (maybe):

add_foreign_key(:employees, :companies, :pg_specific_options => [:deferrable, :some_other_pg_option], :oracle_specific_options => [:some_oracle_option])

but that isn't much different than putting logic in the migration like:

case ActiveRecord::Base.connection.adapter_name
when 'PostgreSQL', 'Oracle'
  

add_foreign_key(:employees, :companies, :options => 'deferrable')

when 'MySQL'
else
  raise "Database adapter not supported. Please file issue at (url to your github repo issues)"

end

but, I still see your point. It would be nice for sure to not have to worry about it.

[Rodrigo Rosenfeld Rosas]

Em 29-11-2012 09:42, Rodrigo Rosenfeld Rosas escreveu:

Em 29-11-2012 09:21, Gary Weaver escreveu:

...

If the Rails community can't convince itself about the importance of basic things in ACID databases like transactions, foreign keys and other constraints than I think I'm out of luck with regards to deferrable constraints... :( (yes, when I talk about transactions I mean the huge amount of Rails applications out there running MySql with MyISAM engine, that used to be the default one until recently in 5.5 when the InnoDB is now the default one).

Sorry, but I've just became aware of this video and didn't resist posting it here :) I'm hoping Rails core members that still use MySQL could open their minds after watching this video:

Why Not MySQL?
http://www.youtube.com/watch?v=1PoFIohBSM4

[Rafael Mendonça França]

Rodrigo, sorry but I think you misunderstood. I don't use MySQL, actually I even don't like it. I prefer to use PostgreSQL. If you take 10 minutes you can see a lot of pull requests adding features for PostgreSQL in Rails.

What I said is that I don't think the feature as it was implemented is a good fit for core. I've been using database constraints but I've always used SQL to create them.

Also I don't like to discuss features without seeing the code. I need to see how the feature was implemented to say if I would accept or not. It don't need to be a full patch but something to, at least, make explicit what are the benefits and the drawbacks of adding a feature to the framework.

We should always look after the cost of maintainability. Add a new feature to Rails is as easy as pressing a green button. Discuss it is even easier. Maintain it is not. I prefer to put into Rails features that I want to maintain in the future and I believe that are good for the framework. This is how Rails work since the beginning.

I'm not saying that I don't believe in your proposed feature, neither that I don't want constraints in the framework. But, without seeing the code I can't discuss anything.

That said, lets see that patch. At least, if it is not accepted, you can easily create a plugin that you can maintain and don't need to worry if it will break in the next Rails release.

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

--

You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.

[Rodrigo Rosenfeld Rosas]

Well, then we have a real problem here. I don't start coding anything before discussing them. I consider it a waste of time (in the case the feature has been rejected).

So I don't think how I could ever contribute to Rails. I won't ever write a patch before getting it accepted first. I've done it once after some previous discussion and after the issue became totally abandoned with no feedback I decided that I wouldn't ever do it again. Too much effort to code to get it rejected or ignored later.

I don't understand why code is needed to discuss a feature or a new API. Some people have a hard work trying to get RSpec to read as plain English but if you try to spec your requested API in plain English, since it is not code, people won't even consider it.

Just pretend my feature requests are Cucumber features ;)

[Steve Klabnik]

Things only happen in Rails if someone implements them. You have to
convince someone to actually do the implementation. It's simplest if
that person is yourself. If nobody else cares enough to implement it,
well, it's not gonna get done.

[Rafael Mendonça França]

We can't accept a feature without know what are the impacts in the codebase.

This is valid for everyone, either the core members.

I can say that I want this feature and accept it, but nothing stops the core members to revert it. And don't expect me to defend your feature, since you should be the interested.

Would be more wast of time if we accept it now and, when you come up with the code, we rejected it because it add more complexity in the framework that we want.

Also, what is the difference of writing 10 huge emails and get the feature reject? I think is the same. With working code you have more chances. And, as I said in my last email, either if we don't accept, you will have working code that solves your problem.

If you don't want to scratch your itch and provide a patch, I'm sorry, but don't expect we to accept.

And, just for record, I don't like Cucumber ;)

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

[Gary Weaver]

I won't ever write a patch before getting it accepted first. I've done it once after some previous discussion and after the issue became totally abandoned with no feedback I decided that I wouldn't ever do it again. Too much effort to code to get it rejected or ignored later.

You could gist an idea or provide a testless spike with minimum viable changes. In that other thread I started ( https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-core/WW9Zl5QC-vI ), what I was getting at was to try to semi-accomplish some way of determining when you could use foreigner or execute to add constraints in a db agnostic way by at least allowing a way to check that from the migration without looking at adapter_name. Version could be important for determining what to execute also.

From what I can tell it isn't that they don't want people to contribute, or anything elitist like that. Sure some have reps to maintain to be on the talk circuit and get books sold, but mostly I think they just don't want something added that makes it harder to maintain, because resources are already limited for the amount of work that needs to be done. And also it is a little disconcerting to hear people that have left Rails badmouth it (head over to the ruby forum for that) and to see (from various sources: indeed.com graphs, google trends, open source code contribution graphs) that ruby/rails have somewhat stagnated, I think that ruby has a lot going for it and rails still provides a hell of a lot, so it would be nice if enough people got interested to help to get the adoption curve moving up again. I hope I'm not getting in the way of that. I'll be the first to admit that I make a lot of mistakes and if you look through my github account there is a lot of crap in there, but getting to be involved in any way is still pretty cool.

[Gary Weaver]

And, just for record, I don't like Cucumber ;)

I didn't like it either, but these guys make it look easy, at least to me :) : https://github.com/gregbell/active_admin/tree/master/features

I think it depends on what it is being used for. I'd rather be writing rspecs though. When is Rails going to give up and just use rspec? :)

[Carlos Antonio da Silva]

Gary, I'd say never, most of the core prefer not to :)

At.
Carlos Antonio

[Rodrigo Rosenfeld Rosas]

No one claims anything other than that. I just don't understand why code
must be written before discussion.

[Rodrigo Rosenfeld Rosas]

Em 29-11-2012 20:06, Rafael Mendonça França escreveu:

We can't accept a feature without know what are the impacts in the codebase.

In the application I maintain I don't need code to understand the impact of some feature. I don't understand why that would be different for Rails maintainers.

This is valid for everyone, either the core members.

I can say that I want this feature and accept it, but nothing stops the core members to revert it. And don't expect me to defend your feature, since you should be the interested.

My perception is that although this list is called rails-core it seems core members don't actually read it. Otherwise it would be easy to achieve a consensus about approving this feature or not before implementing it.

It seems to me that you're suggesting that Rails development is much more like an anarchy. Listening to Aaron in some podcasts also seems to indicate that this is the case as well. This is how I'm understanding development happen on Rails core:

- a core member wants a feature
- he writes the change, commit it and push it
- another core member don't like the change and revert the commit
- some other core member liked the change and revert the commit that has been reverted

How is that sane? In what point prior discussion became invaluable? This is not how I manage my teams/softwares and I'm pretty sure this is not how most projects out there work.

Would be more waste of time if we accept it now and, when you come up with the code, we rejected it because it add more complexity in the framework that we want.

You should be able to understand the impact in complexity that such a simple feature like that would have in the code base beforehand. If it happens that you don't like my change, I'd expect you to show me how you think such feature should be implemented instead. But it wouldn't be rejected. At some point the implementation would conform to what the core-team expect it to look like.

Also, what is the difference of writing 10 huge emails and get the feature reject?

Getting the feature rejected is not the issue. Spending time on developing the feature that got rejected is. I don't consider my e-mails huge (they are not small) but I see that for today's standards where people got used to Twitter (I can't use it) my messages may seem huge. But I don't spend much time writing messages as I would spend to start contributing to any big open-source code. See, last time I contributed to Rails was several years ago, much has changed since then and I'd need to read tons of instructions and code before I started to contribute again. Additionally, AR is inside Rails repository. That means that I can't just point to AR master in my Gemfile for instance, but this is a separate issue, let me get back to the subject.

In the end I just don't mind discussing features, but I do mind writing code that won't ever see the light of the day just because people can't discuss them up-front. This is similar to the Refinements discussion in the Ruby-core list. It started with code, that code has been merged to trunk even before the discussion started. I'm pretty sure Charles Nutter was able to tell the performance impacts and had more insights about that feature just looking at the proposed feature description without starting coding it. If this discussion had started before the feature being implemented the first implementation would probably be much more aligned to the final specs and with less effort.

I think is the same. With working code you have more chances. And, as I said in my last email, either if we don't accept, you will have working code that solves your problem.

No, I won't. First, I don't have a problem. I use PG and I have no plans to support other DB vendor. But it crossed my mind the idea that maybe some open-source software could benefit from a common API to add support for deferrable unique constraints because some very common use cases (ordered tree/list) would require that and just asked here if there was such an interest in that feature.

But if I were to support that in some OSS project and I knew beforehand that this feature isn't going to be accepted in AR I wouldn't use all code that I wrote to make a patch to Rails. I would rather use a much simpler approach:

module ARExtensions
  def add_deferrable_unique_index(table_name, columns, options={})
     (add_index table_name, columns, options.merge(unique: true); return) unless db_supports_deferrable_constraint?
     name = options[:name] || ['idx', table_name, columns, 'unique'].flatten.join('_')
     execute "alter table #{table_name} add constraint #{name} unique (#{columns.join ', '}) deferrable"
  end
 
  private

  def db_supports_deferrable_constraint?
     # TODO:  code to detect database and version here -  would return true for PG and false for MySQL
  end
end

Then just include that module in your migration classes when you need add_deferrable_unique_index. Much simpler to write than writing a patch to AR with docs and tests.

If you don't want to scratch your itch and provide a patch, I'm sorry, but don't expect we to accept.

Ok, I get the message, just don't expect me to contribute to Rails if I have to submit the code before discussing it.

And, just for record, I don't like Cucumber ;)

Me neither :) But I had to try to convince you, maybe you were a Cucumber fan, who knows? ;)

[Rodrigo Rosenfeld Rosas]

An example why it is not that fast to start contributing to Rails:

just tried running bundle from master branch:

Bundler could not find compatible versions for gem "multi_json":
  In snapshot (Gemfile.lock):
    multi_json (1.2.0)

  In Gemfile:
    rails (>= 0) ruby depends on
      multi_json (~> 1.3) ruby

Running `bundle update` will rebuild your snapshot from scratch, using only
the gems in your Gemfile, which may resolve the conflict.

I don't mind spending time contributing to code when I have some free time if the code won't be just rejected. But assuming that I would be able to start contributing with code faster than writing "huge" messages is wrong.

[Rodrigo Rosenfeld Rosas]

Weird, I just fixed it by running bundle update multi_json but git diff doesn't report any changes to Gemfile.lock, go figure it out...

[Rafael Mendonça França]

My perception is that although this list is called rails-core it seems core members don't actually read it. Otherwise it would be easy to achieve a consensus about approving this feature or not before implementing it.

Wrong, they read. I'm a core member.

- a core member wants a feature
- he writes the change, commit it and push it
- another core member don't like the change and revert the commit
- some other core member liked the change and revert the commit that has been reverted
How is that sane? In what point prior discussion became invaluable? This is not how I manage my teams/softwares and I'm pretty sure this is not how most projects out there work.

This is right. But this is more like:

- someone want a feature

- someone write the patch

- the core team discuss

- we accept or reject the feature

- Maybe someone from the core that didn't participate from the discussion has a strong opinion about the feature, we start the discussion again and if we agree, revert.

This is how Rails works since the beginning. And I think it is working.

If it happens that you don't like my change, I'd expect you to show me how you think such feature should be implemented instead. But it wouldn't be rejected. At some point the implementation would conform to what the core-team expect it to look like.

You said everything, all pull requests that I reviewed and the core agree was merged. The discussions were not, since they are only email threads. Pull requests are the best place to discussion, we can see the implementation details and the feature overview in one place.

No, I won't. First, I don't have a problem. I use PG and I have no plans to support other DB vendor. But it crossed my mind the idea that maybe some open-source software could benefit from a common API to add support for deferrable unique constraints because some very common use cases (ordered tree/list) would require that and just asked here if there was such an interest in that feature.

Rails is not a set of guessing features. We try to extract features from real applications that we think is a good fit for the framework.

And to close this thread, I'm really sorry if the contribution experience was not good for you in the past, but you can be sure that there are a lot of people trying to make it better now.

Please, send a patch if you want, you may be surprised. ;)

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca

[Rodrigo Rosenfeld Rosas]

Em 30-11-2012 10:38, Rafael Mendonça França escreveu:

My perception is that although this list is called rails-core it seems core members don't actually read it. Otherwise it would be easy to achieve a consensus about approving this feature or not before implementing it.

Wrong, they read. I'm a core member.

You're one core member. Where are the other ones?

- a core member wants a feature
- he writes the change, commit it and push it
- another core member don't like the change and revert the commit
- some other core member liked the change and revert the commit that has been reverted
How is that sane? In what point prior discussion became invaluable? This is not how I manage my teams/softwares and I'm pretty sure this is not how most projects out there work.

This is right. But this is more like:

- someone want a feature

- someone write the patch

- the core team discuss

- we accept or reject the feature

- Maybe someone from the core that didn't participate from the discussion has a strong opinion about the feature, we start the discussion again and if we agree, revert.

This is how Rails works since the beginning. And I think it is working.

I have seen lots of projects working out there in some companies without even using any kind of SCM. All projects work. It doesn't mean that they do everything the best possible way.

If it happens that you don't like my change, I'd expect you to show me how you think such feature should be implemented instead. But it wouldn't be rejected. At some point the implementation would conform to what the core-team expect it to look like.

You said everything, all pull requests that I reviewed and the core agree was merged. The discussions were not, since they are only email threads. Pull requests are the best place to discussion, we can see the implementation details and the feature overview in one place.

No, I won't. First, I don't have a problem. I use PG and I have no plans to support other DB vendor. But it crossed my mind the idea that maybe some open-source software could benefit from a common API to add support for deferrable unique constraints because some very common use cases (ordered tree/list) would require that and just asked here if there was such an interest in that feature.

Rails is not a set of guessing features. We try to extract features from real applications that we think is a good fit for the framework.

And to close this thread, I'm really sorry if the contribution experience was not good for you in the past, but you can be sure that there are a lot of people trying to make it better now.

Please, send a patch if you want, you may be surprised. ;)

Ok, I'll give it a try, but it will take some time before I can start working on that. I'm under pressure right now and I don't think things will get better in my job until next year. I just tried to git pull my rails clone and run the tests after following this:

http://edgeguides.rubyonrails.org/development_dependencies_install.html

But it seems the test suite don't pass when the falcon patch is applied to Ruby, which is my case:

https://github.com/rails/rails/issues/8384

So, it is not that quick either to get started contributing to Rails (it may require an hour or two just to get the tests passing).

So, I'll post-pone this patch as it seems to be a simple one and I guess people are not opposing against it. But it is likely that I won't try to contribute to Rails for many more years if the patch isn't accepted after some feedback to get it in conformance with core's taste.

Cheers,
Rodrigo.

[Gary Weaver]

So, it is not that quick either to get started contributing to Rails (it may require an hour or two just to get the tests passing).

Rodrigo, did you try the VM method? https://github.com/rails/rails-dev-box

Also, you could just even setup a new RVM gemset, modify Rails in that gemset itself, and if that works (not adding any tests), just patch rails and do a pull request noting in the pull request that you will add tests, etc. if people are interested, then it may be closed or rejected, but then you could redo the request or ask that it be reopened after adding tests and making requested changes. I know some might not like this, but even just a direct untested pull request as a "take a look at this" could be a way to get started at the bare minimum (hopefully at least run ruby -wc path/to/file on the files you edit for syntax check :) ), and a gist wouldn't even involve a fork (could just copy and paste from a file in rails).

I totally understand about the time thing, though. It is good that there is a VM now available, but even that still takes time to setup.

At first it seems overwhelming as a whole just like any (somewhat) large project. But if you just find in files, or grep -r something *, or find . -name "*something*.rb", or find . -name "*something*.rb" -exec grep -H something_else {} \; through the code, ~90% of the time (maybe that is too high) it is readable enough to find what is going on even without a backtrace or tracing, at least in pieces, without having to go that deep. And then slowly as you add something (maybe in an initializer in your app, or in a gem you write) to extend it, you can get more knowledgeable in that part. I think the core team just wants to see people have enough momentum that they only have to course-correct new features; I don't think they are trying to stop anyone, but I know it is frustrating when in other projects you can just say "I'd like X" and they start working on it for you or tell you what files to change or give you some tips up front or spend time discussing it even if they don't know whether anyone would be able to code what they are talking about. Good luck! Would be cool if you could spend time on what you are talking about, because I think the project I'm working on would benefit from your additions to Rails.

[Gary Weaver]

On Friday, November 30, 2012 7:55:43 AM UTC-5, Rodrigo Rosenfeld Rosas wrote:

  it is likely that I won't try to contribute to Rails for many more years if the patch isn't accepted after some feedback to get it in conformance with core's taste.

Understand your past experience, but to limit the chance of rejection you might want to read up on the list and elsewhere on past arguments about why constraints weren't added and try to come up with something that would address those concerns. The problem you are trying to solve seems like it will be more politically (for lack of a better adverb) tough than it is to actually implement the code in the adapters, etc. Maybe think of maintainability first, backwards compatibility second, and flexibility third, and then place your priorities at the bottom of that stack of priorities. (That probably sounds ridiculous, but I'd bet it is correct.)

[Gabriel Sobrinho]

França,

I don't know why we need to see the code to discuss if the feature makes sense or not.

Note that agreeing with the feature will be valuable to the framework does not mean the first implementation will be merged.

After all this thread I don't actually know if foreign keys will be accepted or not.

The question is: Will be foreign keys accepted on rails?

If yes, we need to discuss how we will achieve that.

The rails development can work in this way today, but is this the best way?

The community complained about that before and we are complaining about it again.

Returning to the topic, can we receive a green light like "yes, foreign keys makes sense to rails" and them work in how we'll be achieve that?

If we receive "no, foreign keys does not makes sense to rails" we won't spent time on it :)

Like I said before, I didn't get if it can make sense or not.

[Rodrigo Rosenfeld Rosas]

Em 30-11-2012 13:22, Gary Weaver escreveu:

So, it is not that quick either to get started contributing to Rails (it may require an hour or two just to get the tests passing).

Rodrigo, did you try the VM method? https://github.com/rails/rails-dev-box

Thanks for the pointer, Gary but I avoided it on purpose. 8GB is not enough anymore for my dev machine with so many processes running here (Grails + Rails + GlassFish with Java app + Solr + Redis + PG 9.1 + PG 9.2 + MySql + VirtualBox to test IE + Amarok, etc) I really want to avoid yet another VM :)

But it seems one problem is that I was using the falcon patch to Ruby and the other problem is that I had a few options in ~/.railsrc that prevented the railties tests to pass. All of them have already been fixed and I can now go green with Rails tests. So, next time I find some free time I'll be able to give it a try:

https://github.com/rails/rails/issues/8384
https://github.com/rails/rails/issues/8386

Thanks to Carlos, Rafael and Steve for helping me out in the process :)

Man, those tests in railties are painfully slow! :( Luckily I won't need to change railties but only activerecord I hope :)

So that I can focus on the changes themselves, what is the recommended way of manually testing my changes while pointing my project to use AR under my Rails clone?

gem 'activerecord', path: '~/rails-clone/activerecord' # should this be enough? - recall that I don't use Rails for this database management project, only standalone_migrations gem.

Cheers,
Rodrigo.

[Rodrigo Rosenfeld Rosas]

By the way, before I start implementing this first patch, I'll keep it really short because I won't create more complex patches depending on how this pull request progress...

I'll only add an additional boolean option to add_index called :deferrable. This option will be ignored by non-supporting databases and when unique is false (or should it raise an exception when unique is false?).

Then it will use "alter database add constraint" on PG databases instead of "create index" for such cases.

Please let me know in advance before I implement if the behavior described above is not desired.

[Steve Klabnik]

> So that I can focus on the changes themselves, what is the recommended way of manually testing my changes while pointing my project to use AR under my Rails clone?

Bundler 1.2 has a feature that helps here:

$ bundle config local.activerecord ~/rails-clone/activerecord

This should override the Gemfile entry with your local copy. For more:
http://gembundler.com/v1.2/whats_new.html

[Rodrigo Rosenfeld Rosas]

Thanks!