Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honour DEFERRABLE on all constraints in PostgreSQL #75

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Honour DEFERRABLE on all constraints in PostgreSQL #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
325173f
Honour the deferred flag on *all* constraints
Nov 2, 2015
f32a8dc
Set deferrable default to 1 for FK, 0 otherwise
Nov 3, 2015
f5ce080
Set deferrable lazy, so default can see type attribute
Nov 3, 2015
11106a0
Special-case the default deferrable value in tests
Nov 3, 2015
b9fe3b4
Fix all tests where deferrable has become relevant
Nov 3, 2015
bb565d5
Test deferrable on/off - not very thorough yet
Nov 3, 2015
da2e927
Constraint now mentions deferrable in constructor doc
Nov 3, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/SQL/Translator/Producer/PostgreSQL.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,11 @@ sub create_constraint
push @fks, "$def";
}

# This seemed preferable to doing it in every junction of that if
if ($c->deferrable and $c->type ne FOREIGN_KEY) {
$constraint_defs[-1] .= ' DEFERRABLE';
}

return \@constraint_defs, \@fks;
}

Expand Down
13 changes: 9 additions & 4 deletions lib/SQL/Translator/Schema/Constraint.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Object constructor.
match_type => 'full', # how to match
on_delete => 'cascade', # what to do on deletes
on_update => '', # what to do on updates
deferrable => 0, # whether to set DEFERRABLE, if supported by the database
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Altreus Ah I now understand. I thought you implemented the deferrable flag as an {extra} since it is supported by one and onky one highly non-standard engine. Hence why I was talking about sqlt_extra and was confused by how names etc applies in the other PR.

I really recommend moving this to extra like all other non-standard extension, e.g.: https://metacpan.org/pod/SQL::Translator::Producer::MySQL#Extra-attributes

Then the entire not on what is and isn't ignored in https://github.com/dbsrgits/dbix-class/pull/88/files#diff-14a995427a39511927479ae4ffbbb9c7R768 becomes moot.

@ilmari thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferrable constraints are part of the SQL standard, so I'm with @ilmari's original implementation here (sorry to revive a 6-year old thread, i'm just reviewing this to merge b/c I need it for $work)

);

=cut
Expand All @@ -78,9 +79,10 @@ around BUILDARGS => sub {

=head2 deferrable

Get or set whether the constraint is deferrable. If not defined,
then returns "1." The argument is evaluated by Perl for True or
False, so the following are equivalent:
Get or set whether the constraint is deferrable. The default is based on the
constraint type. Foreign keys are deferrable by default, for backward
compatibility; all other types are not. The argument is evaluated by Perl for
True or False, so the following are equivalent:

$deferrable = $field->deferrable(0);
$deferrable = $field->deferrable('');
Expand All @@ -91,7 +93,10 @@ False, so the following are equivalent:
has deferrable => (
is => 'rw',
coerce => quote_sub(q{ $_[0] ? 1 : 0 }),
default => quote_sub(q{ 1 }),
lazy => 1,
default => sub {
$_[0]->type eq FOREIGN_KEY
},
);

=head2 expression
Expand Down
7 changes: 6 additions & 1 deletion lib/Test/SQL/Translator.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ my %ATTRIBUTES = (
constraint => {
name => '',
type => '',
deferrable => 1,
deferrable => 0,
expression => '',
is_valid => 1,
fields => [],
Expand Down Expand Up @@ -135,6 +135,11 @@ sub default_attribs {
$hashref->{ $attr } = $ATTRIBUTES{ $object_type }{ $attr }
}

# Special case
if ($object_type eq 'constraint' and $hashref->{type} eq FOREIGN_KEY) {
$hashref->{deferrable} = 1;
}

return $hashref;
}

Expand Down
4 changes: 2 additions & 2 deletions t/17sqlfxml-producer.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ $ans = <<EOXML;
</index>
</indices>
<constraints>
<constraint name="" type="PRIMARY KEY" fields="id" reference_table="" reference_fields="" on_delete="" on_update="" match_type="" expression="" options="" deferrable="1">
<constraint name="" type="PRIMARY KEY" fields="id" reference_table="" reference_fields="" on_delete="" on_update="" match_type="" expression="" options="" deferrable="0">
<extra />
</constraint>
<constraint name="" type="UNIQUE" fields="email" reference_table="" reference_fields="" on_delete="" on_update="" match_type="" expression="" options="" deferrable="1">
<constraint name="" type="UNIQUE" fields="email" reference_table="" reference_fields="" on_delete="" on_update="" match_type="" expression="" options="" deferrable="0">
<extra />
</constraint>
</constraints>
Expand Down
8 changes: 4 additions & 4 deletions t/23json.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ my $json = from_json(<<JSON);
"person" : {
"constraints" : [
{
"deferrable" : 1,
"deferrable" : 0,
"expression" : "",
"fields" : [
"person_id"
Expand All @@ -39,7 +39,7 @@ my $json = from_json(<<JSON);
"type" : "PRIMARY KEY"
},
{
"deferrable" : 1,
"deferrable" : 0,
"expression" : "",
"fields" : [
"name"
Expand Down Expand Up @@ -142,7 +142,7 @@ my $json = from_json(<<JSON);
"pet" : {
"constraints" : [
{
"deferrable" : 1,
"deferrable" : 0,
"expression" : "",
"fields" : [],
"match_type" : "",
Expand All @@ -155,7 +155,7 @@ my $json = from_json(<<JSON);
"type" : "CHECK"
},
{
"deferrable" : 1,
"deferrable" : 0,
"expression" : "",
"fields" : [
"pet_id",
Expand Down
8 changes: 4 additions & 4 deletions t/24yaml.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ schema:
tables:
person:
constraints:
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- person_id
Expand All @@ -33,7 +33,7 @@ schema:
reference_fields: []
reference_table: ''
type: PRIMARY KEY
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- name
Expand Down Expand Up @@ -117,7 +117,7 @@ schema:
order: 1
pet:
constraints:
- deferrable: 1
- deferrable: 0
expression: ''
fields: []
match_type: ''
Expand All @@ -128,7 +128,7 @@ schema:
reference_fields: []
reference_table: ''
type: CHECK
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- pet_id
Expand Down
4 changes: 2 additions & 2 deletions t/30sqlt-new-diff-pgsql.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ ALTER TABLE "person" ADD CONSTRAINT "unique_name" UNIQUE ("name");

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") DEFERRABLE;

DROP TABLE "deleted" CASCADE;

Expand Down Expand Up @@ -143,7 +143,7 @@ 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_age_name UNIQUE (age, name);
ALTER TABLE person ADD CONSTRAINT UC_age_name UNIQUE (age, name) DEFERRABLE;

DROP TABLE deleted CASCADE;

Expand Down
2 changes: 1 addition & 1 deletion t/39-filter-globals.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ is_deeply ($struct, {
Person => {
constraints => [
{
deferrable => 1,
deferrable => 0,
expression => "",
fields => [
"modified"
Expand Down
75 changes: 48 additions & 27 deletions t/47postgres-producer.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ BEGIN {
use Test::Differences;
use SQL::Translator;

# normalised-space-is, i.e. are these the same after normalising whitespace?
sub ns_is {
my ($first, $second) = (shift, shift);
$first =~ s/\s+/ /g;
$second =~ s/\s+/ /g;

# avoid prototype
&is( $first, $second, @_ );
}

my $PRODUCER = \&SQL::Translator::Producer::PostgreSQL::create_field;

{
Expand Down Expand Up @@ -146,32 +156,39 @@ for my $name ( 'foo', undef ) {
type => 'FOREIGN_KEY',
reference_table => $table2,
reference_fields => [qw(myfield_2)],
deferrable => 0,
);

my ($fk_constraint_def_ref, $fk_constraint_fk_ref ) = SQL::Translator::Producer::PostgreSQL::create_constraint($fk_constraint);
for my $constraint ($fk_constraint, $fk_constraint_2) {
my ($fk_constraint_def_ref, $fk_constraint_fk_ref ) = SQL::Translator::Producer::PostgreSQL::create_constraint($constraint);

ok(@{$fk_constraint_def_ref} == 0 && @{$fk_constraint_fk_ref} == 1, 'precheck of create_Foreign Key constraint');
ok(@{$fk_constraint_def_ref} == 0 && @{$fk_constraint_fk_ref} == 1, 'precheck of create_Foreign Key constraint');

if ( $name ) {
is($fk_constraint_fk_ref->[0], "ALTER TABLE mytable ADD CONSTRAINT $name FOREIGN KEY (myfield)
REFERENCES mytable2 (myfield_2) DEFERRABLE", 'Create Foreign Key Constraint works');
my $deferrable = $constraint->deferrable ? ' DEFERRABLE' : '';

# ToDo: may we should check if the constraint name was valid, or if next
# unused_name created has choosen a different one
my $alter_fk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($fk_constraint);
is($alter_fk_constraint, "ALTER TABLE mytable DROP CONSTRAINT $name", 'Alter drop Foreign Key constraint works');
}
else {
is($fk_constraint_fk_ref->[0], 'ALTER TABLE mytable ADD FOREIGN KEY (myfield)
REFERENCES mytable2 (myfield_2) DEFERRABLE', 'Create un-named Foreign Key Constraint works');
if ( $name ) {

my $alter_fk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($fk_constraint);
is($alter_fk_constraint, 'ALTER TABLE mytable DROP CONSTRAINT mytable_myfield_fkey', 'Alter drop un-named Foreign Key constraint works');
ns_is($fk_constraint_fk_ref->[0], "ALTER TABLE mytable ADD CONSTRAINT $name FOREIGN KEY (myfield)
REFERENCES mytable2 (myfield_2)$deferrable", 'Create Foreign Key Constraint works');

# ToDo: may we should check if the constraint name was valid, or if next
# unused_name created has choosen a different one
my $alter_fk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($constraint);
is($alter_fk_constraint, "ALTER TABLE mytable DROP CONSTRAINT $name", 'Alter drop Foreign Key constraint works');
}
else {
ns_is($fk_constraint_fk_ref->[0], "ALTER TABLE mytable ADD FOREIGN KEY (myfield)
REFERENCES mytable2 (myfield_2)$deferrable", 'Create un-named Foreign Key Constraint works');

my $alter_fk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($constraint);
is($alter_fk_constraint, 'ALTER TABLE mytable DROP CONSTRAINT mytable_myfield_fkey', 'Alter drop un-named Foreign Key constraint works');
}
}
}

# check named, and unnamed primary keys
for my $name ( 'foo', undef ) {
# PK defaults to deferrable => 0
my $pk_constraint = SQL::Translator::Schema::Constraint->new(
table => $table,
name => $name,
Expand All @@ -183,23 +200,27 @@ for my $name ( 'foo', undef ) {
name => $name,
fields => [qw(myfield)],
type => 'PRIMARY_KEY',
deferrable => 1,
);

my ($pk_constraint_def_ref, $pk_constraint_pk_ref ) = SQL::Translator::Producer::PostgreSQL::create_constraint($pk_constraint);
for my $pk ($pk_constraint, $pk_constraint_2) {
my ($pk_constraint_def_ref, $pk_constraint_pk_ref ) = SQL::Translator::Producer::PostgreSQL::create_constraint($pk);

if ( $name ) {
is($pk_constraint_def_ref->[0], "CONSTRAINT $name PRIMARY KEY (myfield)", 'Create Primary Key Constraint works');
my $deferrable = $pk->deferrable ? ' DEFERRABLE' : '';
if ( $name ) {
is($pk_constraint_def_ref->[0], "CONSTRAINT $name PRIMARY KEY (myfield)$deferrable", 'Create Primary Key Constraint works');

# ToDo: may we should check if the constraint name was valid, or if next
# unused_name created has choosen a different one
my $alter_pk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($pk_constraint);
is($alter_pk_constraint, "ALTER TABLE mytable DROP CONSTRAINT $name", 'Alter drop Primary Key constraint works');
}
else {
is($pk_constraint_def_ref->[0], 'PRIMARY KEY (myfield)', 'Create un-named Primary Key Constraint works');
# ToDo: may we should check if the constraint name was valid, or if next
# unused_name created has choosen a different one
my $alter_pk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($pk);
is($alter_pk_constraint, "ALTER TABLE mytable DROP CONSTRAINT $name", 'Alter drop Primary Key constraint works');
}
else {
is($pk_constraint_def_ref->[0], "PRIMARY KEY (myfield)$deferrable", 'Create un-named Primary Key Constraint works');

my $alter_pk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($pk_constraint);
is($alter_pk_constraint, 'ALTER TABLE mytable DROP CONSTRAINT mytable_pkey', 'Alter drop un-named Foreign Key constraint works');
my $alter_pk_constraint = SQL::Translator::Producer::PostgreSQL::alter_drop_constraint($pk);
is($alter_pk_constraint, 'ALTER TABLE mytable DROP CONSTRAINT mytable_pkey', 'Alter drop un-named Foreign Key constraint works');
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions t/data/diff/pgsql/create1.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ schema:
- person_id
reference_table: person
type: FOREIGN KEY
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- position
Expand Down Expand Up @@ -111,7 +111,7 @@ schema:
order: 4
person:
constraints:
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- person_id
Expand Down
6 changes: 3 additions & 3 deletions t/data/diff/pgsql/create2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ schema:
- person_id
reference_table: person
type: FOREIGN KEY
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- employee_id
Expand Down Expand Up @@ -96,7 +96,7 @@ schema:
order: 4
person:
constraints:
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- person_id
Expand All @@ -108,7 +108,7 @@ schema:
reference_fields: []
reference_table: ''
type: PRIMARY KEY
- deferrable: 1
- deferrable: 0
expression: ''
fields:
- person_id
Expand Down
8 changes: 4 additions & 4 deletions t/data/template/testresult_basic.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ Constraints
match_type:
reference_fields:
reference_table:
deferrable: 1
deferrable: 0
on_delete:
on_update:
options:
Expand All @@ -165,7 +165,7 @@ Constraints
match_type:
reference_fields:
reference_table:
deferrable: 1
deferrable: 0
on_delete:
on_update:
options:
Expand All @@ -178,7 +178,7 @@ Constraints
match_type:
reference_fields:
reference_table:
deferrable: 1
deferrable: 0
on_delete:
on_update:
options:
Expand Down Expand Up @@ -240,7 +240,7 @@ Constraints
match_type:
reference_fields:
reference_table:
deferrable: 1
deferrable: 0
on_delete:
on_update:
options:
Expand Down
8 changes: 4 additions & 4 deletions t/data/xml/schema.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ Created on Fri Aug 15 15:08:18 2003

<constraints>
<constraint name="" type="PRIMARY KEY" fields="id"
reference_table="" options="" deferrable="1" match_type=""
reference_table="" options="" deferrable="0" match_type=""
expression="" on_update="" on_delete="">
<extra foo="bar" hello="world" bar="baz" />
</constraint>
<constraint name="emailuniqueindex" type="UNIQUE" fields="email" />
<constraint name="very_long_index_name_on_title_field_which_should_be_truncated_for_various_rdbms" type="UNIQUE" fields="title" />
<constraint name="emailuniqueindex" type="UNIQUE" fields="email" deferrable="0" />
<constraint name="very_long_index_name_on_title_field_which_should_be_truncated_for_various_rdbms" type="UNIQUE" fields="title" deferrable="0" />
<constraint name="" type="FOREIGN KEY" fields="another_id"
reference_table="Another" options="" deferrable="1" match_type=""
expression="" on_update="" on_delete="">
Expand All @@ -81,7 +81,7 @@ Created on Fri Aug 15 15:08:18 2003

<constraints>
<constraint name="" type="PRIMARY KEY" fields="id"
reference_table="" options="" deferrable="1" match_type=""
reference_table="" options="" deferrable="0" match_type=""
expression="" on_update="" on_delete="">
</constraint>
</constraints>
Expand Down