From: Moritz Onken <onken@netcubed.de>
Date: Wed, 2 Feb 2011 11:58:39 +0000 (+0100)
Subject: Make Pg producer consistent with the rest in terms of quoting
X-Git-Tag: v0.11008~18
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=5f31ed66f75751dbf891ad5cd5e5a933d64dfd29;p=dbsrgits%2FSQL-Translator.git

Make Pg producer consistent with the rest in terms of quoting
and allowing functions in constraints and indices
---

diff --git a/Changes b/Changes
index 62509d0..3f13aba 100644
--- a/Changes
+++ b/Changes
@@ -1,4 +1,7 @@
+
 * Correct postgis geography type insertion and linebreak fix for multiple geometry/geography columns
+* made PostgreSQL producer consistent with other producers in terms of
+  quoting and allowing functions in constraints and indices
 
 # ----------------------------------------------------------
 # 0.11007 2010-11-30
diff --git a/lib/SQL/Translator/Producer/PostgreSQL.pm b/lib/SQL/Translator/Producer/PostgreSQL.pm
index 4029c5d..87fa694 100644
--- a/lib/SQL/Translator/Producer/PostgreSQL.pm
+++ b/lib/SQL/Translator/Producer/PostgreSQL.pm
@@ -131,7 +131,6 @@ my %reserved = map { $_, 1 } qw[
 # my $max_id_length    = 62;
 my %used_identifiers = ();
 my %global_names;
-my %unreserve;
 my %truncated;
 
 =pod
@@ -232,12 +231,6 @@ sub produce {
             warn "Truncated " . keys( %truncated ) . " names:\n";
             warn "\t" . join( "\n\t", sort keys %truncated ) . "\n";
         }
-
-        if ( %unreserve ) {
-            warn "Encounted " . keys( %unreserve ) .
-                " unsafe names in schema (reserved or invalid):\n";
-            warn "\t" . join( "\n\t", sort keys %unreserve ) . "\n";
-        }
     }
 
     return wantarray
@@ -285,27 +278,6 @@ sub mk_name {
 }
 
 # -------------------------------------------------------------------
-sub unreserve {
-    my $name            = shift || '';
-    my $schema_obj_name = shift || '';
-
-    my ( $suffix ) = ( $name =~ s/(\W.*)$// ) ? $1 : '';
-
-    # also trap fields that don't begin with a letter
-    return $name if (!$reserved{ uc $name }) && $name =~ /^[a-z]/i; 
-
-    if ( $schema_obj_name ) {
-        ++$unreserve{"$schema_obj_name.$name"};
-    }
-    else {
-        ++$unreserve{"$name (table name)"};
-    }
-
-    my $unreserve = sprintf '%s_', $name;
-    return $unreserve.$suffix;
-}
-
-# -------------------------------------------------------------------
 sub next_unused_name {
     my $orig_name = shift or return;
     my $name      = $orig_name;
@@ -349,8 +321,8 @@ sub create_table
     my $table_name = $table->name or next;
     my ( $fql_tbl_name ) = ( $table_name =~ s/\W(.*)$// ) ? $1 : q{};
     my $table_name_ur = $qt ? $table_name
-        : $fql_tbl_name ? join('.', $table_name, unreserve($fql_tbl_name))
-        : unreserve($table_name);
+        : $fql_tbl_name ? join('.', $table_name, $fql_tbl_name)
+        : $table_name;
     $table->name($table_name_ur);
 
 # print STDERR "$table_name table_name\n";
@@ -499,13 +471,11 @@ sub create_view {
 
         $field_name_scope{$table_name} ||= {};
         my $field_name    = $field->name;
-        my $field_name_ur = $qf ? $field_name : unreserve($field_name, $table_name );
-        $field->name($field_name_ur);
         my $field_comments = $field->comments 
             ? "-- " . $field->comments . "\n  " 
             : '';
 
-        my $field_def     = $field_comments.qq[$qf$field_name_ur$qf];
+        my $field_def     = $field_comments.qq[$qf$field_name$qf];
 
         #
         # Datatype
@@ -603,7 +573,6 @@ sub create_index
     my $qt = $options->{quote_table_names} ||'';
     my $qf = $options->{quote_field_names} ||'';
     my $table_name = $index->table->name;
-#        my $table_name_ur = $qt ? unreserve($table_name) : $table_name;
 
     my ($index_def, @constraint_defs);
 
@@ -613,26 +582,20 @@ sub create_index
     );
 
     my $type = $index->type || NORMAL;
-    my @fields     = 
-        map { $_ =~ s/\(.+\)//; $_ }
-    map { $qt ? $_ : unreserve($_, $table_name ) }
-    $index->fields;
+    my @fields     =  $index->fields;
     next unless @fields;
 
-    my $def_start = qq[CONSTRAINT "$name" ];
+    my $def_start = qq[CONSTRAINT ${qf}$name${qf} ];
+    my $field_names = '(' . join(", ", (map { $_ =~ /\(.*\)/ ? $_ : ($qf . $_ . $qf ) } @fields)) . ')';
     if ( $type eq PRIMARY_KEY ) {
-        push @constraint_defs, "${def_start}PRIMARY KEY ".
-            '(' .$qf . join( $qf. ', '.$qf, @fields ) . $qf . ')';
+        push @constraint_defs, "${def_start}PRIMARY KEY ".$field_names;
     }
     elsif ( $type eq UNIQUE ) {
-        push @constraint_defs, "${def_start}UNIQUE " .
-            '(' . $qf . join( $qf. ', '.$qf, @fields ) . $qf.')';
+        push @constraint_defs, "${def_start}UNIQUE " .$field_names;
     }
     elsif ( $type eq NORMAL ) {
         $index_def = 
-            "CREATE INDEX ${qf}${name}${qf} on ${qt}${table_name}${qt} (".
-            join( ', ', map { qq[$qf$_$qf] } @fields ).  
-            ')'
+            "CREATE INDEX ${qf}${name}${qf} on ${qt}${table_name}${qt} ".$field_names
             ; 
     }
     else {
@@ -657,34 +620,25 @@ sub create_constraint
         $name = next_unused_name($name);
     }
 
-    my @fields     = 
-        map { $_ =~ s/\(.+\)//; $_ }
-    map { $qt ? $_ : unreserve( $_, $table_name )}
-    $c->fields;
+    my @fields = grep { defined } $c->fields;
 
-    my @rfields     = 
-        map { $_ =~ s/\(.+\)//; $_ }
-    map { $qt ? $_ : unreserve( $_, $table_name )}
-    $c->reference_fields;
+    my @rfields = grep { defined } $c->reference_fields;
 
     next if !@fields && $c->type ne CHECK_C;
-    my $def_start = $name ? qq[CONSTRAINT "$name" ] : '';
+    my $def_start = $name ? qq[CONSTRAINT ${qf}$name${qf} ] : '';
+    my $field_names = '(' . join(", ", (map { $_ =~ /\(.*\)/ ? $_ : ($qf . $_ . $qf ) } @fields)) . ')';
     if ( $c->type eq PRIMARY_KEY ) {
-        push @constraint_defs, "${def_start}PRIMARY KEY ".
-            '('.$qf . join( $qf.', '.$qf, @fields ) . $qf.')';
+        push @constraint_defs, "${def_start}PRIMARY KEY ".$field_names;
     }
     elsif ( $c->type eq UNIQUE ) {
-        $name = next_unused_name($name);
-        push @constraint_defs, "${def_start}UNIQUE " .
-            '('.$qf . join( $qf.', '.$qf, @fields ) . $qf.')';
+        push @constraint_defs, "${def_start}UNIQUE " .$field_names;
     }
     elsif ( $c->type eq CHECK_C ) {
         my $expression = $c->expression;
         push @constraint_defs, "${def_start}CHECK ($expression)";
     }
     elsif ( $c->type eq FOREIGN_KEY ) {
-        my $def .= "ALTER TABLE ${qt}${table_name}${qt} ADD FOREIGN KEY (" . 
-            join( ', ', map { qq[$qf$_$qf] } @fields ) . ')' .
+        my $def .= "ALTER TABLE ${qt}${table_name}${qt} ADD FOREIGN KEY " . $field_names .
             "\n  REFERENCES " . $qt . $c->reference_table . $qt;
 
         if ( @rfields ) {
diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t
index 052f58d..83acc2e 100644
--- a/t/30sqlt-new-diff-pgsql.t
+++ b/t/30sqlt-new-diff-pgsql.t
@@ -78,14 +78,14 @@ ALTER TABLE person ALTER COLUMN iq TYPE bigint;
 
 ALTER TABLE person RENAME COLUMN description TO physical_description;
 
-ALTER TABLE person ADD CONSTRAINT "unique_name" UNIQUE (name);
+ALTER TABLE person ADD CONSTRAINT unique_name UNIQUE (name);
 
 ALTER TABLE employee ADD FOREIGN KEY (employee_id)
   REFERENCES person (person_id) DEFERRABLE;
 
-ALTER TABLE person ADD CONSTRAINT "UC_person_id" UNIQUE (person_id);
+ALTER TABLE person ADD CONSTRAINT UC_person_id UNIQUE (person_id);
 
-ALTER TABLE person ADD CONSTRAINT "UC_age_name" UNIQUE (age, name);
+ALTER TABLE person ADD CONSTRAINT UC_age_name UNIQUE (age, name);
 
 DROP TABLE deleted CASCADE;
 
@@ -133,9 +133,9 @@ ALTER TABLE person ALTER COLUMN iq TYPE bigint;
 
 ALTER TABLE person RENAME COLUMN description TO physical_description;
 
-ALTER TABLE person ADD CONSTRAINT "UC_person_id" UNIQUE (person_id);
+ALTER TABLE person ADD CONSTRAINT UC_person_id UNIQUE (person_id);
 
-ALTER TABLE person ADD CONSTRAINT "UC_age_name" UNIQUE (age, name);
+ALTER TABLE person ADD CONSTRAINT UC_age_name UNIQUE (age, name);
 
 DROP TABLE deleted CASCADE;
 
diff --git a/t/47postgres-producer.t b/t/47postgres-producer.t
index 01969d7..547ddf2 100644
--- a/t/47postgres-producer.t
+++ b/t/47postgres-producer.t
@@ -6,7 +6,7 @@ use warnings;
 use Test::More;
 use Test::Exception;
 use Test::SQL::Translator qw(maybe_plan);
-
+use SQL::Translator::Schema::Constants;
 use Data::Dumper;
 use FindBin qw/$Bin/;
 
@@ -14,7 +14,7 @@ use FindBin qw/$Bin/;
 #=============================================================================
 
 BEGIN {
-    maybe_plan(26,
+    maybe_plan(38,
         'SQL::Translator::Producer::PostgreSQL',
         'Test::Differences',
     )
@@ -382,3 +382,56 @@ my $view2_sql_replace = "CREATE TEMPORARY VIEW view_foo2 AS
     SELECT id, name FROM thing
  WITH CASCADED CHECK OPTION";
 is($view2_sql1, $view2_sql_replace, 'correct "CREATE OR REPLACE VIEW" SQL 2');
+
+{
+    my $table = SQL::Translator::Schema::Table->new( name => 'foobar', fields => [qw( foo  bar )] );
+    my $quote = { quote_table_names => '"', quote_field_names => '"' };
+
+    {
+        my $index = $table->add_index(name => 'myindex', fields => ['foo']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index);
+        is($def, "CREATE INDEX myindex on foobar (foo)", 'index created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index, $quote);
+        is($def, 'CREATE INDEX "myindex" on "foobar" ("foo")', 'index created w/ quotes');
+    }
+
+    {
+        my $index = $table->add_index(name => 'myindex', fields => ['lower(foo)']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index);
+        is($def, "CREATE INDEX myindex on foobar (lower(foo))", 'index created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index, $quote);
+        is($def, 'CREATE INDEX "myindex" on "foobar" (lower(foo))', 'index created w/ quotes');
+    }
+
+    {
+        my $index = $table->add_index(name => 'myindex', fields => ['bar', 'lower(foo)']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index);
+        is($def, "CREATE INDEX myindex on foobar (bar, lower(foo))", 'index created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_index($index, $quote);
+        is($def, 'CREATE INDEX "myindex" on "foobar" ("bar", lower(foo))', 'index created w/ quotes');
+    }
+
+    {
+        my $constr = $table->add_constraint(name => 'constr', type => UNIQUE, fields => ['foo']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr);
+        is($def->[0], 'CONSTRAINT constr UNIQUE (foo)', 'constraint created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr, $quote);
+        is($def->[0], 'CONSTRAINT "constr" UNIQUE ("foo")', 'constraint created w/ quotes');
+    }
+
+    {
+        my $constr = $table->add_constraint(name => 'constr', type => UNIQUE, fields => ['lower(foo)']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr);
+        is($def->[0], 'CONSTRAINT constr UNIQUE (lower(foo))', 'constraint created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr, $quote);
+        is($def->[0], 'CONSTRAINT "constr" UNIQUE (lower(foo))', 'constraint created w/ quotes');
+    }
+
+    {
+        my $constr = $table->add_constraint(name => 'constr', type => UNIQUE, fields => ['bar', 'lower(foo)']);
+        my ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr);
+        is($def->[0], 'CONSTRAINT constr UNIQUE (bar, lower(foo))', 'constraint created');
+        ($def) = SQL::Translator::Producer::PostgreSQL::create_constraint($constr, $quote);
+        is($def->[0], 'CONSTRAINT "constr" UNIQUE ("bar", lower(foo))', 'constraint created w/ quotes');
+    }
+}
diff --git a/t/63-spacial-pgsql.t b/t/63-spacial-pgsql.t
index 64fb313..0cbc8fe 100644
--- a/t/63-spacial-pgsql.t
+++ b/t/63-spacial-pgsql.t
@@ -52,9 +52,9 @@ is($field1_geocol, "INSERT INTO geometry_columns VALUES ('','myschema','mytable'
 
 my $field1_geocon = SQL::Translator::Producer::PostgreSQL::add_geometry_constraints($field1);
 
-is($field1_geocon, qq[ALTER TABLE mytable ADD CONSTRAINT "enforce_dims_myfield" CHECK ((st_ndims(myfield) = 2))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_srid_myfield" CHECK ((st_srid(myfield) = -1))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_geotype_myfield" CHECK ((geometrytype(myfield) = 'POINT'::text OR myfield IS NULL))],
+is($field1_geocon, qq[ALTER TABLE mytable ADD CONSTRAINT enforce_dims_myfield CHECK ((st_ndims(myfield) = 2))
+ALTER TABLE mytable ADD CONSTRAINT enforce_srid_myfield CHECK ((st_srid(myfield) = -1))
+ALTER TABLE mytable ADD CONSTRAINT enforce_geotype_myfield CHECK ((geometrytype(myfield) = 'POINT'::text OR myfield IS NULL))],
  'Add geometry constraints works');
 
 my $field2 = SQL::Translator::Schema::Field->new( name      => 'myfield',
@@ -82,9 +82,9 @@ my $alter_field2 = SQL::Translator::Producer::PostgreSQL::alter_field($field2,
 is($alter_field2, qq[ALTER TABLE mytable ALTER COLUMN myfield DROP NOT NULL
 ALTER TABLE mytable ALTER COLUMN myfield TYPE geometry
 INSERT INTO geometry_columns VALUES ('','myschema','mytable','myfield','2','-1','POINT')
-ALTER TABLE mytable ADD CONSTRAINT "enforce_dims_myfield" CHECK ((st_ndims(myfield) = 2))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_srid_myfield" CHECK ((st_srid(myfield) = -1))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_geotype_myfield" CHECK ((geometrytype(myfield) = 'POINT'::text OR myfield IS NULL))],
+ALTER TABLE mytable ADD CONSTRAINT enforce_dims_myfield CHECK ((st_ndims(myfield) = 2))
+ALTER TABLE mytable ADD CONSTRAINT enforce_srid_myfield CHECK ((st_srid(myfield) = -1))
+ALTER TABLE mytable ADD CONSTRAINT enforce_geotype_myfield CHECK ((geometrytype(myfield) = 'POINT'::text OR myfield IS NULL))],
  'Alter field non geometry to geometry works');
 
 $field1->name('field3');
@@ -92,9 +92,9 @@ my $add_field = SQL::Translator::Producer::PostgreSQL::add_field($field1);
 
 is($add_field, qq[ALTER TABLE mytable ADD COLUMN field3 geometry
 INSERT INTO geometry_columns VALUES ('','myschema','mytable','field3','2','-1','POINT')
-ALTER TABLE mytable ADD CONSTRAINT "enforce_dims_field3" CHECK ((st_ndims(field3) = 2))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_srid_field3" CHECK ((st_srid(field3) = -1))
-ALTER TABLE mytable ADD CONSTRAINT "enforce_geotype_field3" CHECK ((geometrytype(field3) = 'POINT'::text OR field3 IS NULL))],
+ALTER TABLE mytable ADD CONSTRAINT enforce_dims_field3 CHECK ((st_ndims(field3) = 2))
+ALTER TABLE mytable ADD CONSTRAINT enforce_srid_field3 CHECK ((st_srid(field3) = -1))
+ALTER TABLE mytable ADD CONSTRAINT enforce_geotype_field3 CHECK ((geometrytype(field3) = 'POINT'::text OR field3 IS NULL))],
  'Add geometry field works');
 
 my $drop_field = SQL::Translator::Producer::PostgreSQL::drop_field($field1);
@@ -124,9 +124,9 @@ is($create_table,qq[--
 CREATE TABLE mytable (
   field3 geometry,
   field4 geography(POINT,-1),
-  CONSTRAINT "enforce_dims_field3" CHECK ((st_ndims(field3) = 2)),
-  CONSTRAINT "enforce_srid_field3" CHECK ((st_srid(field3) = -1)),
-  CONSTRAINT "enforce_geotype_field3" CHECK ((geometrytype(field3) = 'POINT'::text OR field3 IS NULL))
+  CONSTRAINT enforce_dims_field3 CHECK ((st_ndims(field3) = 2)),
+  CONSTRAINT enforce_srid_field3 CHECK ((st_srid(field3) = -1)),
+  CONSTRAINT enforce_geotype_field3 CHECK ((geometrytype(field3) = 'POINT'::text OR field3 IS NULL))
 );
 INSERT INTO geometry_columns VALUES ('','myschema','mytable','field3','2','-1','POINT')],'Create table with geometry works.');