On Friday, September 13, 2013 8:50:48 PM UTC-7, GregD wrote:
> Jeremy-
>
> Okay, this is probably going to be the hardest sell for me, but I'll give
> it a try because I did get it all integration tests to work except I had to
> make 1 change to Sequel proper and 1 to the schema tests. The duplicate
> SQL was not because of the adapter's alter_table_sql was calling itself,
> but it was in Sequel proper. Sybase does not do implicit alters. In other
> words, you can NOT drop a column that is a foreign key with out dropping
> the foreign key first. Same with adding foreign keys where either the
> column can not be nullable or in the case of a composite key the unique
> constraint must be in place to add the foreign key. My guess is others
> don't require this.
>
> in schema_generator.rb about line 431 we have:
>
> def drop_foreign_key(name, opts=OPTS)
> drop_composite_foreign_key(Array(name), opts)
> drop_column(name) unless name.is_a?(Array) <--- this is causing
> the grieve
> end
>
> If you just drop a column that is a foreign key and in a database the
> requires an explicit drop of the foreign key, then you must drop the
> foreign key first. So, I put the code in to check if a column is part of a
> foreign key on a plane drop column to get that test to pass (a failing test
> earlier). Then when I get to a test that drops a foreign key, it also
> drops the column. It may be a convenience to do so, but I will argue they
> are exclusive actions especially for a database that will not allow you to
> drop a column until you drop the foreign key constraint first. Once I
> commented out that line, then a drop a single column foreign key worked and
> the drop of a column that is a foreign key still worked because of the
> schema check. I still cannot believe other database don't have a problem
> with that, but I'm guessing most will drop a foreign key implicitly when
> just dropping a column.
>
Subclass the AlterTableGenerator in the adapter and override
drop_foreign_key to only call drop_composite_foreign_key if
name.is_a?(Array). Set Database#alter_table_generator_class to the
subclass. See the shared postgres adapter for an example of
adapter-specific generator overrides. That should ensure that the related
DROP statement is only issued once.
> Now for the other problem and it is with a test but along the same
> reasoning. If we add a foreign key in a database that requires explicit
> commands to do so, then we need to add the constraint prior to adding the
> foreign key. This could be making a column not nullable or adding a unique
> constraint on a composite key. Maybe other DBs will do this for you
> implicitly, but I can not imagine all do so. So, in order to get a add
> composite foreign key to work, I had to make sure that a unique constraint
> existed and create it if you add a composite key. This caused the
> following test to fail because it was adding the unique constraint prior to
> adding the key.
>
> in integration/schema_test.rb about line 549
>
> @db.alter_table(:items) do
> add_unique_constraint [:item_id, :id]
> <--- this is causing the grieve
> add_foreign_key [:id, :item_id], :items, :key=>[:item_id, :id]
> <--- because of the checking, this will also add the constraint for a db
> that requires it do.
> end
>
>
> My thought is this test was set up for a database that requires explicit
> commands and other test were mostly set up for database that will
> implicitly do things for you if they are not there.
>
If your add_foreign_key is also adding a unique or NOT NULL constraint to
the referenced table, I think that is what needs to change.
alter_table(:foo){add_foreign_key :column, :bar} should never do anything
starting with ALTER TABLE bar. If there needs to be a unique or NOT NULL
constraint on the referenced table, then the user should add that manually.
> I like to make Sequel act like things are done implicitly and if the DB
> requires explicit commands to accomplish X then it must generate all the
> commands to accomplish X. I'm an old c programmer and if you know anything
> about make then creating implicit rules rule.
>
Whether implicit behavior is good is mostly a matter of taste. Not all
implicit behavior is bad, but I don't think implicitly modifying the
referenced table when altering the referencing table is a good idea. IMO
that breaks the POLS.
> Finally, I know these 2 minor changes may break a lot of DBs, but maybe
> they will make Sequel more consistent when altering a db schema and the
> breaks are not that bad.
>
While I do break backwards compatibility in Sequel on occasion, it's almost
never in anything that would run in a migration, since the idea is that
users should never have to modify old migrations.
That's not to say that SQLanywhere can't have slightly different behavior
than the default if justified (since there is no backwards compatibility to
worry about there), but as I mentioned earlier, I think in this case,
Sequel's default behavior makes sense.
Thanks,
Jeremy
--
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sequel-talk.
For more options, visit https://groups.google.com/groups/opt_out.
