Introduce ALTER TABLE query

[bevzzz]

This PR introduces ALTER TABLE query as part of the effort to bring auto-migration to bun, see #456. Therefore, only a limited scope of the functionality related to modifying a database schema is supported in this step.

Changes also include:

• minor refactor of some unit tests
• some typos fixed
• tiny formatting changes (my IDE has added those, let me know if they do not fit the style and I should remove them)

[bevzzz]

Something I can't make my up about is this: When building ALTER COLUMN SET DATA TYPE new_type, which option should we provide?

// Similarly to Column in SelectQuery, but we pass `typ` as `schema.Safe(typ)` rather than `schema.UnsafeIdent(typ)`
func (q *AlterColumnSubquery) Type(typ string) *AlterColumnSubquery {
    q.modification = schema.QueryWithArgs{
	Query: "SET DATA TYPE ?",
        Args:  []interface{}{schema.Safe(typ)},
    }
}  

// Similarly to ColumnExpr, but we append our SET DATA TYPE in front of the user's `query`
func (q *AlterColumnSubquery) TypeExpr(query string, args ...interface{}) *AlterColumnSubquery {
    	query = fmt.Sprintf("SET DATA TYPE %s", query)
	q.modification = schema.SafeQuery(query, args)
} 

It is currently done in the 1st way, but I dislike it, because it is not being explicit (in the method's name) about the fact that the input is treated as a "safe" expression.
The second way is somewhat similar in the way that Order() is handled in SelectQuery in that it manipulates user's input before wrapping it in schema.QueryWithArgs, so I would be somehow leaning its way.

What do you think?

I'm using SET DATA TYPE as an example here, but there're other cases (such as ADD CONTRAINT or SET DEFAULT) that follow roughly the same pattern.

[vmihailenco]

I am not sure, but we can start with collecting examples for different RDBMS.

PostgreSQL:

ALTER TABLE table_name
ALTER COLUMN column_name1 [SET DATA] TYPE new_data_type,
ALTER COLUMN column_name2 [SET DATA] TYPE new_data_type,

MySQL:

ALTER TABLE table_name    
MODIFY COLUMN column_name1 data_type,  
MODIFY COLUMN column_name2 data_type ... ;   

MSSQL:

ALTER TABLE table_name
ALTER COLUMN column_name datatype;

SQLite does not support it and requires creating a new table and copying existing data into it.

So to work on all supported RDBMS it should be something like:

db.NewAlterTable().AlterColumnType(columnName, columnType)

At least that is my line of thinking without looking at the PR.

[vmihailenco]

After looking at the PR it looks like it makes sense to split AlterTableQuery into separate queries, e.g.:

db.NewRenameTable().Table("old_table_name").To("new_table_name")
db.NewRenameTable().Model(&model).To("new_table_name")
db.NewAlterColumnType().Model(&model).Column("name1", "type1").Column("name2", "type2")
db.NewDropColumn().Model(&model).Column("col1").Column("col2")

That would make API cleaner and we won't need chainable/parent queries any more. WDYT?

[bevzzz]

That would make API cleaner and we won't need chainable/parent queries any more. WDYT?

tldr; Yes, I definitely agree that it would make sense to handle each modification separately, primarily because of the stark differences in syntax and degree of support for ALTER TABLE in the 4 DBMS.

Syntax and feature support

I am not sure, but we can start with collecting examples for different RDBMS.

Following your suggestion, I've put up the examples for most of the queries from this list in the dialects Bun supports. I'm posting them here as a convenient summary and also as a reference for future development:

RENAME TABLE

-- PostgresSQL
ALTER TABLE t RENAME TO new;

-- MySQL
ALTER TABLE t RENAME TO new;
-- or, for multiple renamed tables:
RENAME TABLE
	t TO new,
	d TO dew;

-- MSSQL
EXEC sp_rename 't', 'new', 'TABLE';

-- SQLite
ALTER TABLE t RENAME TO new;

RENAME COLUMN

-- PostgresSQL
ALTER TABLE t RENAME COLUMN a TO new_a;

-- MySQL
ALTER TABLE t RENAME COLUMN a TO new_a;

-- MSSQL
EXEC sp_rename 't.a', 'col_a', 'COLUMN'

-- SQLite
ALTER TABLE t RENAME COLUMN a TO new_a;

CHANGE COLUMN TYPE

-- PostgresSQL
ALTER TABLE t
	ALTER COLUMN a SET DATA TYPE new_data_type,
	ALTER COLUMN b SET DATA TYPE new_data_type;

-- MySQL
ALTER TABLE t
	MODIFY a new_data_type,
	MODIFY b new_data_type;

-- MSSQL
ALTER TABLE table_name ALTER COLUMN a new_data_type;
ALTER TABLE table_name ALTER COLUMN b new_data_type;


-- SQLite
Not supported in the standard API, probably something like this:
BEGIN TRANSACTION
ALTER TABLE t ADD COLUMN __temp_a new_data_type_a;
UPDATE t SET __temp_a = a;
ALTER TABLE t DROP COLUMN a;
ALTER TABLE t RENAME COLUMN __temp_a TO a;
COMMIT;

ADD COLUMN

-- PostgresSQL
ALTER TABLE t
	ADD COLUMN a data_type,
	ADD COLUMN b data_type;

-- MySQL
ALTER TABLE t
	ADD COLUMN a data_type,
	ADD COLUMN b data_type;

-- MSSQL
ALTER TABLE t
ADD 
	a data_type,
	b data_type;

-- SQLite
ALTER TABLE t ADD COLUMN a data_type;
ALTER TABLE t ADD COLUMN b data_type;

SET DEFAULT

-- PostgresSQL
ALTER TABLE t 
	ALTER COLUMN a SET DEFAULT 0,
	ALTER COLUMN b SET DEFAULT 0;

-- MySQL
ALTER TABLE t
	ALTER COLUMN a SET DEFAULT 0,
	ALTER COLUMN b SET DEFAULT 0;

-- MSSQL
ALTER TABLE t ADD
	CONSTRAINT my_default_value_a DEFAULT 0 FOR a,
	CONSTRAINT my_default_value_a DEFAULT 0 FOR b; 

-- SQLite
Not supported in the standard API:

BEGIN TRANSACTION
ALTER TABLE t ADD COLUMN __temp_a data_type_a DEFAULT 0;
UPDATE t SET __temp_a = a;
ALTER TABLE t DROP COLUMN a;
ALTER TABLE t RENAME COLUMN __temp_a TO a;
COMMIT;

There are 2 things to notice here:

• SQLite has limited support for ALTER TABLE. Rename table, rename/add/drop column are the only 4 queries supported, the rest must be done by moving data to temp columns/tables and back.
• Some DMBS allow executing multiple statements within one query. This was the reason I've introduced the ChainableSubquery in the first place -- that should've been a mechanism for e.g., changing data types in 5 columns, adding a new column, and dropping a default constraint from another one within a single SQL statement. However, with that not being the case for all 4 DBMS (PostgresSQL and MySQL the most consistent support for "chainable subqueries"), I think it's not worth breaking our backs trying to accommodate that in the Bun's API in this iteration.
• _Different "angles" _. For example, while setting a default is "altering the column definition" for PostgreSQL and MySQL, it is "adding a table constraint" in MSSQL.

In conclusion, I think these APIs for altering DB schema have less commonalities than I have initially hoped for, and in this case it really makes more sense to design Bun's API around specific operations rather than SQL syntactic constructs.

[vmihailenco]

really makes more sense to design Bun's API around specific operations rather than SQL syntactic constructs.

Fully agree. Perhaps even ignore AlterTableQuery altogether and just provide minimal API like:

RenameTable(context.Context, oldName, newName string) error
AddColumn(context.Context, *sqlschema.Column) error
DropColumn(context.Context, colName string) error
ChangeColumnType(context.Context, colName, colType string) error

This way you can do whatever you want in dialects, .e.g. start transactions, create temp tables etc.

[bevzzz]

After giving it some thought here's roughly the solution I came up with:

1. ~~Define AlterTableQuery, which has methods for individual table-modifications~~ ~~The reason I want to continue with the standard bun-query approach is: a) familiar syntax b) the way models are parsed in baseQuery is very helpful and easily reusable~~

Edit: Provide minimal API, as suggested here.

Although baseQuery does a very good job at parsing the models, it's important to remember that at the time any of the ALTER TABLE functions are called, the bun models are likely already modified and the database schema is not yet modified.
Eventually, when auto-migration is introduced, there shouldn't be a need for the higher-level AlterTableQuery at all, as migrations can be done with simple SyncModels((*Model)(nil)).

2. Delegate building the query to individual dialects. Because of the differences in the syntax, they seem like the best place to handle that

type Dialect interface {
    RenameTable(...) []QueryWithArgs
    AddColumn(...)   []QueryWithArgs
}

These methods return a slice of queries, so that, for instance, SQLite could build several queries for something like SET DEFAULT (which is not natively supported).

3. Execute multiple raw queries in a transaction. The "exec" step belongs to bun, because that's the part of the program that holds the db connection.

package bun


func DropDefault(ctx context.Context, db *DB, colName string) {
    // Build query
    ...
    // Do
    if len(q.mod) > 1 {
        // execute raw query at q.mod[0]
    }
    // start a transaction and execute each one of the queries in q.mod
}

Will add implementation for that tomorrow, let's see how that works out.

[vmihailenco]

Delegate building the query to individual dialects. Because of the differences in the syntax, they seem like the best place to handle that

The idea sounds right, but returning []QueryWithArgs looks very weird. It should be much simpler to just execute the query instead of constructing some weird constructs like []QueryWithArgs.

I guess you are worried about not having access to bun.DB in dialects, but we can introduce a separate sub-package that works with bun.DB and does some dispatching for different dialects internally...

[bevzzz]

You're right, I too thought that returning []QueryWithArgs felt awkward. The reason I would like the dialects to only build the query (doesn't have to be a "bun query") itself and to handle its execution in a separate package is something I've mentioned in one of my earlier comments:

Once we've got auto-migrations figured out, we could leverage the fact that all bun's queries implement Stringer and create "up" and "down" SQL-migration files for the user.

The only alternative that comes to my mind atm is to have the dialects return []byte like other QueryAppenders do, but it's only a slight improvement over []QueryWithArgs. WDYT? Is there a more elegant solution?

I guess you are worried about not having access to bun.DB in dialects, but we can introduce a separate sub-package that works with bun.DB and does some dispatching for different dialects internally...

Yes, if we put ALTER TABLE functionality in a sub-separate package, we could just pass bun.DB as an argument.

[vmihailenco]

build the query (doesn't have to be a "bun query") itself and to handle its execution in a separate package

I've missed that, but it would be nice to have. It is not mandatory though and we could start with Go-only migrations.

Perhaps we could achieve that by having some special *sql.DB abstracted under an interface that would record queries instead of executing them. For example, https://github.com/DATA-DOG/go-sqlmock records queries for testing, but we could write them into a file instead.