Honor supplied field order when adding fields to a table object

diff --git a/Changes b/Changes
index 56e7a15..3c49720 100644 (file)
--- a/Changes
+++ b/Changes
@@ -47,6 +47,8 @@
 * Fix ignored option to script/sqlt-diagram (RT#5992)
 * Fix t/17sqlfxml-producer.t failures due to whitespace differences introduced by
   environment config snippets (RT#70786)
+* Fix assembly of Table objects with numbered columns being added out of order
+  (RT#74771) (based on patch from Jonathan Otsuka)
 * Deprecate SQL::Translator::Schema::Graph and the as_graph() schema method
 * Bump minimum supported perl version to 5.8.1 (mostly due to Moo)
 

diff --git a/lib/SQL/Translator/Schema/Table.pm b/lib/SQL/Translator/Schema/Table.pm
index b984e2b..36c2c09 100644 (file)
--- a/lib/SQL/Translator/Schema/Table.pm
+++ b/lib/SQL/Translator/Schema/Table.pm
@@ -26,7 +26,9 @@ use SQL::Translator::Schema::Constants;
 use SQL::Translator::Schema::Constraint;
 use SQL::Translator::Schema::Field;
 use SQL::Translator::Schema::Index;
-use Data::Dumper;
+
+use Carp::Clan '^SQL::Translator';
+use List::Util 'max';
 
 use base 'SQL::Translator::Schema::Object';
 
@@ -56,18 +58,6 @@ Object constructor.
 
 =cut
 
-sub new {
-  my $class = shift;
-  my $self = $class->SUPER::new (@_)
-    or return;
-
-  $self->{_order} = { map { $_ => 0 } qw/
-    field
-  /};
-
-  return $self;
-}
-
 sub add_constraint {
 
 =pod
@@ -306,11 +296,35 @@ existing field, you will get an error and the field will not be created.
             $self->error( $field_class->error );
     }
 
-    $field->order( ++$self->{_order}{field} );
+    my $existing_order = { map { $_->order => $_->name } $self->get_fields };
+
+    # supplied order, possible unordered assembly
+    if ( $field->order ) {
+        if($existing_order->{$field->order}) {
+            croak sprintf
+                "Requested order '%d' for column '%s' conflicts with already existing column '%s'",
+                $field->order,
+                $field->name,
+                $existing_order->{$field->order},
+            ;
+        }
+    }
+    else {
+        my $last_field_no = max(keys %$existing_order) || 0;
+        if ( $last_field_no != scalar keys %$existing_order ) {
+            croak sprintf
+                "Table '%s' field order incomplete - unable to auto-determine order for newly added field",
+                $self->name,
+            ;
+        }
+
+        $field->order( $last_field_no + 1 );
+    }
+
     # We know we have a name as the Field->new above errors if none given.
     my $field_name = $field->name;
 
-    if ( exists $self->{'fields'}{ $field_name } ) {
+    if ( $self->get_field($field_name) ) {
         return $self->error(qq[Can't create field: "$field_name" exists]);
     }
     else {

diff --git a/t/13schema.t b/t/13schema.t
index c85c3b6..3d38208 100644 (file)
--- a/t/13schema.t
+++ b/t/13schema.t
@@ -4,7 +4,8 @@
 $| = 1;
 
 use strict;
-use Test::More tests => 238;
+use Test::More tests => 245;
+use Test::Exception;
 use SQL::Translator::Schema::Constants;
 
 require_ok( 'SQL::Translator' );
@@ -715,3 +716,31 @@ require_ok( 'SQL::Translator::Schema' );
 
     $s->add_procedure($p);
 }
+
+#
+# Test field order
+#
+{
+    my $s  = SQL::Translator::Schema->new;
+    my $t  = $s->add_table( name => 'person'          );
+    my $f3 = $t->add_field( name => 'age', order => 3 );
+    my $f1 = $t->add_field( name => 'person_id', order => 1 );
+    my $f2 = $t->add_field( name => 'name', order => 2 );
+    my $f4 = $t->add_field( name => 'gender' );
+    my $f5 = $t->add_field( name => 'alias' );
+
+    is( $f1->order, 1, 'Field order is passed, order is 1' );
+    is( $f2->order, 2, 'Field order is passed, order is 2' );
+    is( $f3->order, 3, 'Field order is passed, order is 3' );
+    is( $f4->order, 4, 'Field order is not passed, order is 4' );
+    is( $f5->order, 5, 'Field order is not passed, order is 5' );
+
+    my $t2  = $s->add_table( name => 'place'                );
+    $f2 = $t2->add_field( name => 'name', order => 2 );
+
+    throws_ok { my $f22 = $t2->add_field( name => 'name2', order => 2 ) }
+        qr/\QRequested order '2' for column 'name2' conflicts with already existing column 'name'/;
+
+    throws_ok { $f1 = $t2->add_field( name => 'location' ) }
+        qr/field order incomplete/;
+}

diff --git a/t/38-mysql-producer.t b/t/38-mysql-producer.t
index b2b15d4..1b72059 100644 (file)
--- a/t/38-mysql-producer.t
+++ b/t/38-mysql-producer.t
@@ -46,27 +46,27 @@ schema:
           data_type: unsigned int
           is_primary_key: 1
           is_auto_increment: 1
-          order: 0
+          order: 1
         name:
           name: name
           data_type: varchar
           size:
             - 32
-          order: 1
+          order: 2
         swedish_name:
           name: swedish_name
           data_type: varchar
           size: 32
           extra:
             mysql_charset: swe7
-          order: 2
+          order: 3
         description:
           name: description
           data_type: text
           extra:
             mysql_charset: utf8
             mysql_collate: utf8_general_ci
-          order: 3
+          order: 4
       constraints:
         - type: UNIQUE
           fields:
@@ -82,22 +82,22 @@ schema:
           name: id
           data_type: int
           is_primary_key: 0
-          order: 0
+          order: 1
           is_foreign_key: 1
         foo:
           name: foo
           data_type: int
-          order: 1
+          order: 2
           is_not_null: 1
         foo2:
           name: foo2
           data_type: int
-          order: 2
+          order: 3
           is_not_null: 1
         bar_set:
           name: bar_set
           data_type: set
-          order: 3
+          order: 4
           is_not_null: 1
           extra:
             list:
@@ -136,22 +136,22 @@ schema:
           name: id
           data_type: int
           is_primary_key: 0
-          order: 0
+          order: 1
           is_foreign_key: 1
         foo:
           name: foo
           data_type: int
-          order: 1
+          order: 2
           is_not_null: 1
         foo2:
           name: foo2
           data_type: int
-          order: 2
+          order: 3
           is_not_null: 1
         bar_set:
           name: bar_set
           data_type: set
-          order: 3
+          order: 4
           is_not_null: 1
           extra:
             list:

diff --git a/t/data/diff/create1.yml b/t/data/diff/create1.yml
index 0319b00..4cddd5c 100644 (file)
--- a/t/data/diff/create1.yml
+++ b/t/data/diff/create1.yml
@@ -23,7 +23,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: id
-          order: 10
+          order: 1
           size:
             - 11
       indices: []
@@ -67,7 +67,7 @@ schema:
           is_primary_key: 1
           is_unique: 0
           name: employee_id
-          order: 8
+          order: 2
           size:
             - 11
         job_title:
@@ -78,7 +78,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: job_title
-          order: 9
+          order: 3
           size:
             - 255
         position:
@@ -89,7 +89,7 @@ schema:
           is_primary_key: 1
           is_unique: 0
           name: position
-          order: 7
+          order: 1
           size:
             - 50
       indices: []

diff --git a/t/data/diff/create2.yml b/t/data/diff/create2.yml
index 897dc62..693c20a 100644 (file)
--- a/t/data/diff/create2.yml
+++ b/t/data/diff/create2.yml
@@ -13,7 +13,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: id
-          order: 10
+          order: 1
           size:
             - 11
       indices: []
@@ -57,7 +57,7 @@ schema:
           is_primary_key: 1
           is_unique: 0
           name: employee_id
-          order: 9
+          order: 2
           size:
             - 11
         position:
@@ -68,7 +68,7 @@ schema:
           is_primary_key: 1
           is_unique: 0
           name: position
-          order: 8
+          order: 1
           size:
             - 50
       indices: []

diff --git a/t/data/oracle/schema_diff_a.yaml b/t/data/oracle/schema_diff_a.yaml
index fefb46d..0951ab6 100644 (file)
--- a/t/data/oracle/schema_diff_a.yaml
+++ b/t/data/oracle/schema_diff_a.yaml
@@ -48,7 +48,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: other
-          order: 59
+          order: 60
           size:
             - 10
       name: d_operator

diff --git a/t/data/oracle/schema_diff_b.yaml b/t/data/oracle/schema_diff_b.yaml
index 1b9c898..42f71ba 100644 (file)
--- a/t/data/oracle/schema_diff_b.yaml
+++ b/t/data/oracle/schema_diff_b.yaml
@@ -48,7 +48,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: other
-          order: 59
+          order: 60
           size:
             - 10
       name: d_operator

diff --git a/t/data/oracle/schema_diff_c.yaml b/t/data/oracle/schema_diff_c.yaml
index f565ce3..27f2944 100644 (file)
--- a/t/data/oracle/schema_diff_c.yaml
+++ b/t/data/oracle/schema_diff_c.yaml
@@ -48,7 +48,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: foo
-          order: 59
+          order: 60
           size:
             - 10
         other:
@@ -59,7 +59,7 @@ schema:
           is_primary_key: 0
           is_unique: 0
           name: other
-          order: 59
+          order: 61
           size:
             - 10
       name: d_operator