Skip to content

Commit

Permalink
Update JSON schema to specify that not all Column fields are requir…
Browse files Browse the repository at this point in the history
…ed (#265)

Update the JSON schema in `schema.json` to state that only `name` and
`type` are required fields when specifying a column.

Previously `required` was set to `["name", "nullable", "pk", "type",
"unique"]` resulting in some valid `create_table` and `add_column`
migrations failing to validate.

Add two new testcases to the JSON schema tests to cover 'create_table'
and 'add_column' operations.
  • Loading branch information
andrew-farries committed Feb 2, 2024
1 parent d00d804 commit d06a0af
Show file tree
Hide file tree
Showing 25 changed files with 303 additions and 216 deletions.
22 changes: 22 additions & 0 deletions pkg/jsonschema/testdata/add-column-1.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
This is a valid 'add_column' migration.
Some optional column fields are specified and some aren't.

-- create_table.json --
{
"name": "migration_name",
"operations": [
{
"add_column": {
"table": "reviews",
"column": {
"name": "rating",
"type": "text",
"default": "0"
}
}
}
]
}

-- valid --
true
33 changes: 33 additions & 0 deletions pkg/jsonschema/testdata/create-table-1.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
This is a valid 'create_table' migration.
Some optional column fields are specified and some aren't.

-- create_table.json --
{
"name": "migration_name",
"operations": [
{
"create_table": {
"name": "posts",
"columns": [
{
"name": "id",
"type": "serial",
"pk": true
},
{
"name": "title",
"type": "varchar(255)"
},
{
"name": "user_id",
"type": "integer",
"nullable": true
}
]
}
}
]
}

-- valid --
true
24 changes: 24 additions & 0 deletions pkg/migrations/column.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// SPDX-License-Identifier: Apache-2.0

package migrations

func (c *Column) IsNullable() bool {
if c.Nullable != nil {
return *c.Nullable
}
return false
}

func (c *Column) IsUnique() bool {
if c.Unique != nil {
return *c.Unique
}
return false
}

func (c *Column) IsPrimaryKey() bool {
if c.Pk != nil {
return *c.Pk
}
return false
}
12 changes: 6 additions & 6 deletions pkg/migrations/op_add_column.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (o *OpAddColumn) Start(ctx context.Context, conn *sql.DB, stateSchema strin
}
}

if !o.Column.Nullable && o.Column.Default == nil {
if !o.Column.IsNullable() && o.Column.Default == nil {
if err := addNotNullConstraint(ctx, conn, o.Table, o.Column.Name, TemporaryName(o.Column.Name)); err != nil {
return fmt.Errorf("failed to add not null constraint: %w", err)
}
Expand Down Expand Up @@ -84,7 +84,7 @@ func (o *OpAddColumn) Complete(ctx context.Context, conn *sql.DB, s *schema.Sche
return err
}

if !o.Column.Nullable && o.Column.Default == nil {
if !o.Column.IsNullable() && o.Column.Default == nil {
_, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE IF EXISTS %s VALIDATE CONSTRAINT %s",
pq.QuoteIdentifier(o.Table),
pq.QuoteIdentifier(NotNullConstraintName(o.Column.Name))))
Expand Down Expand Up @@ -170,11 +170,11 @@ func (o *OpAddColumn) Validate(ctx context.Context, s *schema.Schema) error {
return InvalidPrimaryKeyError{Table: o.Table, Fields: len(pk)}
}

if !o.Column.Nullable && o.Column.Default == nil && o.Up == nil {
if !o.Column.IsNullable() && o.Column.Default == nil && o.Up == nil {
return FieldRequiredError{Name: "up"}
}

if o.Column.Pk {
if o.Column.IsPrimaryKey() {
return errors.New("adding primary key columns is not supported")
}

Expand All @@ -189,8 +189,8 @@ func addColumn(ctx context.Context, conn *sql.DB, o OpAddColumn, t *schema.Table
// - validating the constraint and converting the column to not null
// on migration completion
// This is to avoid unnecessary exclusive table locks.
if !o.Column.Nullable && o.Column.Default == nil {
o.Column.Nullable = true
if !o.Column.IsNullable() && o.Column.Default == nil {
o.Column.Nullable = ptr(true)
}

// Don't add a column with a CHECK constraint directly.
Expand Down
58 changes: 29 additions & 29 deletions pkg/migrations/op_add_column_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ func TestAddColumn(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -46,7 +46,7 @@ func TestAddColumn(t *testing.T) {
Column: migrations.Column{
Name: "age",
Type: "integer",
Nullable: false,
Nullable: ptr(false),
Default: ptr("0"),
Comment: ptr("the age of the user"),
},
Expand Down Expand Up @@ -125,12 +125,12 @@ func TestAddForeignKeyColumn(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -140,7 +140,7 @@ func TestAddForeignKeyColumn(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "quantity",
Expand All @@ -163,7 +163,7 @@ func TestAddForeignKeyColumn(t *testing.T) {
Table: "users",
Column: "id",
},
Nullable: true,
Nullable: ptr(true),
},
},
},
Expand Down Expand Up @@ -227,12 +227,12 @@ func TestAddForeignKeyColumn(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -242,7 +242,7 @@ func TestAddForeignKeyColumn(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "quantity",
Expand All @@ -265,7 +265,7 @@ func TestAddForeignKeyColumn(t *testing.T) {
Table: "users",
Column: "id",
},
Nullable: false,
Nullable: ptr(false),
},
Up: ptr("1"),
},
Expand Down Expand Up @@ -337,12 +337,12 @@ func TestAddColumnWithUpSql(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -357,7 +357,7 @@ func TestAddColumnWithUpSql(t *testing.T) {
Column: migrations.Column{
Name: "description",
Type: "varchar(255)",
Nullable: true,
Nullable: ptr(true),
},
},
},
Expand Down Expand Up @@ -419,12 +419,12 @@ func TestAddColumnWithUpSql(t *testing.T) {
{
Name: "id",
Type: "text",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -439,7 +439,7 @@ func TestAddColumnWithUpSql(t *testing.T) {
Column: migrations.Column{
Name: "description",
Type: "varchar(255)",
Nullable: true,
Nullable: ptr(true),
},
},
},
Expand Down Expand Up @@ -509,12 +509,12 @@ func TestAddNotNullColumnWithNoDefault(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -529,7 +529,7 @@ func TestAddNotNullColumnWithNoDefault(t *testing.T) {
Column: migrations.Column{
Name: "description",
Type: "varchar(255)",
Nullable: false,
Nullable: ptr(false),
},
},
},
Expand Down Expand Up @@ -575,12 +575,12 @@ func TestAddColumnValidation(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -600,7 +600,7 @@ func TestAddColumnValidation(t *testing.T) {
Column: migrations.Column{
Name: "age",
Type: "integer",
Nullable: false,
Nullable: ptr(false),
Default: ptr("0"),
},
},
Expand Down Expand Up @@ -640,7 +640,7 @@ func TestAddColumnValidation(t *testing.T) {
Column: migrations.Column{
Name: "age",
Type: "integer",
Nullable: false,
Nullable: ptr(false),
},
},
},
Expand Down Expand Up @@ -694,12 +694,12 @@ func TestAddColumnWithCheckConstraint(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand Down Expand Up @@ -769,12 +769,12 @@ func TestAddColumnWithComment(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Type: "varchar(255)",
Unique: true,
Unique: ptr(true),
},
},
},
Expand All @@ -788,7 +788,7 @@ func TestAddColumnWithComment(t *testing.T) {
Column: migrations.Column{
Name: "age",
Type: "integer",
Nullable: false,
Nullable: ptr(false),
Default: ptr("0"),
Comment: ptr("the age of the user"),
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/migrations/op_alter_column_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestAlterColumnValidation(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "name",
Expand All @@ -34,7 +34,7 @@ func TestAlterColumnValidation(t *testing.T) {
{
Name: "id",
Type: "serial",
Pk: true,
Pk: ptr(true),
},
{
Name: "title",
Expand Down
Loading

0 comments on commit d06a0af

Please sign in to comment.