Resolve issues in UPDATE and DELETE when using schemas (#618)

* Resolve MySQL issues and improve test migrations
* Bump CockroachDB to maintained and supported versions
Version 2.1 has reached EoL in 2019

Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com>

.github/workflows/tests.yml

@@ -110,12 +110,12 @@ jobs:
         run: |
           mkdir -p crdb/certs
           pushd crdb
-          wget -qO- https://binaries.cockroachdb.com/cockroach-v2.1.0.linux-amd64.tgz | tar  zxv
-          mv cockroach-v2.1.0.linux-amd64/cockroach .
+          wget -qO- https://binaries.cockroachdb.com/cockroach-v20.2.4.linux-amd64.tgz | tar  zxv
+          mv cockroach-v20.2.4.linux-amd64/cockroach .
           ./cockroach cert create-ca --certs-dir certs --ca-key key
           ./cockroach cert create-client root --certs-dir certs --ca-key key
           ./cockroach cert create-node localhost 127.0.0.1 `hostname -s` `hostname -f` --certs-dir certs --ca-key key
-          ./cockroach start --certs-dir certs --listen-addr localhost --port 26259 --http-port 8089 --background
+          ./cockroach start-single-node --certs-dir certs --listen-addr localhost --port 26259 --http-port 8089 --background
           popd
       - name: Build and run soda
         env:
@@ -152,9 +152,9 @@ jobs:
         run: |
           mkdir -p crdb
           pushd crdb
-          wget -qO- https://binaries.cockroachdb.com/cockroach-v2.1.0.linux-amd64.tgz | tar  zxv
-          mv cockroach-v2.1.0.linux-amd64/cockroach .
-          ./cockroach start --insecure --background
+          wget -qO- https://binaries.cockroachdb.com/cockroach-v20.2.4.linux-amd64.tgz | tar  zxv
+          mv cockroach-v20.2.4.linux-amd64/cockroach .
+          ./cockroach start-single-node --insecure --background
           popd
       - name: Build and run soda
         env:

dialect_cockroach.go

@@ -105,7 +105,7 @@ func (p *cockroach) Update(s store, model *Model, cols columns.Columns) error {
 }
 
 func (p *cockroach) Destroy(s store, model *Model) error {
-	stmt := p.TranslateSQL(fmt.Sprintf("DELETE FROM %s WHERE %s", p.Quote(model.TableName()), model.whereID()))
+	stmt := p.TranslateSQL(fmt.Sprintf("DELETE FROM %s AS %s WHERE %s", p.Quote(model.TableName()), model.alias(), model.whereID()))
 	_, err := genericExec(s, stmt, model.ID())
 	return err
 }

dialect_common.go

@@ -99,7 +99,7 @@ func genericCreate(s store, model *Model, cols columns.Columns, quoter quotable)
 }
 
 func genericUpdate(s store, model *Model, cols columns.Columns, quoter quotable) error {
-	stmt := fmt.Sprintf("UPDATE %s SET %s WHERE %s", quoter.Quote(model.TableName()), cols.Writeable().QuotedUpdateString(quoter), model.whereNamedID())
+	stmt := fmt.Sprintf("UPDATE %s AS %s SET %s WHERE %s", quoter.Quote(model.TableName()), model.alias(), cols.Writeable().QuotedUpdateString(quoter), model.whereNamedID())
 	log(logging.SQL, stmt, model.ID())
 	_, err := s.NamedExec(stmt, model.Value)
 	if err != nil {
@@ -109,7 +109,7 @@ func genericUpdate(s store, model *Model, cols columns.Columns, quoter quotable)
 }
 
 func genericDestroy(s store, model *Model, quoter quotable) error {
-	stmt := fmt.Sprintf("DELETE FROM %s WHERE %s", quoter.Quote(model.TableName()), model.whereID())
+	stmt := fmt.Sprintf("DELETE FROM %s AS %s WHERE %s", quoter.Quote(model.TableName()), model.alias(), model.whereID())
 	_, err := genericExec(s, stmt, model.ID())
 	if err != nil {
 		return err

dialect_mysql.go

@@ -91,7 +91,9 @@ func (m *mysql) Update(s store, model *Model, cols columns.Columns) error {
 }
 
 func (m *mysql) Destroy(s store, model *Model) error {
-	return errors.Wrap(genericDestroy(s, model, m), "mysql destroy")
+	stmt := fmt.Sprintf("DELETE FROM %s  WHERE %s = ?", m.Quote(model.TableName()), model.IDField())
+	_, err := genericExec(s, stmt, model.ID())
+	return errors.Wrap(err, "mysql destroy")
 }
 
 func (m *mysql) SelectOne(s store, model *Model, query Query) error {

dialect_postgresql.go

@@ -92,7 +92,7 @@ func (p *postgresql) Update(s store, model *Model, cols columns.Columns) error {
 }
 
 func (p *postgresql) Destroy(s store, model *Model) error {
-	stmt := p.TranslateSQL(fmt.Sprintf("DELETE FROM %s WHERE %s", p.Quote(model.TableName()), model.whereID()))
+	stmt := p.TranslateSQL(fmt.Sprintf("DELETE FROM %s AS %s WHERE %s", p.Quote(model.TableName()), model.alias(), model.whereID()))
 	_, err := genericExec(s, stmt, model.ID())
 	if err != nil {
 		return err

docker-compose.yml

@@ -24,10 +24,9 @@ services:
     volumes:
       - ./sqldumps:/docker-entrypoint-initdb.d
   cockroach:
-    image: cockroachdb/cockroach:v2.1.0
+    image: cockroachdb/cockroach:v20.2.4
     ports:
       - "26257:26257"
-      - "8080:8080"
     volumes:
       - "./cockroach-data/roach1:/cockroach/cockroach-data"
-    command: start --insecure
+    command: start-single-node --insecure

model.go

@@ -223,21 +223,19 @@ func (m *Model) touchUpdatedAt() {
 }
 
 func (m *Model) whereID() string {
-	as := m.As
-	if as == "" {
-		as = strings.ReplaceAll(m.TableName(), ".", "_")
-	}
-
-	return fmt.Sprintf("%s.%s = ?", as, m.IDField())
+	return fmt.Sprintf("%s.%s = ?", m.alias(), m.IDField())
 }
 
-func (m *Model) whereNamedID() string {
+func (m *Model) alias() string {
 	as := m.As
 	if as == "" {
 		as = strings.ReplaceAll(m.TableName(), ".", "_")
 	}
+	return as
+}
 
-	return fmt.Sprintf("%s.%s = :%s", as, m.IDField(), m.IDField())
+func (m *Model) whereNamedID() string {
+	return fmt.Sprintf("%s.%s = :%s", m.alias(), m.IDField(), m.IDField())
 }
 
 func (m *Model) isSlice() bool {

model_context_test.go

@@ -17,9 +17,24 @@ type ContextTable struct {
 }
 
 func (t ContextTable) TableName(ctx context.Context) string {
-	// This is singular on purpose! It will checck if the TableName is properly
+	// This is singular on purpose! It will check if the TableName is properly
 	// Respected in slices as well.
-	return "context_prefix_" + ctx.Value("prefix").(string) + "_table"
+	prefix := ctx.Value("prefix").(string)
+
+	// PostgreSQL and CockroachDB support schemas which work like a prefix. For those cases, we use
+	// the schema to ensure that name normalization does not cause query problems.
+	//
+	// Since this works only for those two databases, we use underscore for the rest.
+	//
+	// While this schema is hardcoded, it would have been too difficult to add this special
+	// case to the migrations.
+	switch PDB.Dialect.Name() {
+	case nameCockroach:
+		fallthrough
+	case namePostgreSQL:
+		prefix = prefix + "." + prefix
+	}
+	return "context_prefix_" + prefix + "_table"
 }
 
 func Test_ModelContext(t *testing.T) {
@@ -41,6 +56,7 @@ func Test_ModelContext(t *testing.T) {
 
 			expected := ContextTable{ID: prefix, Value: prefix}
 			c := PDB.WithContext(context.WithValue(context.Background(), "prefix", prefix))
+
 			r.NoError(c.Create(&expected))
 
 			var actual ContextTable
@@ -79,7 +95,7 @@ func Test_ModelContext(t *testing.T) {
 		err := c.Create(&ContextTable{ID: "unknown"})
 		r.Error(err)
 
-		if !strings.Contains(err.Error(), "context_prefix_unknown_table") { // All other databases
+		if !strings.Contains(err.Error(), "context_prefix_unknown") { // All other databases
 			t.Fatalf("Expected error to contain indicator that table does not exist but got: %s", err.Error())
 		}
 	})

sql_builder.go

@@ -125,10 +125,7 @@ func (sq *sqlBuilder) buildfromClauses() fromClauses {
 	fc := sq.Query.fromClauses
 	for _, m := range models {
 		tableName := m.TableName()
-		asName := m.As
-		if asName == "" {
-			asName = strings.Replace(tableName, ".", "_", -1)
-		}
+		asName := m.alias()
 		fc = append(fc, fromClause{
 			From: tableName,
 			As:   asName,
@@ -216,10 +213,7 @@ var columnCacheMutex = sync.RWMutex{}
 
 func (sq *sqlBuilder) buildColumns() columns.Columns {
 	tableName := sq.Model.TableName()
-	asName := sq.Model.As
-	if asName == "" {
-		asName = strings.Replace(tableName, ".", "_", -1)
-	}
+	asName := sq.Model.alias()
 	acl := len(sq.AddColumns)
 	if acl == 0 {
 		columnCacheMutex.RLock()

testdata/migrations/20210104145901_context_tables.up.fizz

@@ -1,9 +1,30 @@
-create_table("context_prefix_a_table") {
-    t.Column("id", "string", { primary: true })
-    t.Column("value", "string")
-}
-
-create_table("context_prefix_b_table") {
-    t.Column("id", "string", { primary: true })
-    t.Column("value", "string")
-}
+{{ if eq .Dialect "sqlite3" }}
+    create_table("context_prefix_a_table") {
+        t.Column("id", "string", { primary: true })
+        t.Column("value", "string")
+    }
+
+    create_table("context_prefix_b_table") {
+        t.Column("id", "string", { primary: true })
+        t.Column("value", "string")
+    }
+{{ end }}
+
+{{ if eq .Dialect "mysql" }}
+    create_table("context_prefix_a_table") {
+        t.Column("id", "string", { primary: true })
+        t.Column("value", "string")
+    }
+
+    create_table("context_prefix_b_table") {
+        t.Column("id", "string", { primary: true })
+        t.Column("value", "string")
+    }
+{{ end }}
+
+{{ if eq .Dialect "postgres" }}
+    sql("CREATE SCHEMA IF NOT EXISTS \"context_prefix_a\";COMMIT TRANSACTION;BEGIN TRANSACTION;")
+    sql("CREATE SCHEMA IF NOT EXISTS \"context_prefix_b\";COMMIT TRANSACTION;BEGIN TRANSACTION;")
+    sql("CREATE TABLE \"context_prefix_a\".\"a_table\" (id character varying(255) NOT NULL, value character varying(255) NOT NULL, created_at timestamp without time zone NOT NULL, updated_at timestamp without time zone NOT NULL);COMMIT TRANSACTION;BEGIN TRANSACTION;")
+    sql("CREATE TABLE \"context_prefix_b\".\"b_table\" (id character varying(255) NOT NULL, value character varying(255) NOT NULL, created_at timestamp without time zone NOT NULL, updated_at timestamp without time zone NOT NULL);COMMIT TRANSACTION;BEGIN TRANSACTION;")
+{{ end }}