From bd17db54435cadfd495134d0110fa5bb0ef11951 Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Tue, 28 Nov 2023 20:23:22 +0100 Subject: [PATCH 1/4] refactor: delegate autoincrement to dialects --- dialect/mssqldialect/dialect.go | 4 ++++ dialect/mysqldialect/dialect.go | 4 ++++ dialect/pgdialect/dialect.go | 4 ++++ dialect/sqlitedialect/dialect.go | 4 ++++ query_table_create.go | 17 +++++------------ schema/dialect.go | 8 ++++++++ 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/dialect/mssqldialect/dialect.go b/dialect/mssqldialect/dialect.go index 540529ce1..6bbe69cd8 100755 --- a/dialect/mssqldialect/dialect.go +++ b/dialect/mssqldialect/dialect.go @@ -137,6 +137,10 @@ func (d *Dialect) DefaultVarcharLen() int { return 255 } +func (d *Dialect) AppendSequence(b []byte) []byte { + return append(b, " IDENTITY"...) +} + func sqlType(field *schema.Field) string { switch field.DiscoveredSQLType { case sqltype.Timestamp: diff --git a/dialect/mysqldialect/dialect.go b/dialect/mysqldialect/dialect.go index 65a4634d3..28797f1c8 100644 --- a/dialect/mysqldialect/dialect.go +++ b/dialect/mysqldialect/dialect.go @@ -180,6 +180,10 @@ func (d *Dialect) DefaultVarcharLen() int { return 255 } +func (d *Dialect) AppendSequence(b []byte) []byte { + return append(b, " AUTO_INCREMENT"...) +} + func sqlType(field *schema.Field) string { if field.DiscoveredSQLType == sqltype.Timestamp { return datetimeType diff --git a/dialect/pgdialect/dialect.go b/dialect/pgdialect/dialect.go index 6ff85e166..4941fb893 100644 --- a/dialect/pgdialect/dialect.go +++ b/dialect/pgdialect/dialect.go @@ -108,3 +108,7 @@ func (d *Dialect) AppendUint32(b []byte, n uint32) []byte { func (d *Dialect) AppendUint64(b []byte, n uint64) []byte { return strconv.AppendInt(b, int64(n), 10) } + +func (d *Dialect) AppendSequence(b []byte) []byte { + return append(b, " GENERATED BY DEFAULT AS IDENTITY"...) +} diff --git a/dialect/sqlitedialect/dialect.go b/dialect/sqlitedialect/dialect.go index 3c809e7a7..ac80282b0 100644 --- a/dialect/sqlitedialect/dialect.go +++ b/dialect/sqlitedialect/dialect.go @@ -91,6 +91,10 @@ func (d *Dialect) DefaultVarcharLen() int { return 0 } +func (d *Dialect) AppendSequence(b []byte) []byte { + return b +} + func fieldSQLType(field *schema.Field) string { switch field.DiscoveredSQLType { case sqltype.SmallInt, sqltype.BigInt: diff --git a/query_table_create.go b/query_table_create.go index 518dbfd1c..2c81c8a7f 100644 --- a/query_table_create.go +++ b/query_table_create.go @@ -178,19 +178,12 @@ func (q *CreateTableQuery) AppendQuery(fmter schema.Formatter, b []byte) (_ []by if field.NotNull { b = append(b, " NOT NULL"...) } - if field.AutoIncrement { - switch { - case fmter.Dialect().Features().Has(feature.AutoIncrement): - b = append(b, " AUTO_INCREMENT"...) - case fmter.Dialect().Features().Has(feature.Identity): - b = append(b, " IDENTITY"...) - } - } - if field.Identity { - if fmter.Dialect().Features().Has(feature.GeneratedIdentity) { - b = append(b, " GENERATED BY DEFAULT AS IDENTITY"...) - } + + if (field.Identity && q.db.HasFeature(feature.GeneratedIdentity)) || + (field.AutoIncrement && (q.db.HasFeature(feature.AutoIncrement) || q.db.HasFeature(feature.Identity))) { + b = q.db.dialect.AppendSequence(b) } + if field.SQLDefault != "" { b = append(b, " DEFAULT "...) b = append(b, field.SQLDefault...) diff --git a/schema/dialect.go b/schema/dialect.go index fea8238dc..72c62c2a5 100644 --- a/schema/dialect.go +++ b/schema/dialect.go @@ -31,6 +31,10 @@ type Dialect interface { AppendJSON(b, jsonb []byte) []byte AppendBool(b []byte, v bool) []byte + // AppendSequence adds the appropriate instruction for the driver to create a sequence + // from which (autoincremented) values for the column will be generated. + AppendSequence(b []byte) []byte + // DefaultVarcharLen should be returned for dialects in which specifying VARCHAR length // is mandatory in queries that modify the schema (CREATE TABLE / ADD COLUMN, etc). // Dialects that do not have such requirement may return 0, which should be interpreted so by the caller. @@ -177,3 +181,7 @@ func (d *nopDialect) IdentQuote() byte { func (d *nopDialect) DefaultVarcharLen() int { return 0 } + +func (d *nopDialect) AppendSequence(b []byte) []byte { + return b +} From 98580cb75081cc6da4467d04beb76b95e312eb22 Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Wed, 29 Nov 2023 13:48:46 +0100 Subject: [PATCH 2/4] feat: support AUTOINCREMENT PKs in SQLite --- dialect/mssqldialect/dialect.go | 2 +- dialect/mysqldialect/dialect.go | 2 +- dialect/pgdialect/dialect.go | 2 +- dialect/sqlitedialect/dialect.go | 18 +++++++++++++++++- .../testdata/snapshots/TestQuery-sqlite-102 | 2 +- .../testdata/snapshots/TestQuery-sqlite-103 | 2 +- .../testdata/snapshots/TestQuery-sqlite-151 | 2 +- .../testdata/snapshots/TestQuery-sqlite-32 | 2 +- .../testdata/snapshots/TestQuery-sqlite-33 | 2 +- .../testdata/snapshots/TestQuery-sqlite-57 | 2 +- .../testdata/snapshots/TestQuery-sqlite-71 | 2 +- query_table_create.go | 5 +++-- schema/dialect.go | 4 ++-- 13 files changed, 32 insertions(+), 15 deletions(-) diff --git a/dialect/mssqldialect/dialect.go b/dialect/mssqldialect/dialect.go index 6bbe69cd8..353417af7 100755 --- a/dialect/mssqldialect/dialect.go +++ b/dialect/mssqldialect/dialect.go @@ -137,7 +137,7 @@ func (d *Dialect) DefaultVarcharLen() int { return 255 } -func (d *Dialect) AppendSequence(b []byte) []byte { +func (d *Dialect) AppendSequence(b []byte, _ *schema.Table, _ *schema.Field) []byte { return append(b, " IDENTITY"...) } diff --git a/dialect/mysqldialect/dialect.go b/dialect/mysqldialect/dialect.go index 28797f1c8..959747d94 100644 --- a/dialect/mysqldialect/dialect.go +++ b/dialect/mysqldialect/dialect.go @@ -180,7 +180,7 @@ func (d *Dialect) DefaultVarcharLen() int { return 255 } -func (d *Dialect) AppendSequence(b []byte) []byte { +func (d *Dialect) AppendSequence(b []byte, _ *schema.Table, _ *schema.Field) []byte { return append(b, " AUTO_INCREMENT"...) } diff --git a/dialect/pgdialect/dialect.go b/dialect/pgdialect/dialect.go index 4941fb893..f100e682c 100644 --- a/dialect/pgdialect/dialect.go +++ b/dialect/pgdialect/dialect.go @@ -109,6 +109,6 @@ func (d *Dialect) AppendUint64(b []byte, n uint64) []byte { return strconv.AppendInt(b, int64(n), 10) } -func (d *Dialect) AppendSequence(b []byte) []byte { +func (d *Dialect) AppendSequence(b []byte, _ *schema.Table, _ *schema.Field) []byte { return append(b, " GENERATED BY DEFAULT AS IDENTITY"...) } diff --git a/dialect/sqlitedialect/dialect.go b/dialect/sqlitedialect/dialect.go index ac80282b0..3bfe500ff 100644 --- a/dialect/sqlitedialect/dialect.go +++ b/dialect/sqlitedialect/dialect.go @@ -39,6 +39,7 @@ func New() *Dialect { feature.InsertOnConflict | feature.TableNotExists | feature.SelectExists | + feature.AutoIncrement | feature.CompositeIn return d } @@ -91,7 +92,22 @@ func (d *Dialect) DefaultVarcharLen() int { return 0 } -func (d *Dialect) AppendSequence(b []byte) []byte { +// AppendSequence adds AUTOINCREMENT keyword to the column definition. As per [documentation], +// AUTOINCREMENT is only valid for INTEGER PRIMARY KEY, and this method will be a noop for other columns. +// +// Because this is a valid construct: +// CREATE TABLE ("id" INTEGER PRIMARY KEY AUTOINCREMENT); +// and this is not: +// CREATE TABLE ("id" INTEGER AUTOINCREMENT, PRIMARY KEY ("id")); +// AppendSequence adds a primary key constraint as a *side-effect*. Callers should expect it to avoid building invalid SQL. +// SQLite also [does not support] AUTOINCREMENT column in composite primary keys. +// +// [documentation]: https://www.sqlite.org/autoinc.html +// [does not support]: https://stackoverflow.com/a/6793274/14726116 +func (d *Dialect) AppendSequence(b []byte, table *schema.Table, field *schema.Field) []byte { + if field.IsPK && len(table.PKs) == 1 && field.CreateTableSQLType == sqltype.Integer { + b = append(b, " PRIMARY KEY AUTOINCREMENT"...) + } return b } diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-102 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-102 index 5083c3448..f82eceee3 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-102 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-102 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, "str" VARCHAR, PRIMARY KEY ("id")) PARTITION BY HASH (id) +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, "str" VARCHAR) PARTITION BY HASH (id) diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-103 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-103 index 45b5197cc..5ae4b403c 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-103 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-103 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, "str" VARCHAR, PRIMARY KEY ("id")) TABLESPACE "fasttablespace" +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, "str" VARCHAR) TABLESPACE "fasttablespace" diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-151 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-151 index 2b1eb5a4b..a6db3a630 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-151 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-151 @@ -1 +1 @@ -CREATE TABLE "users" ("id" INTEGER NOT NULL, PRIMARY KEY ("id")) +CREATE TABLE "users" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT) diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-32 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-32 index b8d7b30f2..4ee7415f1 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-32 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-32 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, "str" VARCHAR, PRIMARY KEY ("id")) +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, "str" VARCHAR) diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-33 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-33 index e7a2407d0..aba20c173 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-33 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-33 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, "struct" VARCHAR, "map" VARCHAR, "slice" VARCHAR, "array" VARCHAR, PRIMARY KEY ("id")) +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, "struct" VARCHAR, "map" VARCHAR, "slice" VARCHAR, "array" VARCHAR) diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-57 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-57 index aa4de6d74..54b748c88 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-57 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-57 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, "str" VARCHAR, PRIMARY KEY ("id"), FOREIGN KEY ("profile_id") REFERENCES "profiles" ("id")) +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, "str" VARCHAR, FOREIGN KEY ("profile_id") REFERENCES "profiles" ("id")) diff --git a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-71 b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-71 index 82c3e47b9..d26907bd7 100644 --- a/internal/dbtest/testdata/snapshots/TestQuery-sqlite-71 +++ b/internal/dbtest/testdata/snapshots/TestQuery-sqlite-71 @@ -1 +1 @@ -CREATE TABLE "models" ("id" INTEGER NOT NULL, PRIMARY KEY ("id")) +CREATE TABLE "models" ("id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT) diff --git a/query_table_create.go b/query_table_create.go index 2c81c8a7f..46d19da30 100644 --- a/query_table_create.go +++ b/query_table_create.go @@ -181,7 +181,7 @@ func (q *CreateTableQuery) AppendQuery(fmter schema.Formatter, b []byte) (_ []by if (field.Identity && q.db.HasFeature(feature.GeneratedIdentity)) || (field.AutoIncrement && (q.db.HasFeature(feature.AutoIncrement) || q.db.HasFeature(feature.Identity))) { - b = q.db.dialect.AppendSequence(b) + b = q.db.dialect.AppendSequence(b, q.table, field) } if field.SQLDefault != "" { @@ -302,7 +302,8 @@ func (q *CreateTableQuery) appendFKConstraints( } func (q *CreateTableQuery) appendPKConstraint(b []byte, pks []*schema.Field) []byte { - if len(pks) == 0 { + // Primary key constraint might've already been created together with AUTOINCREMENT directives. + if len(pks) == 0 || bytes.Contains(b, []byte("PRIMARY KEY")) { return b } diff --git a/schema/dialect.go b/schema/dialect.go index 72c62c2a5..8814313f7 100644 --- a/schema/dialect.go +++ b/schema/dialect.go @@ -33,7 +33,7 @@ type Dialect interface { // AppendSequence adds the appropriate instruction for the driver to create a sequence // from which (autoincremented) values for the column will be generated. - AppendSequence(b []byte) []byte + AppendSequence(b []byte, t *Table, f *Field) []byte // DefaultVarcharLen should be returned for dialects in which specifying VARCHAR length // is mandatory in queries that modify the schema (CREATE TABLE / ADD COLUMN, etc). @@ -182,6 +182,6 @@ func (d *nopDialect) DefaultVarcharLen() int { return 0 } -func (d *nopDialect) AppendSequence(b []byte) []byte { +func (d *nopDialect) AppendSequence(b []byte, _ *Table, _ *Field) []byte { return b } From 2a59927fff2c1b015bce387314fcc480053da9c4 Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Wed, 29 Nov 2023 13:51:56 +0100 Subject: [PATCH 3/4] chore: use succinct syntax --- query_table_create.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/query_table_create.go b/query_table_create.go index 46d19da30..a29c12c92 100644 --- a/query_table_create.go +++ b/query_table_create.go @@ -1,6 +1,7 @@ package bun import ( + "bytes" "context" "database/sql" "fmt" @@ -157,7 +158,7 @@ func (q *CreateTableQuery) AppendQuery(fmter schema.Formatter, b []byte) (_ []by b = append(b, "TEMP "...) } b = append(b, "TABLE "...) - if q.ifNotExists && fmter.Dialect().Features().Has(feature.TableNotExists) { + if q.ifNotExists && fmter.HasFeature(feature.TableNotExists) { b = append(b, "IF NOT EXISTS "...) } b, err = q.appendFirstTable(fmter, b) @@ -179,8 +180,8 @@ func (q *CreateTableQuery) AppendQuery(fmter schema.Formatter, b []byte) (_ []by b = append(b, " NOT NULL"...) } - if (field.Identity && q.db.HasFeature(feature.GeneratedIdentity)) || - (field.AutoIncrement && (q.db.HasFeature(feature.AutoIncrement) || q.db.HasFeature(feature.Identity))) { + if (field.Identity && fmter.HasFeature(feature.GeneratedIdentity)) || + (field.AutoIncrement && (fmter.HasFeature(feature.AutoIncrement) || fmter.HasFeature(feature.Identity))) { b = q.db.dialect.AppendSequence(b, q.table, field) } From 445ac0d245c5a0140f2dd51dac3bf7c2b5189051 Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Sun, 7 Jan 2024 12:21:27 +0100 Subject: [PATCH 4/4] refactor: push the check for PK to the caller #929 --- query_table_create.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/query_table_create.go b/query_table_create.go index a29c12c92..bcb8abcc0 100644 --- a/query_table_create.go +++ b/query_table_create.go @@ -204,7 +204,12 @@ func (q *CreateTableQuery) AppendQuery(fmter schema.Formatter, b []byte) (_ []by } } - b = q.appendPKConstraint(b, q.table.PKs) + // In SQLite AUTOINCREMENT is only valid for INTEGER PRIMARY KEY columns, so it might be that + // a primary key constraint has already been created in dialect.AppendSequence() call above. + // See sqldialect.Dialect.AppendSequence() for more details. + if len(q.table.PKs) > 0 && !bytes.Contains(b, []byte("PRIMARY KEY")) { + b = q.appendPKConstraint(b, q.table.PKs) + } b = q.appendUniqueConstraints(fmter, b) b, err = q.appendFKConstraints(fmter, b) if err != nil { @@ -303,11 +308,6 @@ func (q *CreateTableQuery) appendFKConstraints( } func (q *CreateTableQuery) appendPKConstraint(b []byte, pks []*schema.Field) []byte { - // Primary key constraint might've already been created together with AUTOINCREMENT directives. - if len(pks) == 0 || bytes.Contains(b, []byte("PRIMARY KEY")) { - return b - } - b = append(b, ", PRIMARY KEY ("...) b = appendColumns(b, "", pks) b = append(b, ")"...)