Repository: schemahero/schemahero
Pull request #389 (2021-03-24 21:20:44)

Reverse logic of time zone handling in postgres
As mentioned in #345, our implementation of time zones was incorrect.

Now, we pass was was requested and rely on the underlying engine.



Patch:
diff --git a/pkg/database/cassandra/alter.go b/pkg/database/cassandra/alter.go
index 537c4636..4c1d5465 100644
--- a/pkg/database/cassandra/alter.go
+++ b/pkg/database/cassandra/alter.go
@@ -18,7 +18,7 @@ func AlterColumnStatements(keyspace string, tableName string, desiredColumns []*
 				return nil, err
 			}
 
-			if columnsMatch(existingColumn, column) {
+			if columnsMatch(*existingColumn, *column) {
 				return []string{}, nil
 			}
 
@@ -38,7 +38,7 @@ func AlterColumnStatements(keyspace string, tableName string, desiredColumns []*
 	return []string{fmt.Sprintf(`alter table "%s.%s" drop column %s`, keyspace, tableName, existingColumn.Name)}, nil
 }
 
-func columnsMatch(col1 *types.Column, col2 *types.Column) bool {
+func columnsMatch(col1 types.Column, col2 types.Column) bool {
 	if col1.DataType != col2.DataType {
 		return false
 	}
diff --git a/pkg/database/mysql/alter.go b/pkg/database/mysql/alter.go
index 4b5d6692..96c32c5a 100644
--- a/pkg/database/mysql/alter.go
+++ b/pkg/database/mysql/alter.go
@@ -26,7 +26,7 @@ func AlterColumnStatements(tableName string, primaryKeys []string, desiredColumn
 				ensureColumnConstraintsNotNullTrue(column)
 			}
 
-			if columnsMatch(existingColumn, column) {
+			if columnsMatch(*existingColumn, *column) {
 				return []string{}, nil
 			}
 
@@ -45,7 +45,7 @@ func AlterColumnStatements(tableName string, primaryKeys []string, desiredColumn
 	}.DDL(), nil
 }
 
-func columnsMatch(col1 *types.Column, col2 *types.Column) bool {
+func columnsMatch(col1 types.Column, col2 types.Column) bool {
 	if col1.DataType != col2.DataType {
 		return false
 	}
diff --git a/pkg/database/postgres/alter.go b/pkg/database/postgres/alter.go
index aba4c9a7..5844a894 100644
--- a/pkg/database/postgres/alter.go
+++ b/pkg/database/postgres/alter.go
@@ -20,7 +20,7 @@ func AlterColumnStatements(tableName string, primaryKeys []string, desiredColumn
 				return nil, err
 			}
 
-			if columnsMatch(existingColumn, column) {
+			if columnsMatch(*existingColumn, *column) {
 				return []string{}, nil
 			}
 
@@ -122,7 +122,24 @@ func AlterColumnStatements(tableName string, primaryKeys []string, desiredColumn
 	return []string{fmt.Sprintf(`alter table %s drop column %s`, pgx.Identifier{tableName}.Sanitize(), pgx.Identifier{existingColumn.Name}.Sanitize())}, nil
 }
 
-func columnsMatch(col1 *types.Column, col2 *types.Column) bool {
+func columnsMatch(col1 types.Column, col2 types.Column) bool {
+	// for time and timestamp comparisons, we know that postgres
+	// defaults to without time zone, so let's normalize a case
+	// where someone is relying on the default and specifying it
+	if col1.DataType == "timestamp" {
+		col1.DataType = "timestamp without time zone"
+	}
+	if col2.DataType == "timestamp" {
+		col2.DataType = "timestamp without time zone"
+	}
+
+	if col1.DataType == "time" {
+		col1.DataType = "time without time zone"
+	}
+	if col2.DataType == "time" {
+		col2.DataType = "time without time zone"
+	}
+
 	if col1.DataType != col2.DataType {
 		return false
 	}
diff --git a/pkg/database/postgres/column.go b/pkg/database/postgres/column.go
index 4239aa8f..fe967da5 100644
--- a/pkg/database/postgres/column.go
+++ b/pkg/database/postgres/column.go
@@ -9,6 +9,7 @@ import (
 	"github.com/schemahero/schemahero/pkg/database/types"
 )
 
+// schemaColumnToColumn converts the requested type from the v1alpha4 schema to a postgres schema type
 func schemaColumnToColumn(schemaColumn *schemasv1alpha4.PostgresqlTableColumn) (*types.Column, error) {
 	column := &types.Column{
 		Name:          schemaColumn.Name,
diff --git a/pkg/database/postgres/parameterized.go b/pkg/database/postgres/parameterized.go
index 55397fe9..8199c837 100644
--- a/pkg/database/postgres/parameterized.go
+++ b/pkg/database/postgres/parameterized.go
@@ -115,17 +115,25 @@ func maybeParseParameterizedColumnType(requestedType string) (string, error) {
 		withPrecisionWithoutTimeZone := regexp.MustCompile(`timestamp\s*\(\s*(?P<precision>.*)\s*\)\s*without time zone`)
 		withPrecision := regexp.MustCompile(`timestamp\s*\(\s*(?P<precision>.*)\s*\)`)
 		withoutPrecisionWithoutTimeZone := regexp.MustCompile(`timestamp\s*without time zone`)
+		withPrecisionWithTimeZone := regexp.MustCompile(`timestamp\s*\(\s*(?P<precision>.*)\s*\)\s*with time zone`)
+		withoutPrecisionWithTimeZone := regexp.MustCompile(`timestamp\s*with time zone`)
 		withoutPrecision := regexp.MustCompile(`timestamp\s*`)
 
 		withPrecisionMatchGroups := withPrecision.FindStringSubmatch(requestedType)
 		withPrecisionWithoutTimeZoneMatchGroups := withPrecisionWithoutTimeZone.FindStringSubmatch(requestedType)
+		withPrecisionWithTimeZoneMatchGroups := withPrecisionWithTimeZone.FindStringSubmatch(requestedType)
 		withoutPrecisionMatchGroups := withoutPrecision.FindStringSubmatch(requestedType)
 		withoutPrecisionWithoutTimeZoneMatchGroups := withoutPrecisionWithoutTimeZone.FindStringSubmatch(requestedType)
+		withoutPrecisionWithTimeZoneMatchGroups := withoutPrecisionWithTimeZone.FindStringSubmatch(requestedType)
 
 		if len(withPrecisionWithoutTimeZoneMatchGroups) == 2 {
 			columnType = fmt.Sprintf("timestamp (%s) without time zone", withPrecisionWithoutTimeZoneMatchGroups[1])
 		} else if len(withoutPrecisionWithoutTimeZoneMatchGroups) == 1 {
 			columnType = "timestamp without time zone"
+		} else if len(withPrecisionWithTimeZoneMatchGroups) == 2 {
+			columnType = fmt.Sprintf("timestamp (%s) with time zone", withPrecisionWithTimeZoneMatchGroups[1])
+		} else if len(withoutPrecisionWithTimeZoneMatchGroups) == 1 {
+			columnType = "timestamp with time zone"
 		} else if len(withPrecisionMatchGroups) == 2 {
 			columnType = fmt.Sprintf("timestamp (%s)", withPrecisionMatchGroups[1])
 		} else if len(withoutPrecisionMatchGroups) == 1 {
@@ -137,17 +145,25 @@ func maybeParseParameterizedColumnType(requestedType string) (string, error) {
 		withPrecisionWithoutTimeZone := regexp.MustCompile(`time\s*\(\s*(?P<precision>.*)\s*\)\s*without time zone`)
 		withPrecision := regexp.MustCompile(`time\s*\(\s*(?P<precision>.*)\s*\)`)
 		withoutPrecisionWithoutTimeZone := regexp.MustCompile(`time\s*without time zone`)
+		withPrecisionWithTimeZone := regexp.MustCompile(`timestamp\s*\(\s*(?P<precision>.*)\s*\)\s*with time zone`)
 		withoutPrecision := regexp.MustCompile(`time\s*`)
+		withoutPrecisionWithTimeZone := regexp.MustCompile(`time\s*with time zone`)
 
 		withPrecisionMatchGroups := withPrecision.FindStringSubmatch(requestedType)
 		withPrecisionWithoutTimeZoneMatchGroups := withPrecisionWithoutTimeZone.FindStringSubmatch(requestedType)
+		withPrecisionWithTimeZoneMatchGroups := withPrecisionWithTimeZone.FindStringSubmatch(requestedType)
 		withoutPrecisionMatchGroups := withoutPrecision.FindStringSubmatch(requestedType)
 		withoutPrecisionWithoutTimeZoneMatchGroups := withoutPrecisionWithoutTimeZone.FindStringSubmatch(requestedType)
+		withoutPrecisionWithTimeZoneMatchGroups := withoutPrecisionWithTimeZone.FindStringSubmatch(requestedType)
 
 		if len(withPrecisionWithoutTimeZoneMatchGroups) == 2 {
 			columnType = fmt.Sprintf("time (%s) without time zone", withPrecisionWithoutTimeZoneMatchGroups[1])
 		} else if len(withoutPrecisionWithoutTimeZoneMatchGroups) == 1 {
 			columnType = "time without time zone"
+		} else if len(withPrecisionWithTimeZoneMatchGroups) == 2 {
+			columnType = fmt.Sprintf("time (%s) with time zone", withPrecisionWithoutTimeZoneMatchGroups[1])
+		} else if len(withoutPrecisionWithTimeZoneMatchGroups) == 1 {
+			columnType = "time with time zone"
 		} else if len(withPrecisionMatchGroups) == 2 {
 			columnType = fmt.Sprintf("time (%s)", withPrecisionMatchGroups[1])
 		} else if len(withoutPrecisionMatchGroups) == 1 {
diff --git a/pkg/database/sqlite/alter.go b/pkg/database/sqlite/alter.go
index 55195498..c2d6fa98 100644
--- a/pkg/database/sqlite/alter.go
+++ b/pkg/database/sqlite/alter.go
@@ -47,7 +47,7 @@ func RecreateTableStatements(tableName string, sqliteTableSchema *schemasv1alpha
 	return statements, nil
 }
 
-func columnsMatch(col1 *types.Column, col2 *types.Column) bool {
+func columnsMatch(col1 types.Column, col2 types.Column) bool {
 	if col1.DataType != col2.DataType {
 		return false
 	}
diff --git a/pkg/database/sqlite/deploy.go b/pkg/database/sqlite/deploy.go
index 63fa8fcd..8c7ecd9d 100644
--- a/pkg/database/sqlite/deploy.go
+++ b/pkg/database/sqlite/deploy.go
@@ -137,7 +137,7 @@ AND m.name = ?`
 				if err != nil {
 					return nil, errors.Wrap(err, "failed to convert desired column")
 				}
-				if !columnsMatch(colA, &existingColumn) {
+				if !columnsMatch(*colA, existingColumn) {
 					needsRecreate = true
 				}
 			}


Test patch:
diff --git a/integration/tests/postgres/Makefile b/integration/tests/postgres/Makefile
index baa68b4d..30830d12 100644
--- a/integration/tests/postgres/Makefile
+++ b/integration/tests/postgres/Makefile
@@ -10,6 +10,7 @@ run: 10.16 11.11 12.6 13.2
 .PHONY: 10.16
 10.16: export PG_VERSION = 10.16
 10.16:
+	make -C alter-column-timezone run
 	make -C column-set-default run
 	make -C column-unset-default run
 	make -C create-table run
diff --git a/integration/tests/postgres/alter-column-timezone/Dockerfile b/integration/tests/postgres/alter-column-timezone/Dockerfile
new file mode 100644
index 00000000..6ea51e62
--- /dev/null
+++ b/integration/tests/postgres/alter-column-timezone/Dockerfile
@@ -0,0 +1,7 @@
+FROM postgres:10.7
+
+ENV POSTGRES_USER=schemahero
+ENV POSTGRES_DB=schemahero
+
+## Insert fixtures
+COPY ./fixtures.sql /docker-entrypoint-initdb.d/
diff --git a/integration/tests/postgres/alter-column-timezone/Makefile b/integration/tests/postgres/alter-column-timezone/Makefile
new file mode 100644
index 00000000..00c3ce20
--- /dev/null
+++ b/integration/tests/postgres/alter-column-timezone/Makefile
@@ -0,0 +1,4 @@
+include ../common.mk
+
+TEST_NAME := postgres-alter-table-timezone
+SPEC_FILE := ./specs/users.yaml
diff --git a/integration/tests/postgres/alter-column-timezone/expect.sql b/integration/tests/postgres/alter-column-timezone/expect.sql
new file mode 100644
index 00000000..e69de29b
diff --git a/integration/tests/postgres/alter-column-timezone/fixtures.sql b/integration/tests/postgres/alter-column-timezone/fixtures.sql
new file mode 100644
index 00000000..7af4f1d8
--- /dev/null
+++ b/integration/tests/postgres/alter-column-timezone/fixtures.sql
@@ -0,0 +1,7 @@
+create table users (
+  id integer primary key not null,
+  tz_1 timestamp,
+  tz_2 timestamp without time zone,
+  tz_3 timestamp with time zone,
+  tz_4 timestamp without time zone
+);
diff --git a/integration/tests/postgres/alter-column-timezone/specs/users.yaml b/integration/tests/postgres/alter-column-timezone/specs/users.yaml
new file mode 100644
index 00000000..0e9c72cf
--- /dev/null
+++ b/integration/tests/postgres/alter-column-timezone/specs/users.yaml
@@ -0,0 +1,17 @@
+database: schemahero
+name: users
+requires: []
+schema:
+  postgres:
+    primaryKey: [id]
+    columns:
+      - name: id
+        type: integer
+      - name: tz_1
+        type: timestamp
+      - name: tz_2
+        type: timestamp without time zone
+      - name: tz_3
+        type: timestamp with time zone
+      - name: tz_4
+        type: timestamp
diff --git a/integration/tests/postgres/create-table/expect.sql b/integration/tests/postgres/create-table/expect.sql
index c7ee2777..476cce2d 100644
--- a/integration/tests/postgres/create-table/expect.sql
+++ b/integration/tests/postgres/create-table/expect.sql
@@ -1,1 +1,1 @@
-create table "users" ("id" integer, "login" character varying (255), "name" character varying (255) not null default 'ethan', primary key ("id"));
+create table "users" ("id" integer, "login" character varying (255), "name" character varying (255) not null default 'ethan', "tz_1" timestamp, "tz_2" timestamp with time zone, "tz_3" timestamp without time zone, primary key ("id"));
diff --git a/integration/tests/postgres/create-table/specs/users.yaml b/integration/tests/postgres/create-table/specs/users.yaml
index 971ca188..1ba6bfe6 100644
--- a/integration/tests/postgres/create-table/specs/users.yaml
+++ b/integration/tests/postgres/create-table/specs/users.yaml
@@ -14,3 +14,9 @@ schema:
         constraints:
           notNull: true
         default: ethan
+      - name: tz_1
+        type: timestamp
+      - name: tz_2
+        type: timestamp with time zone
+      - name: tz_3
+        type: timestamp without time zone
diff --git a/pkg/database/postgres/alter_test.go b/pkg/database/postgres/alter_test.go
index 956d8877..ca1c9011 100644
--- a/pkg/database/postgres/alter_test.go
+++ b/pkg/database/postgres/alter_test.go
@@ -10,6 +10,35 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func Test_ColumnsMatch(t *testing.T) {
+	tests := []struct {
+		name   string
+		col1   types.Column
+		col2   types.Column
+		expect bool
+	}{
+		{
+			name: "timestamp and timestamp without time zone",
+			col1: types.Column{
+				Name:     "a",
+				DataType: "timestamp",
+			},
+			col2: types.Column{
+				Name:     "a",
+				DataType: "timestamp without time zone",
+			},
+			expect: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actual := columnsMatch(test.col1, test.col2)
+			assert.Equal(t, test.expect, actual)
+		})
+	}
+}
+
 func Test_AlterColumnStatments(t *testing.T) {
 	defaultEleven := "11"
 	defaultEmpty := ""
@@ -25,11 +54,11 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "no change",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "integer",
 				},
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "b",
 					Type: "integer",
 				},
@@ -45,7 +74,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "no change varchar",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "varchar(32)",
 				},
@@ -60,11 +89,11 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "change data type",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "integer",
 				},
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "b",
 					Type: "integer",
 				},
@@ -80,7 +109,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "drop column",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "integer",
 				},
@@ -118,7 +147,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "drop not null constraint",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "integer",
 					Constraints: &schemasv1alpha4.PostgresqlTableColumnConstraints{
@@ -140,7 +169,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "no change to not null constraint",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "t",
 					Type: "text",
 				},
@@ -159,7 +188,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "no change to not nullable timestamp using short column type",
 			tableName: "ts",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "ts",
 					Type: "timestamp",
 					Constraints: &schemasv1alpha4.PostgresqlTableColumnConstraints{
@@ -181,7 +210,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "no change to not nullable timestamp",
 			tableName: "ts",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "ts",
 					Type: "timestamp with time zone",
 					Constraints: &schemasv1alpha4.PostgresqlTableColumnConstraints{
@@ -191,7 +220,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			},
 			existingColumn: &types.Column{
 				Name:          "ts",
-				DataType:      "timestamp",
+				DataType:      "timestamp with time zone",
 				ColumnDefault: nil,
 				Constraints: &types.ColumnConstraints{
 					NotNull: &trueValue,
@@ -203,7 +232,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "default set",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name:    "a",
 					Type:    "integer",
 					Default: &defaultEleven,
@@ -219,7 +248,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "default unset",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name: "a",
 					Type: "integer",
 				},
@@ -235,7 +264,7 @@ func Test_AlterColumnStatments(t *testing.T) {
 			name:      "default empty string",
 			tableName: "t",
 			desiredColumns: []*schemasv1alpha4.PostgresqlTableColumn{
-				&schemasv1alpha4.PostgresqlTableColumn{
+				{
 					Name:    "a",
 					Type:    "varchar (32)",
 					Default: &defaultEmpty,