Constraint/index name fix from rdj

diff --git a/Changes b/Changes
index 3d391bb..3516d74 100644 (file)
--- a/Changes
+++ b/Changes
@@ -7,6 +7,8 @@ Revision history for DBIx::Class
         - sqltargs respected correctly in deploy et al.
         - Added support for savepoints, and using them automatically in
           nested transactions if auto_savepoint is set in connect_info.
+        - Changed naming scheme for constraints and keys in the sqlt parser;
+          names should now be consistent and collision-free.
 
 0.08010 2008-03-01 10:30
         - Fix t/94versioning.t so it passes with latest SQL::Translator

diff --git a/Makefile.PL b/Makefile.PL
index f8ccc29..f45d51e 100644 (file)
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -18,6 +18,7 @@ requires 'Class::Inspector'          => 0;
 requires 'Class::Accessor::Grouped'  => 0.05002;
 requires 'JSON::Any'                 => 1.00; 
 requires 'Scope::Guard'              => 0.03;
+requires 'Digest::SHA1'              => 2.00;
 
 # Perl 5.8.0 doesn't have utf8::is_utf8()
 requires 'Encode'                    => 0 if ($] <= 5.008000);  

diff --git a/lib/DBIx/Class.pm b/lib/DBIx/Class.pm
index c8dbd36..9147f46 100644 (file)
--- a/lib/DBIx/Class.pm
+++ b/lib/DBIx/Class.pm
@@ -268,6 +268,8 @@ phaylon: Robert Sedlacek <phaylon@dunkelheit.at>
 
 quicksilver: Jules Bean
 
+rdj: Ryan D Johnson <ryan@innerfence.com>
+
 sc_: Just Another Perl Hacker
 
 scotty: Scotty Allen <scotty@scottyallen.com>

diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index bfdaece..2c45eee 100644 (file)
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -1538,6 +1538,7 @@ sub create_ddl_dir
           unless $dest_schema->name;
       }
 
+      $DB::single = 1;
       my $diff = SQL::Translator::Diff::schema_diff($source_schema, $db,
                                                     $dest_schema,   $db,
                                                     $sqltargs

diff --git a/lib/SQL/Translator/Parser/DBIx/Class.pm b/lib/SQL/Translator/Parser/DBIx/Class.pm
index dc18bbb..1b6266f 100644 (file)
--- a/lib/SQL/Translator/Parser/DBIx/Class.pm
+++ b/lib/SQL/Translator/Parser/DBIx/Class.pm
@@ -14,6 +14,7 @@ $DEBUG = 0 unless defined $DEBUG;
 
 use Exporter;
 use Data::Dumper;
+use Digest::SHA1 qw( sha1_hex );
 use SQL::Translator::Utils qw(debug normalize_name);
 
 use base qw(Exporter);
@@ -107,13 +108,12 @@ sub parse {
             if (!$source->compare_relationship_keys($unique_constraints{$uniq}, \@primary)) {
                 $table->add_constraint(
                             type             => 'unique',
-                            name             => "$uniq",
+                            name             => _create_unique_symbol($uniq),
                             fields           => $unique_constraints{$uniq}
                 );
 
                my $index = $table->add_index(
-                            # TODO: Pick a better than that wont conflict
-                            name   => $unique_constraints{$uniq}->[0],
+                            name   => _create_unique_symbol(join('_', @{$unique_constraints{$uniq}})),
                             fields => $unique_constraints{$uniq},
                             type   => 'NORMAL',
                );
@@ -179,7 +179,9 @@ sub parse {
                 if (scalar(@keys)) {
                   $table->add_constraint(
                                     type             => 'foreign_key',
-                                    name             => $table->name . "_fk_$keys[0]",
+                                    name             => _create_unique_symbol($table->name
+                                                                            . '_fk_'
+                                                                            . join('_', @keys)),
                                     fields           => \@keys,
                                     reference_fields => \@refkeys,
                                     reference_table  => $rel_table,
@@ -189,7 +191,7 @@ sub parse {
                   );
                     
                   my $index = $table->add_index(
-                                    name   => join('_', @keys),
+                                    name   => _create_unique_symbol(join('_', @keys)),
                                     fields => \@keys,
                                     type   => 'NORMAL',
                   );
@@ -209,5 +211,31 @@ sub parse {
     return 1;
 }
 
-1;
+# TODO - is there a reasonable way to pass configuration?
+# Default of 64 comes from mysql's limit.
+our $MAX_SYMBOL_LENGTH    ||= 64;
+our $COLLISION_TAG_LENGTH ||= 8;
+
+# -------------------------------------------------------------------
+# $resolved_name = _create_unique_symbol($desired_name)
+#
+# If desired_name is really long, it will be truncated in a way that
+# has a high probability of leaving it unique.
+# -------------------------------------------------------------------
+sub _create_unique_symbol {
+    my $desired_name = shift;
+    return $desired_name if length $desired_name <= $MAX_SYMBOL_LENGTH;
 
+    my $truncated_name = substr $desired_name, 0, $MAX_SYMBOL_LENGTH - $COLLISION_TAG_LENGTH - 1;
+
+    # Hex isn't the most space-efficient, but it skirts around allowed
+    # charset issues
+    my $digest = sha1_hex($desired_name);
+    my $collision_tag = substr $digest, 0, $COLLISION_TAG_LENGTH;
+
+    return $truncated_name
+         . '_'
+         . $collision_tag;
+}
+
+1;

diff --git a/t/86sqlt.t b/t/86sqlt.t
index 987ee39..513a9b6 100644 (file)
--- a/t/86sqlt.t
+++ b/t/86sqlt.t
@@ -10,7 +10,7 @@ plan skip_all => 'SQL::Translator required' if $@;
 
 my $schema = DBICTest->init_schema;
 
-plan tests => 77;
+plan tests => 160;
 
 my $translator = SQL::Translator->new( 
   parser_args => {
@@ -40,12 +40,14 @@ my %fk_constraints = (
   twokeys => [
     {
       'display' => 'twokeys->cd',
+      'name' => 'twokeys_fk_cd', 'index_name' => 'cd',
       'selftable' => 'twokeys', 'foreigntable' => 'cd', 
       'selfcols'  => ['cd'], 'foreigncols' => ['cdid'], 
       on_delete => '', on_update => '', deferrable => 0,
     },
     {
       'display' => 'twokeys->artist',
+      'name' => 'twokeys_fk_artist', 'index_name' => 'artist',
       'selftable' => 'twokeys', 'foreigntable' => 'artist', 
       'selfcols'  => ['artist'], 'foreigncols' => ['artistid'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
@@ -56,12 +58,14 @@ my %fk_constraints = (
   fourkeys_to_twokeys => [
     {
       'display' => 'fourkeys_to_twokeys->twokeys',
+      'name' => 'fourkeys_to_twokeys_fk_t_cd_t_artist', 'index_name' => 't_cd_t_artist',
       'selftable' => 'fourkeys_to_twokeys', 'foreigntable' => 'twokeys', 
       'selfcols'  => ['t_artist', 't_cd'], 'foreigncols' => ['artist', 'cd'], 
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
     },
     {
-      'display' => 'fourkeys_to_twokeys->fourkeys',
+      'display' => 'fourkeys_to_twokeys->fourkeys', 'index_name' => 'f_foo_f_goodbye_f_hello_f_bar',
+      'name' => 'fourkeys_to_twokeys_fk_f_foo_f_goodbye_f_hello_f_bar',
       'selftable' => 'fourkeys_to_twokeys', 'foreigntable' => 'fourkeys', 
       'selfcols'  => [qw(f_foo f_bar f_hello f_goodbye)],
       'foreigncols' => [qw(foo bar hello goodbye)], 
@@ -73,12 +77,14 @@ my %fk_constraints = (
   cd_to_producer => [
     {
       'display' => 'cd_to_producer->cd',
+      'name' => 'cd_to_producer_fk_cd', 'index_name' => 'cd',
       'selftable' => 'cd_to_producer', 'foreigntable' => 'cd', 
       'selfcols'  => ['cd'], 'foreigncols' => ['cdid'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
     },
     {
       'display' => 'cd_to_producer->producer',
+      'name' => 'cd_to_producer_fk_producer', 'index_name' => 'producer',
       'selftable' => 'cd_to_producer', 'foreigntable' => 'producer', 
       'selfcols'  => ['producer'], 'foreigncols' => ['producerid'],
       on_delete => '', on_update => '', deferrable => 1,
@@ -89,12 +95,14 @@ my %fk_constraints = (
   self_ref_alias => [
     {
       'display' => 'self_ref_alias->self_ref for self_ref',
+      'name' => 'self_ref_alias_fk_self_ref', 'index_name' => 'self_ref',
       'selftable' => 'self_ref_alias', 'foreigntable' => 'self_ref', 
       'selfcols'  => ['self_ref'], 'foreigncols' => ['id'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
     },
     {
       'display' => 'self_ref_alias->self_ref for alias',
+      'name' => 'self_ref_alias_fk_alias', 'index_name' => 'alias',
       'selftable' => 'self_ref_alias', 'foreigntable' => 'self_ref', 
       'selfcols'  => ['alias'], 'foreigncols' => ['id'],
       on_delete => '', on_update => '', deferrable => 1,
@@ -105,6 +113,7 @@ my %fk_constraints = (
   cd => [
     {
       'display' => 'cd->artist',
+      'name' => 'cd_fk_artist', 'index_name' => 'artist',
       'selftable' => 'cd', 'foreigntable' => 'artist', 
       'selfcols'  => ['artist'], 'foreigncols' => ['artistid'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
@@ -115,12 +124,14 @@ my %fk_constraints = (
   artist_undirected_map => [
     {
       'display' => 'artist_undirected_map->artist for id1',
+      'name' => 'artist_undirected_map_fk_id1', 'index_name' => 'id1',
       'selftable' => 'artist_undirected_map', 'foreigntable' => 'artist', 
       'selfcols'  => ['id1'], 'foreigncols' => ['artistid'],
       on_delete => 'CASCADE', on_update => '', deferrable => 1,
     },
     {
       'display' => 'artist_undirected_map->artist for id2',
+      'name' => 'artist_undirected_map_fk_id2', 'index_name' => 'id2',
       'selftable' => 'artist_undirected_map', 'foreigntable' => 'artist', 
       'selfcols'  => ['id2'], 'foreigncols' => ['artistid'],
       on_delete => 'CASCADE', on_update => '', deferrable => 1,
@@ -131,6 +142,7 @@ my %fk_constraints = (
   track => [
     {
       'display' => 'track->cd',
+      'name' => 'track_fk_cd', 'index_name' => 'cd',
       'selftable' => 'track', 'foreigntable' => 'cd', 
       'selfcols'  => ['cd'], 'foreigncols' => ['cdid'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
@@ -141,6 +153,7 @@ my %fk_constraints = (
   treelike => [
     {
       'display' => 'treelike->treelike for parent',
+      'name' => 'treelike_fk_parent', 'index_name' => 'parent',
       'selftable' => 'treelike', 'foreigntable' => 'treelike', 
       'selfcols'  => ['parent'], 'foreigncols' => ['id'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
@@ -151,6 +164,7 @@ my %fk_constraints = (
   twokeytreelike => [
     {
       'display' => 'twokeytreelike->twokeytreelike for parent1,parent2',
+      'name' => 'twokeytreelike_fk_parent1_parent2', 'index_name' => 'parent1_parent2',
       'selftable' => 'twokeytreelike', 'foreigntable' => 'twokeytreelike', 
       'selfcols'  => ['parent1', 'parent2'], 'foreigncols' => ['id1','id2'],
       on_delete => '', on_update => '', deferrable => 1,
@@ -161,6 +175,7 @@ my %fk_constraints = (
   tags => [
     {
       'display' => 'tags->cd',
+      'name' => 'tags_fk_cd', 'index_name' => 'cd',
       'selftable' => 'tags', 'foreigntable' => 'cd', 
       'selfcols'  => ['cd'], 'foreigncols' => ['cdid'],
       on_delete => 'CASCADE', on_update => 'CASCADE', deferrable => 1,
@@ -171,6 +186,7 @@ my %fk_constraints = (
   bookmark => [
     {
       'display' => 'bookmark->link',
+      'name' => 'bookmark_fk_link', 'index_name' => 'link',
       'selftable' => 'bookmark', 'foreigntable' => 'link', 
       'selfcols'  => ['link'], 'foreigncols' => ['id'],
       on_delete => '', on_update => '', deferrable => 1,
@@ -180,12 +196,42 @@ my %fk_constraints = (
   forceforeign => [
     {
       'display' => 'forceforeign->artist',
+      'name' => 'forceforeign_fk_artist', 'index_name' => 'artist',
       'selftable' => 'forceforeign', 'foreigntable' => 'artist', 
       'selfcols'  => ['artist'], 'foreigncols' => ['artist_id'], 
       on_delete => '', on_update => '', deferrable => 1,
     },
   ],
 
+  # LongColumns
+  long_columns => [
+    {
+      'display' => 'long_columns->owner',
+      'name' => 'long_columns_fk_64_character_column_aaaaaaaaaaaaaaaaaaa_1ca973e2',
+      'index_name' => '64_character_column_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
+      'selftable' => 'long_columns', 'foreigntable' => 'long_columns',
+      'selfcols' => ['64_character_column_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'],
+      'foreigncols' => ['lcid'],
+      on_delete => '', on_update => '', deferrable => 1,
+    },
+    {
+      'display' => 'long_columns->owner2',
+      'name' => 'long_columns_fk_32_character_column_aaaaaaaaaaaa_32_cha_6060a8f3',
+      'index_name' => '32_character_column_aaaaaaaaaaaa_32_character_column_bb_30f7a7fe',
+      'selftable' => 'long_columns', 'foreigntable' => 'long_columns',
+      'selfcols' => ['32_character_column_bbbbbbbbbbbb', '32_character_column_aaaaaaaaaaaa'],
+      'foreigncols' => ['32_character_column_aaaaaaaaaaaa', '32_character_column_bbbbbbbbbbbb'],
+      on_delete => '', on_update => '', deferrable => 1,
+    },
+    {
+      'display' => 'long_columns->owner3',
+      'name' => 'long_columns_fk_16_character_col',
+      'index_name' => '16_character_col',
+      'selftable' => 'long_columns', 'foreigntable' => 'long_columns',
+      'selfcols' => ['16_character_col'], 'foreigncols' => ['8_char_c'],
+      on_delete => '', on_update => '', deferrable => 1,
+    },
+  ],
 );
 
 my %unique_constraints = (
@@ -193,6 +239,7 @@ my %unique_constraints = (
   cd => [
     {
       'display' => 'cd artist and title unique',
+      'name' => 'cd_artist_title',
       'table' => 'cd', 'cols' => ['artist', 'title'],
     },
   ],
@@ -201,14 +248,39 @@ my %unique_constraints = (
   producer => [
     {
       'display' => 'producer name unique',
+      'name' => 'prod_name', # explicit name
       'table' => 'producer', 'cols' => ['name'],
     },
   ],
 
+  long_columns => [
+    {
+      'display' => 'long but not quite truncated unique',
+      'name' => 'long_columns_16_character_col_32_character_column_aaaaaaaaaaaa',
+      'table' => 'long_columns', 'cols' => [qw( 32_character_column_aaaaaaaaaaaa 16_character_col )],
+    },
+    {
+      'display' => 'multi column truncated unique',
+      'name' => 'long_columns_8_char_c_16_character_col_32_character_col_ee4a438c',
+      'table' => 'long_columns', 'cols' => [qw( 32_character_column_aaaaaaaaaaaa 16_character_col 8_char_c )],
+    },
+    {
+      'display' => 'different multi column truncated unique with same base',
+      'name' => 'long_columns_8_char_c_16_character_col_32_character_col_c5dbc7a7',
+      'table' => 'long_columns', 'cols' => [qw( 32_character_column_bbbbbbbbbbbb 16_character_col 8_char_c )],
+    },
+    {
+      'display' => 'single column truncated unique',
+      'name' => 'long_columns_64_character_column_aaaaaaaaaaaaaaaaaaaaaa_095dc664',
+      'table' => 'long_columns', 'cols' => ['64_character_column_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'],
+    },
+  ],
+
   # TwoKeyTreeLike
   twokeytreelike => [
     {
       'display' => 'twokeytreelike name unique',
+      'name' => 'tktlnameunique', # explicit name
       'table' => 'twokeytreelike', 'cols'  => ['name'],
     },
   ],
@@ -218,6 +290,7 @@ my %unique_constraints = (
 #  employee => [
 #    {
 #      'display' => 'employee position and group_id unique',
+#      'name' => 'position_group',
 #      'table' => 'employee', cols => ['position', 'group_id'],
 #    },
 #  ],
@@ -264,6 +337,7 @@ for my $expected_constraints (keys %unique_constraints) {
       'UNIQUE', $expected_constraint->{table}, $expected_constraint->{cols},
     );
     ok( defined($constraint), "UNIQUE constraint matching `$desc' found" );
+    test_unique($expected_constraint, $constraint);
   }
 }
 
@@ -355,10 +429,23 @@ sub get_index {
 sub test_fk {
   my ($expected, $got) = @_;
   my $desc = $expected->{display};
+  is( $got->name, $expected->{name},
+      "name parameter correct for `$desc'" );
   is( $got->on_delete, $expected->{on_delete},
       "on_delete parameter correct for `$desc'" );
   is( $got->on_update, $expected->{on_update},
       "on_update parameter correct for `$desc'" );
   is( $got->deferrable, $expected->{deferrable},
       "is_deferrable parameter correct for `$desc'" );
+
+  my $index = get_index( $got->table, { fields => $expected->{selfcols} } );
+  ok( defined $index, "index exists for `$desc'" );
+  is( $index->name, $expected->{index_name}, "index has correct name for `$desc'" );
+}
+
+sub test_unique {
+  my ($expected, $got) = @_;
+  my $desc = $expected->{display};
+  is( $got->name, $expected->{name},
+      "name parameter correct for `$desc'" );
 }

diff --git a/t/lib/DBICTest/Schema.pm b/t/lib/DBICTest/Schema.pm
index e3329a7..a07db60 100644 (file)
--- a/t/lib/DBICTest/Schema.pm
+++ b/t/lib/DBICTest/Schema.pm
@@ -38,7 +38,8 @@ __PACKAGE__->load_classes(qw/
   qw/SelfRefAlias TreeLike TwoKeyTreeLike Event EventTZ NoPrimaryKey/,
   qw/Collection CollectionObject TypedObject/,
   qw/Owners BooksInLibrary/,
-  qw/ForceForeign/  
+  qw/ForceForeign/,
+  qw/LongColumns/,
 );
 
 sub sqlt_deploy_hook {