From 954abb2fbfd7ccb0a66e41933db6e7743b9c5bef Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 28 Jan 2021 12:34:42 +0100 Subject: [PATCH] Resolve issues in UPDATE and DELETE when using schemas --- dialect_cockroach.go | 2 +- dialect_common.go | 4 +-- dialect_postgresql.go | 2 +- model.go | 14 ++++----- model_context_test.go | 26 ++++++++++++++++ sql_builder.go | 10 ++---- .../20210104145901_context_tables.up.fizz | 31 +++++++++++++++++++ 7 files changed, 69 insertions(+), 20 deletions(-) diff --git a/dialect_cockroach.go b/dialect_cockroach.go index 610947658..cab020a5a 100644 --- a/dialect_cockroach.go +++ b/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 } diff --git a/dialect_common.go b/dialect_common.go index b34caae80..7da27adb8 100644 --- a/dialect_common.go +++ b/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 diff --git a/dialect_postgresql.go b/dialect_postgresql.go index 3409d4ba9..ccb8e43d1 100644 --- a/dialect_postgresql.go +++ b/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 diff --git a/model.go b/model.go index 1dea97f5b..2b20dace2 100644 --- a/model.go +++ b/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 { diff --git a/model_context_test.go b/model_context_test.go index 6b03a8820..2bd7ffdad 100644 --- a/model_context_test.go +++ b/model_context_test.go @@ -2,6 +2,8 @@ package pop import ( "context" + "io/ioutil" + "os" "strings" "testing" "time" @@ -22,6 +24,14 @@ func (t ContextTable) TableName(ctx context.Context) string { return "context_prefix_" + ctx.Value("prefix").(string) + "_table" } +func dump(t *testing.T) { + f, err := ioutil.TempFile(os.TempDir(), "sql-dump-*.sql") + require.NoError(t, err) + defer f.Close() + t.Logf(f.Name()) + require.NoError(t, PDB.Dialect.DumpSchema(f)) +} + func Test_ModelContext(t *testing.T) { if PDB == nil { t.Skip("skipping integration tests") @@ -39,8 +49,24 @@ func Test_ModelContext(t *testing.T) { t.Run("prefix="+prefix, func(t *testing.T) { r := require.New(t) + // 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 + dump(t) + } + expected := ContextTable{ID: prefix, Value: prefix} c := PDB.WithContext(context.WithValue(context.Background(), "prefix", prefix)) + r.NoError(c.Create(&expected)) var actual ContextTable diff --git a/sql_builder.go b/sql_builder.go index edd05c465..44820f4e6 100644 --- a/sql_builder.go +++ b/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() diff --git a/testdata/migrations/20210104145901_context_tables.up.fizz b/testdata/migrations/20210104145901_context_tables.up.fizz index ae94796ff..f4c21ba06 100644 --- a/testdata/migrations/20210104145901_context_tables.up.fizz +++ b/testdata/migrations/20210104145901_context_tables.up.fizz @@ -7,3 +7,34 @@ 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 context_prefix_a") + sql("CREATE SCHEMA context_prefix_b") + 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)") + 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)") +{{ end }}