Always surround up and down clauses in parentheses (#481)

ryanslade · web-flow · commit 929ea276dad3 · 2024-11-22T15:00:35.000+01:00
Closes #473
diff --git a/docs/README.md b/docs/README.md
@@ -265,7 +265,7 @@ Here is the `pgroll` migration that will perform the migration to make the `desc
         "table": "users",
         "column": "description",
         "nullable": false,
-        "up": "(SELECT CASE WHEN description IS NULL THEN 'description for ' || name ELSE description END)",
+        "up": "SELECT CASE WHEN description IS NULL THEN 'description for ' || name ELSE description END",
         "down": "description"
       }
     }
@@ -307,7 +307,7 @@ You should see something like this:
 `pgroll` has added a `_pgroll_new_description` field to the table and populated the field for all rows using the `up` SQL from the `02_user_description_set_nullable.json` file:
 
 ```json
-"up": "(SELECT CASE WHEN description IS NULL THEN 'description for ' || name ELSE description END)",
+"up": "SELECT CASE WHEN description IS NULL THEN 'description for ' || name ELSE description END",
 ```
 
 This has copied over all `description` values into the `_pgroll_new_description` field, rewriting any `NULL` values using the provided SQL.
diff --git a/examples/16_set_nullable.json b/examples/16_set_nullable.json
@@ -6,7 +6,7 @@
         "table": "reviews",
         "column": "review",
         "nullable": false,
-        "up": "(SELECT CASE WHEN review IS NULL THEN product || ' is good' ELSE review END)",
+        "up": "SELECT CASE WHEN review IS NULL THEN product || ' is good' ELSE review END",
         "down": "review"
       }
     }
diff --git a/examples/21_add_foreign_key_constraint.json b/examples/21_add_foreign_key_constraint.json
@@ -11,7 +11,7 @@
           "column": "id",
           "on_delete": "CASCADE"
         },
-        "up": "(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)",
+        "up": "SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END",
         "down": "user_id"
       }
     }
diff --git a/examples/22_add_check_constraint.json b/examples/22_add_check_constraint.json
@@ -9,7 +9,7 @@
           "name": "title_length",
           "constraint": "length(title) > 3"
         },
-        "up": "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)",
+        "up": "SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END",
         "down": "title"
       }
     }
diff --git a/examples/23_drop_check_constraint.json b/examples/23_drop_check_constraint.json
@@ -6,7 +6,7 @@
         "table": "posts",
         "name": "title_length",
         "up": "title",
-        "down": "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)"
+        "down": "SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END"
       }
     }
   ]
diff --git a/examples/24_drop_foreign_key_constraint.json b/examples/24_drop_foreign_key_constraint.json
@@ -6,7 +6,7 @@
         "table": "posts",
         "name": "fk_users_id",
         "up": "user_id",
-        "down": "(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)"
+        "down": "SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END"
       }
     }
   ]
diff --git a/examples/31_unset_not_null.json b/examples/31_unset_not_null.json
@@ -7,7 +7,7 @@
         "column": "title",
         "nullable": true,
         "up": "title",
-        "down": "(SELECT CASE WHEN title IS NULL THEN 'placeholder title' ELSE title END)"
+        "down": "SELECT CASE WHEN title IS NULL THEN 'placeholder title' ELSE title END"
       }
     }
   ]
diff --git a/examples/35_alter_column_multiple.json b/examples/35_alter_column_multiple.json
@@ -17,7 +17,7 @@
           "name": "event_name_length",
           "constraint": "length(name) > 3"
         },
-        "up": "(SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END)",
+        "up": "SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END",
         "down": "event_name"
       }
     }
diff --git a/examples/45_add_table_check_constraint.json b/examples/45_add_table_check_constraint.json
@@ -13,7 +13,7 @@
         "check": "sellers_name = 'alice' OR sellers_zip > 0",
         "up": {
           "sellers_name": "sellers_name",
-          "sellers_zip": "(SELECT CASE WHEN sellers_name != 'alice' AND sellers_zip <= 0 THEN 123 WHEN sellers_name != 'alice' THEN sellers_zip ELSE sellers_zip END)"
+          "sellers_zip": "SELECT CASE WHEN sellers_name != 'alice' AND sellers_zip <= 0 THEN 123 WHEN sellers_name != 'alice' THEN sellers_zip ELSE sellers_zip END"
         },
         "down": {
           "sellers_name": "sellers_name",
diff --git a/internal/benchmarks/benchmarks_test.go b/internal/benchmarks/benchmarks_test.go
@@ -256,7 +256,7 @@ var migAlterColumn = migrations.Migration{
 		&migrations.OpAlterColumn{
 			Table:    "users",
 			Column:   "name",
-			Up:       "(SELECT CASE WHEN name IS NULL THEN 'placeholder' ELSE name END)",
+			Up:       "SELECT CASE WHEN name IS NULL THEN 'placeholder' ELSE name END",
 			Down:     "user_name",
 			Comment:  nullable.NewNullableWithValue("the name of the user"),
 			Nullable: ptr(false),
diff --git a/pkg/migrations/op_add_column_test.go b/pkg/migrations/op_add_column_test.go
@@ -786,6 +786,170 @@ func TestAddColumnWithUpSql(t *testing.T) {
 				TriggerMustNotExist(t, db, schema, "products", triggerName)
 			},
 		},
+		{
+			name: "add column with up sql missing parentheses",
+			migrations: []migrations.Migration{
+				{
+					Name: "01_add_table",
+					Operations: migrations.Operations{
+						&migrations.OpCreateTable{
+							Name: "products",
+							Columns: []migrations.Column{
+								{
+									Name: "id",
+									Type: "serial",
+									Pk:   ptr(true),
+								},
+								{
+									Name:   "name",
+									Type:   "varchar(255)",
+									Unique: ptr(true),
+								},
+							},
+						},
+					},
+				},
+				{
+					Name: "02_add_column",
+					Operations: migrations.Operations{
+						&migrations.OpAddColumn{
+							Table: "products",
+							Up:    "select name",
+							Column: migrations.Column{
+								Name:     "description",
+								Type:     "varchar(255)",
+								Nullable: ptr(true),
+							},
+						},
+					},
+				},
+			},
+			afterStart: func(t *testing.T, db *sql.DB, schema string) {
+				// inserting via both the old and the new views works
+				MustInsert(t, db, schema, "01_add_table", "products", map[string]string{
+					"name": "apple",
+				})
+				MustInsert(t, db, schema, "02_add_column", "products", map[string]string{
+					"name":        "banana",
+					"description": "a yellow banana",
+				})
+
+				res := MustSelect(t, db, schema, "02_add_column", "products")
+				assert.Equal(t, []map[string]any{
+					// the description column has been populated for the product inserted into the old view.
+					{"id": 1, "name": "apple", "description": "apple"},
+					// the description column for the product inserted into the new view is as inserted.
+					{"id": 2, "name": "banana", "description": "a yellow banana"},
+				}, res)
+			},
+			afterRollback: func(t *testing.T, db *sql.DB, schema string) {
+				// The trigger function has been dropped.
+				triggerFnName := migrations.TriggerFunctionName("products", "description")
+				FunctionMustNotExist(t, db, schema, triggerFnName)
+
+				// The trigger has been dropped.
+				triggerName := migrations.TriggerName("products", "description")
+				TriggerMustNotExist(t, db, schema, "products", triggerName)
+			},
+			afterComplete: func(t *testing.T, db *sql.DB, schema string) {
+				// after rollback + restart + complete, all 'description' values are the backfilled ones.
+				res := MustSelect(t, db, schema, "02_add_column", "products")
+				assert.Equal(t, []map[string]any{
+					{"id": 1, "name": "apple", "description": "apple"},
+					{"id": 2, "name": "banana", "description": "banana"},
+				}, res)
+
+				// The trigger function has been dropped.
+				triggerFnName := migrations.TriggerFunctionName("products", "description")
+				FunctionMustNotExist(t, db, schema, triggerFnName)
+
+				// The trigger has been dropped.
+				triggerName := migrations.TriggerName("products", "description")
+				TriggerMustNotExist(t, db, schema, "products", triggerName)
+			},
+		},
+		{
+			name: "add column with up sql missing parentheses, no select",
+			migrations: []migrations.Migration{
+				{
+					Name: "01_add_table",
+					Operations: migrations.Operations{
+						&migrations.OpCreateTable{
+							Name: "products",
+							Columns: []migrations.Column{
+								{
+									Name: "id",
+									Type: "serial",
+									Pk:   ptr(true),
+								},
+								{
+									Name:   "name",
+									Type:   "varchar(255)",
+									Unique: ptr(true),
+								},
+							},
+						},
+					},
+				},
+				{
+					Name: "02_add_column",
+					Operations: migrations.Operations{
+						&migrations.OpAddColumn{
+							Table: "products",
+							Up:    "name",
+							Column: migrations.Column{
+								Name:     "description",
+								Type:     "varchar(255)",
+								Nullable: ptr(true),
+							},
+						},
+					},
+				},
+			},
+			afterStart: func(t *testing.T, db *sql.DB, schema string) {
+				// inserting via both the old and the new views works
+				MustInsert(t, db, schema, "01_add_table", "products", map[string]string{
+					"name": "apple",
+				})
+				MustInsert(t, db, schema, "02_add_column", "products", map[string]string{
+					"name":        "banana",
+					"description": "a yellow banana",
+				})
+
+				res := MustSelect(t, db, schema, "02_add_column", "products")
+				assert.Equal(t, []map[string]any{
+					// the description column has been populated for the product inserted into the old view.
+					{"id": 1, "name": "apple", "description": "apple"},
+					// the description column for the product inserted into the new view is as inserted.
+					{"id": 2, "name": "banana", "description": "a yellow banana"},
+				}, res)
+			},
+			afterRollback: func(t *testing.T, db *sql.DB, schema string) {
+				// The trigger function has been dropped.
+				triggerFnName := migrations.TriggerFunctionName("products", "description")
+				FunctionMustNotExist(t, db, schema, triggerFnName)
+
+				// The trigger has been dropped.
+				triggerName := migrations.TriggerName("products", "description")
+				TriggerMustNotExist(t, db, schema, "products", triggerName)
+			},
+			afterComplete: func(t *testing.T, db *sql.DB, schema string) {
+				// after rollback + restart + complete, all 'description' values are the backfilled ones.
+				res := MustSelect(t, db, schema, "02_add_column", "products")
+				assert.Equal(t, []map[string]any{
+					{"id": 1, "name": "apple", "description": "apple"},
+					{"id": 2, "name": "banana", "description": "banana"},
+				}, res)
+
+				// The trigger function has been dropped.
+				triggerFnName := migrations.TriggerFunctionName("products", "description")
+				FunctionMustNotExist(t, db, schema, triggerFnName)
+
+				// The trigger has been dropped.
+				triggerName := migrations.TriggerName("products", "description")
+				TriggerMustNotExist(t, db, schema, "products", triggerName)
+			},
+		},
 		{
 			name: "add column with up sql and a text pk",
 			migrations: []migrations.Migration{
diff --git a/pkg/migrations/op_alter_column_test.go b/pkg/migrations/op_alter_column_test.go
@@ -47,7 +47,7 @@ func TestAlterColumnMultipleSubOperations(t *testing.T) {
 						&migrations.OpAlterColumn{
 							Table:    "events",
 							Column:   "name",
-							Up:       "(SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END)",
+							Up:       "SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END",
 							Down:     "event_name",
 							Name:     ptr("event_name"),
 							Type:     ptr("text"),
@@ -205,7 +205,7 @@ func TestAlterColumnMultipleSubOperations(t *testing.T) {
 							Column:   "name",
 							Nullable: ptr(false),
 							Default:  nullable.NewNullableWithValue("'default'"),
-							Up:       "(SELECT CASE WHEN name IS NULL THEN 'rewritten by up SQL' ELSE name END)",
+							Up:       "SELECT CASE WHEN name IS NULL THEN 'rewritten by up SQL' ELSE name END",
 						},
 					},
 				},
@@ -411,7 +411,7 @@ func TestAlterColumnMultipleSubOperations(t *testing.T) {
 						&migrations.OpAlterColumn{
 							Table:    "people",
 							Column:   "manages",
-							Up:       "(SELECT CASE WHEN manages IS NULL THEN 1 ELSE manages END)",
+							Up:       "SELECT CASE WHEN manages IS NULL THEN 1 ELSE manages END",
 							Down:     "manages",
 							Nullable: ptr(false),
 							References: &migrations.ForeignKeyReference{
diff --git a/pkg/migrations/op_change_type_test.go b/pkg/migrations/op_change_type_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/xataio/pgroll/internal/testutils"
 
 	"github.com/stretchr/testify/assert"
+
 	"github.com/xataio/pgroll/pkg/migrations"
 	"github.com/xataio/pgroll/pkg/roll"
 )
@@ -390,7 +391,7 @@ func TestChangeColumnType(t *testing.T) {
 							Column: "age",
 							Type:   ptr("integer"),
 							Up:     "CAST(age AS integer)",
-							Down:   "(SELECT CASE WHEN age < 100 THEN age::text ELSE '0' END)",
+							Down:   "SELECT CASE WHEN age < 100 THEN age::text ELSE '0' END",
 						},
 					},
 				},
diff --git a/pkg/migrations/op_create_constraint_test.go b/pkg/migrations/op_create_constraint_test.go
@@ -296,7 +296,7 @@ func TestCreateConstraint(t *testing.T) {
 							Columns: []string{"name", "email"},
 							Up: map[string]string{
 								"name":  "name",
-								"email": "(SELECT CASE WHEN email ~ '@' THEN email ELSE email || '@example.com' END)",
+								"email": "SELECT CASE WHEN email ~ '@' THEN email ELSE email || '@example.com' END",
 							},
 							Down: map[string]string{
 								"name":  "name",
diff --git a/pkg/migrations/op_drop_constraint_test.go b/pkg/migrations/op_drop_constraint_test.go
diff --git a/pkg/migrations/op_drop_not_null_test.go b/pkg/migrations/op_drop_not_null_test.go
diff --git a/pkg/migrations/op_set_check_test.go b/pkg/migrations/op_set_check_test.go
diff --git a/pkg/migrations/op_set_comment_test.go b/pkg/migrations/op_set_comment_test.go
diff --git a/pkg/migrations/op_set_fk_test.go b/pkg/migrations/op_set_fk_test.go
diff --git a/pkg/migrations/op_set_notnull_test.go b/pkg/migrations/op_set_notnull_test.go
diff --git a/pkg/migrations/trigger.go b/pkg/migrations/trigger.go

Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@`
`6`	`6`	`"table": "reviews",`
`7`	`7`	`"column": "review",`
`8`	`8`	`"nullable": false,`
`9`		`- "up": "(SELECT CASE WHEN review IS NULL THEN product \|\| ' is good' ELSE review END)",`
	`9`	`+ "up": "SELECT CASE WHEN review IS NULL THEN product \|\| ' is good' ELSE review END",`
`10`	`10`	`"down": "review"`
`11`	`11`	`}`
`12`	`12`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@`
`11`	`11`	`"column": "id",`
`12`	`12`	`"on_delete": "CASCADE"`
`13`	`13`	`},`
`14`		`- "up": "(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)",`
	`14`	`+ "up": "SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END",`
`15`	`15`	`"down": "user_id"`
`16`	`16`	`}`
`17`	`17`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@`
`9`	`9`	`"name": "title_length",`
`10`	`10`	`"constraint": "length(title) > 3"`
`11`	`11`	`},`
`12`		`- "up": "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)",`
	`12`	`+ "up": "SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END",`
`13`	`13`	`"down": "title"`
`14`	`14`	`}`
`15`	`15`	`}`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@`
`6`	`6`	`"table": "posts",`
`7`	`7`	`"name": "title_length",`
`8`	`8`	`"up": "title",`
`9`		`- "down": "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)"`
	`9`	`+ "down": "SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END"`
`10`	`10`	`}`
`11`	`11`	`}`
`12`	`12`	`]`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@`
`6`	`6`	`"table": "posts",`
`7`	`7`	`"name": "fk_users_id",`
`8`	`8`	`"up": "user_id",`
`9`		`- "down": "(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)"`
	`9`	`+ "down": "SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END"`
`10`	`10`	`}`
`11`	`11`	`}`
`12`	`12`	`]`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`"column": "title",`
`8`	`8`	`"nullable": true,`
`9`	`9`	`"up": "title",`
`10`		`- "down": "(SELECT CASE WHEN title IS NULL THEN 'placeholder title' ELSE title END)"`
	`10`	`+ "down": "SELECT CASE WHEN title IS NULL THEN 'placeholder title' ELSE title END"`
`11`	`11`	`}`
`12`	`12`	`}`
`13`	`13`	`]`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@`
`17`	`17`	`"name": "event_name_length",`
`18`	`18`	`"constraint": "length(name) > 3"`
`19`	`19`	`},`
`20`		`- "up": "(SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END)",`
	`20`	`+ "up": "SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END",`
`21`	`21`	`"down": "event_name"`
`22`	`22`	`}`
`23`	`23`	`}`