From ca6c4d533d6904fc60cb53b98445fa9e584e0447 Mon Sep 17 00:00:00 2001 From: Michael Conrad Date: Tue, 3 Jan 2023 21:32:46 -0500 Subject: [PATCH 1/3] Fix default values returned by Parser::DBI::PostgreSQL The postgres function pg_get_expr used in reading the default values for columns always returns a properly quoted DDL expression. This means that the default values written to $table->add_field should always be scalar-refs, unless some further step were taken to parse the DDL for the case of simple strings. As it was (for the last 17 years?!) the parsed default would be a string of SQL and if passed back to a producer it would attempt to quote that SQL as a string value and, in almost all cases, fail. This was even wrong in the unit tests: is( $f3->default_value, "'FOO'::text", 'Default value is "FOO"' ); When written back to the database, 'FOO'::text gets further wrapped in quotes like '''FOO''::text' resulting in the wrong default value. for any other data type, it simply fails to create thetable. --- lib/SQL/Translator/Parser/DBI/PostgreSQL.pm | 3 ++- t/66-postgres-dbi-parser.t | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm index c751fc5d9..caf48c431 100644 --- a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm +++ b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm @@ -127,7 +127,8 @@ ORDER BY 1; my $col = $table->add_field( name => $$columnhash{'attname'}, - default_value => $$columnhash{'adsrc'}, + (!$$columnhash{'atthasdef'}? () + : ( default_value => \$$columnhash{'adsrc'} )), data_type => $$columnhash{'typname'}, order => $$columnhash{'attnum'}, ) || die $table->error; diff --git a/t/66-postgres-dbi-parser.t b/t/66-postgres-dbi-parser.t index aae516227..69779b72a 100644 --- a/t/66-postgres-dbi-parser.t +++ b/t/66-postgres-dbi-parser.t @@ -93,7 +93,7 @@ my $f1 = shift @t1_fields; is( $f1->name, 'f_serial', 'First field is "f_serial"' ); is( $f1->data_type, 'integer', 'Field is an integer' ); is( $f1->is_nullable, 0, 'Field cannot be null' ); -is( $f1->default_value, "nextval('sqlt_test1_f_serial_seq'::regclass)", 'Default value is nextval()' ); +is( ${$f1->default_value}, "nextval('sqlt_test1_f_serial_seq'::regclass)", 'Default value is nextval()' ); is( $f1->is_primary_key, 1, 'Field is PK' ); #FIXME: not set to auto-increment? maybe we can guess auto-increment behavior by looking at the default_value (i.e. it call function nextval() ) #is( $f1->is_auto_increment, 1, 'Field is auto increment' ); @@ -114,7 +114,7 @@ is( $f3->name, 'f_text', 'Third field is "f_text"' ); is( $f3->data_type, 'text', 'Field is a text' ); is( $f3->is_nullable, 1, 'Field can be null' ); is( $f3->size, 0, 'Size is 0' ); -is( $f3->default_value, "'FOO'::text", 'Default value is "FOO"' ); +is( ${$f3->default_value}, "'FOO'::text", 'Default value is "FOO"' ); is( $f3->is_primary_key, 0, 'Field is not PK' ); is( $f3->is_auto_increment, 0, 'Field is not auto increment' ); is( $f3->comments, 'this is a comment on a field of the first table', 'There is a comment on the third field'); From 8241aab834e7096b8d7312a8d09cbd0380dfcf29 Mon Sep 17 00:00:00 2001 From: Michael Conrad Date: Tue, 3 Jan 2023 22:55:01 -0500 Subject: [PATCH 2/3] Fix data_type and size returned by Parser::DBI::PostgreSQL The handling of column sizes was also broken for the postgres parser. It was returning a data_type of 'character varying(50)' with a size of '54', when it should be returning a type of 'character varying' and size of '54'. When passed back to the producer, it generates syntax errors of "character varying(50)(54)". For numeric(8,4), it was ignoring the size entirely. For reasons undiscovered, the postgres column atttypmod does not contain useful numbers to represent the size, so I just parse the size from the data_type and then remove it. This works for at least varchar and numeric columns (as shown in the unit test). It might not be a universal solution but it is at least a lot more useful than the previous broken state of things. While I was at it, I added a check for default-values that are simple unquoted numbers or simple quoted strings, and un-quote them to be more useful to the cross-database 'translator' aspect of SQL::Translator. --- lib/SQL/Translator/Parser/DBI/PostgreSQL.pm | 33 ++++++++++++++------- t/66-postgres-dbi-parser.t | 24 +++++++++++---- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm index caf48c431..95ae11cc7 100644 --- a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm +++ b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm @@ -121,21 +121,32 @@ ORDER BY 1; my %column_by_attrid; while (my $columnhash = $column_select->fetchrow_hashref ) { - - #data_type seems to not be populated; perhaps there needs to - #be a mapping of query output to reserved constants in sqlt? - + my $type = $$columnhash{'typname'}; + # For the case of character varying(50), atttypmod will be 54 and the (50) + # will be listed as part of the type. For numeric(8,5) the atttypmod will + # be a meaningless large number. To make this compatible with the + # rest of SQL::Translator, remove the size from the type and change the + # size to whatever was removed from the type. + my @size= ($type =~ s/\(([0-9,]+)\)$//)? (split /,/, $1) : (); my $col = $table->add_field( name => $$columnhash{'attname'}, - (!$$columnhash{'atthasdef'}? () - : ( default_value => \$$columnhash{'adsrc'} )), - data_type => $$columnhash{'typname'}, + data_type => $type, order => $$columnhash{'attnum'}, ) || die $table->error; - - $col->{size} = [$$columnhash{'length'}] - if $$columnhash{'length'}>0 && $$columnhash{'length'}<=0xFFFF; - $col->{is_nullable} = $$columnhash{'attnotnull'} ? 0 : 1; + $col->size(\@size) if @size; + # default values are a DDL expression. Convert the obvious ones like '...'::text + # to a plain value and let the rest be scalarrefs. + my $default= $$columnhash{'adsrc'}; + if (defined $default) { + if ($default =~ /^[0-9.]+$/) { $col->default_value($default) } + elsif ($default =~ /^'(.*?)'(::\Q$type\E)?$/) { + my $str= $1; + $str =~ s/''/'/g; + $col->default_value($1) + } + else { $col->default_value(\$default) } + } + $col->is_nullable( $$columnhash{'attnotnull'} ? 0 : 1 ); $col->comments($$columnhash{'description'}) if $$columnhash{'description'}; $column_by_attrid{$$columnhash{'attnum'}}= $$columnhash{'attname'}; } diff --git a/t/66-postgres-dbi-parser.t b/t/66-postgres-dbi-parser.t index 69779b72a..6cd481305 100644 --- a/t/66-postgres-dbi-parser.t +++ b/t/66-postgres-dbi-parser.t @@ -54,7 +54,8 @@ my $sql = q[ CREATE TABLE sqlt_products_1 ( product_no integer, name text, - price numeric + price numeric(8,4) default 0.0, + created_at timestamp without time zone default now() ); -- drop a column, to not have a linear id @@ -100,10 +101,9 @@ is( $f1->is_primary_key, 1, 'Field is PK' ); my $f2 = shift @t1_fields; is( $f2->name, 'f_varchar', 'Second field is "f_varchar"' ); -is( $f2->data_type, 'character varying(255)', 'Field is a character varying(255)' ); +is( $f2->data_type, 'character varying', 'Field is a character varying(255)' ); is( $f2->is_nullable, 1, 'Field can be null' ); -#FIXME: should not be 255? -is( $f2->size, 259, 'Size is "259"' ); +is( $f2->size, 255, 'Size is "255"' ); is( $f2->default_value, undef, 'Default value is undefined' ); is( $f2->is_primary_key, 0, 'Field is not PK' ); is( $f2->is_auto_increment, 0, 'Field is not auto increment' ); @@ -114,7 +114,7 @@ is( $f3->name, 'f_text', 'Third field is "f_text"' ); is( $f3->data_type, 'text', 'Field is a text' ); is( $f3->is_nullable, 1, 'Field can be null' ); is( $f3->size, 0, 'Size is 0' ); -is( ${$f3->default_value}, "'FOO'::text", 'Default value is "FOO"' ); +is( $f3->default_value, 'FOO', 'Default value is "FOO"' ); is( $f3->is_primary_key, 0, 'Field is not PK' ); is( $f3->is_auto_increment, 0, 'Field is not auto increment' ); is( $f3->comments, 'this is a comment on a field of the first table', 'There is a comment on the third field'); @@ -165,12 +165,26 @@ my $fk_ref1 = $t2_f3->foreign_key_reference; isa_ok( $fk_ref1, 'SQL::Translator::Schema::Constraint', 'FK' ); is( $fk_ref1->reference_table, 'sqlt_test1', 'FK is to "sqlt_test1" table' ); +my $t3 = $schema->get_table("sqlt_products_1"); +my $t3_f3= $t3->get_field('price'); +is( $t3_f3->name, 'price', 'Third field is "price"' ); +is( $t3_f3->data_type, 'numeric', 'Third field type "numeric"' ); +is_deeply( [$t3_f3->size], [8,4], 'Third field size "(8,4)"' ); +is( $t3_f3->default_value, '0.0', 'Third field default "0.0"' ); + +my $t3_f4= $t3->get_field('created_at'); +is( $t3_f4->name, 'created_at', 'fourth field is "created_at"' ); +is( $t3_f4->data_type, 'timestamp without time zone', 'type is "timestamp without time zone"' ); +is( $t2_f3->size, 0, 'Size is "0"' ); +is_deeply( $t3_f4->default_value, \"now()", 'default \\"now()"' ); + my @t2_constraints = $t2->get_constraints; is( scalar @t2_constraints, 1, "One constraint on table" ); my $t2_c1 = shift @t2_constraints; is( $t2_c1->type, FOREIGN_KEY, "Constraint is a FK" ); + $dbh->rollback; $dbh->disconnect; From 4f39271152643ca0a1b72866a3d67d75692fd9c6 Mon Sep 17 00:00:00 2001 From: Michael Conrad Date: Wed, 4 Jan 2023 01:11:18 -0500 Subject: [PATCH 3/3] Fix bug in previous and add unit test for escaped quotes The previous commit had broken handling of escaped single quote characters in a literal string default value. this fixes that and adds a unit test for it. --- lib/SQL/Translator/Parser/DBI/PostgreSQL.pm | 2 +- t/66-postgres-dbi-parser.t | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm index 95ae11cc7..2c54f7148 100644 --- a/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm +++ b/lib/SQL/Translator/Parser/DBI/PostgreSQL.pm @@ -142,7 +142,7 @@ ORDER BY 1; elsif ($default =~ /^'(.*?)'(::\Q$type\E)?$/) { my $str= $1; $str =~ s/''/'/g; - $col->default_value($1) + $col->default_value($str); } else { $col->default_value(\$default) } } diff --git a/t/66-postgres-dbi-parser.t b/t/66-postgres-dbi-parser.t index 6cd481305..08118ff8f 100644 --- a/t/66-postgres-dbi-parser.t +++ b/t/66-postgres-dbi-parser.t @@ -53,7 +53,7 @@ my $sql = q[ CREATE TABLE sqlt_products_1 ( product_no integer, - name text, + name text default '['''']', price numeric(8,4) default 0.0, created_at timestamp without time zone default now() ); @@ -166,6 +166,11 @@ isa_ok( $fk_ref1, 'SQL::Translator::Schema::Constraint', 'FK' ); is( $fk_ref1->reference_table, 'sqlt_test1', 'FK is to "sqlt_test1" table' ); my $t3 = $schema->get_table("sqlt_products_1"); + +my $t3_f2= $t3->get_field('name'); +is( $t3_f2->data_type, 'text', 'Second field, type "text"' ); +is( $t3_f2->default_value, q{['']}, 'default value is json array of empty string' ); + my $t3_f3= $t3->get_field('price'); is( $t3_f3->name, 'price', 'Third field is "price"' ); is( $t3_f3->data_type, 'numeric', 'Third field type "numeric"' );