Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: replace "id" with model.IDField #604

Merged
merged 6 commits into from Oct 29, 2020
Merged

fix: replace "id" with model.IDField #604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e25179b
fix: replace "id" with model.IDField
zepatrik Oct 28, 2020
68bb93d
fix: model is string
zepatrik Oct 28, 2020
6965b00
fix: model is pointer to slice
zepatrik Oct 28, 2020
91a5f8e
tests: add test cases
zepatrik Oct 28, 2020
15e0292
tests: improve assertions
zepatrik Oct 29, 2020
c4b8e6a
revert: model iteration columns construction
zepatrik Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions columns/columns.go
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ type Columns struct {
lock *sync.RWMutex
TableName string
TableAlias string
IDField string
}

// Add a column to the list.
Expand Down Expand Up @@ -74,7 +75,7 @@ func (c *Columns) Add(names ...string) []*Column {
} else if xs[1] == "w" {
col.Readable = false
}
} else if col.Name == "id" {
} else if col.Name == c.IDField {
col.Writeable = false
}

Expand All @@ -98,7 +99,7 @@ func (c *Columns) Remove(names ...string) {

// Writeable gets a list of the writeable columns from the column list.
func (c Columns) Writeable() *WriteableColumns {
w := &WriteableColumns{NewColumnsWithAlias(c.TableName, c.TableAlias)}
w := &WriteableColumns{NewColumnsWithAlias(c.TableName, c.TableAlias, c.IDField)}
for _, col := range c.Cols {
if col.Writeable {
w.Cols[col.Name] = col
Expand All @@ -109,7 +110,7 @@ func (c Columns) Writeable() *WriteableColumns {

// Readable gets a list of the readable columns from the column list.
func (c Columns) Readable() *ReadableColumns {
w := &ReadableColumns{NewColumnsWithAlias(c.TableName, c.TableAlias)}
w := &ReadableColumns{NewColumnsWithAlias(c.TableName, c.TableAlias, c.IDField)}
for _, col := range c.Cols {
if col.Readable {
w.Cols[col.Name] = col
Expand Down Expand Up @@ -157,17 +158,18 @@ func (c Columns) SymbolizedString() string {
}

// NewColumns constructs a list of columns for a given table name.
func NewColumns(tableName string) Columns {
return NewColumnsWithAlias(tableName, "")
func NewColumns(tableName, idField string) Columns {
return NewColumnsWithAlias(tableName, "", idField)
}

// NewColumnsWithAlias constructs a list of columns for a given table
// name, using a given alias for the table.
func NewColumnsWithAlias(tableName string, tableAlias string) Columns {
func NewColumnsWithAlias(tableName, tableAlias, idField string) Columns {
return Columns{
lock: &sync.RWMutex{},
Cols: map[string]*Column{},
TableName: tableName,
TableAlias: tableAlias,
IDField: idField,
}
}
10 changes: 5 additions & 5 deletions columns/columns_for_struct.go
View file
Open in desktop
Expand Up @@ -6,17 +6,17 @@ import (

// ForStruct returns a Columns instance for
// the struct passed in.
func ForStruct(s interface{}, tableName string) (columns Columns) {
return ForStructWithAlias(s, tableName, "")
func ForStruct(s interface{}, tableName, idField string) (columns Columns) {
return ForStructWithAlias(s, tableName, "", idField)
}

// ForStructWithAlias returns a Columns instance for the struct passed in.
// If the tableAlias is not empty, it will be used.
func ForStructWithAlias(s interface{}, tableName string, tableAlias string) (columns Columns) {
columns = NewColumnsWithAlias(tableName, tableAlias)
func ForStructWithAlias(s interface{}, tableName, tableAlias, idField string) (columns Columns) {
columns = NewColumnsWithAlias(tableName, tableAlias, idField)
defer func() {
if r := recover(); r != nil {
columns = NewColumnsWithAlias(tableName, tableAlias)
columns = NewColumnsWithAlias(tableName, tableAlias, idField)
columns.Add("*")
}
}()
Expand Down
12 changes: 6 additions & 6 deletions columns/columns_test.go
View file
Open in desktop
Expand Up @@ -21,16 +21,16 @@ type foos []foo
func Test_Column_MapsSlice(t *testing.T) {
r := require.New(t)

c1 := columns.ForStruct(&foo{}, "foo")
c2 := columns.ForStruct(&foos{}, "foo")
c1 := columns.ForStruct(&foo{}, "foo", "id")
c2 := columns.ForStruct(&foos{}, "foo", "id")
r.Equal(c1.String(), c2.String())
}

func Test_Columns_Basics(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is missing two tests:

  • PK is id and the ID field can not be written but can be read.
  • PK is notid and the ID field ca be written and read but notid can not be written.
    have a test where the PK is not "id" and you are not able to update that particular PK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added cases but not everything is possible like you expect, see #556

r := require.New(t)

for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
r.Equal(len(c.Cols), 4)
r.Equal(c.Cols["first_name"], &columns.Column{Name: "first_name", Writeable: false, Readable: true, SelectSQL: "first_name as f"})
r.Equal(c.Cols["LastName"], &columns.Column{Name: "LastName", Writeable: true, Readable: true, SelectSQL: "foo.LastName"})
Expand All @@ -43,7 +43,7 @@ func Test_Columns_Add(t *testing.T) {
r := require.New(t)

for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
r.Equal(len(c.Cols), 4)
c.Add("foo", "first_name")
r.Equal(len(c.Cols), 5)
Expand All @@ -55,7 +55,7 @@ func Test_Columns_Remove(t *testing.T) {
r := require.New(t)

for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
r.Equal(len(c.Cols), 4)
c.Remove("foo", "first_name")
r.Equal(len(c.Cols), 3)
Expand All @@ -75,7 +75,7 @@ func (fooQuoter) Quote(key string) string {
func Test_Columns_Sorted(t *testing.T) {
r := require.New(t)

c := columns.ForStruct(fooWithSuffix{}, "fooWithSuffix")
c := columns.ForStruct(fooWithSuffix{}, "fooWithSuffix", "id")
r.Equal(len(c.Cols), 2)
r.Equal(c.SymbolizedString(), ":amount, :amount_units")
r.Equal(c.String(), "amount, amount_units")
Expand Down
6 changes: 3 additions & 3 deletions columns/readable_columns_test.go
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ import (
func Test_Columns_ReadableString(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Readable().String()
r.Equal(u, "LastName, first_name, read")
}
Expand All @@ -19,7 +19,7 @@ func Test_Columns_ReadableString(t *testing.T) {
func Test_Columns_Readable_SelectString(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Readable().SelectString()
r.Equal(u, "first_name as f, foo.LastName, foo.read")
}
Expand All @@ -28,7 +28,7 @@ func Test_Columns_Readable_SelectString(t *testing.T) {
func Test_Columns_ReadableString_Symbolized(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Readable().SymbolizedString()
r.Equal(u, ":LastName, :first_name, :read")
}
zepatrik marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 4 additions & 4 deletions columns/writeable_columns_test.go
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ import (
func Test_Columns_WriteableString_Symbolized(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Writeable().SymbolizedString()
r.Equal(u, ":LastName, :write")
}
Expand All @@ -19,7 +19,7 @@ func Test_Columns_WriteableString_Symbolized(t *testing.T) {
func Test_Columns_UpdateString(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Writeable().UpdateString()
r.Equal(u, "LastName = :LastName, write = :write")
}
Expand All @@ -35,7 +35,7 @@ func Test_Columns_QuotedUpdateString(t *testing.T) {
r := require.New(t)
q := testQuoter{}
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Writeable().QuotedUpdateString(q)
r.Equal(u, "\"LastName\" = :LastName, \"write\" = :write")
}
Expand All @@ -44,7 +44,7 @@ func Test_Columns_QuotedUpdateString(t *testing.T) {
func Test_Columns_WriteableString(t *testing.T) {
r := require.New(t)
for _, f := range []interface{}{foo{}, &foo{}} {
c := columns.ForStruct(f, "foo")
c := columns.ForStruct(f, "foo", "id")
u := c.Writeable().String()
r.Equal(u, "LastName, write")
}
zepatrik marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
10 changes: 5 additions & 5 deletions executors.go
View file
Open in desktop
Expand Up @@ -228,7 +228,7 @@ func (c *Connection) Create(model interface{}, excludeColumns ...string) error {
}

tn := m.TableName()
cols := columns.ForStructWithAlias(m.Value, tn, m.As)
cols := m.Columns()

if tn == sm.TableName() {
cols.Remove(excludeColumns...)
Expand Down Expand Up @@ -350,8 +350,8 @@ func (c *Connection) Update(model interface{}, excludeColumns ...string) error {
}

tn := m.TableName()
cols := columns.ForStructWithAlias(model, tn, m.As)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the outer model instead of the iteration model. Not sure if this was a bug or intended behavior but I assume it should have been the current model within the iteration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this has had problems in the past where TableName() had to be defined on the slice type or something similar. I agree that this should be m and not model. Can you please add a failing test case for this?

Copy link
Contributor Author

@zepatrik zepatrik Oct 29, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it actually does not matter. It is not possible to pass a interface{} slice, because that breaks reflection everywhere. So only slices of a struct type are allowed, and it also makes sense to only allow them. Therefore it resembles the exact same behavior as before in my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know too little about reflection to judge this statement, so I would prefer reverting this to the original behavior just in case this breaks something somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, reverted

cols.Remove("id", "created_at")
cols := m.Columns()
cols.Remove(m.IDField(), "created_at")

if tn == sm.TableName() {
cols.Remove(excludeColumns...)
Expand Down Expand Up @@ -393,11 +393,11 @@ func (c *Connection) UpdateColumns(model interface{}, columnNames ...string) err

cols := columns.Columns{}
if len(columnNames) > 0 && tn == sm.TableName() {
cols = columns.NewColumnsWithAlias(tn, m.As)
cols = columns.NewColumnsWithAlias(tn, m.As, sm.IDField())
cols.Add(columnNames...)

} else {
cols = columns.ForStructWithAlias(model, tn, m.As)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, model was the outer model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/gobuffalo/pop/pull/604/files#r513381638

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #604 (comment)

cols = m.Columns()
}
cols.Remove("id", "created_at")

Expand Down
5 changes: 5 additions & 0 deletions model.go
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@ package pop

import (
"fmt"
"github.com/gobuffalo/pop/v5/columns"
"github.com/pkg/errors"
"reflect"
"sync"
Expand Down Expand Up @@ -101,6 +102,10 @@ func (m *Model) TableName() string {
return tableMap[cacheKey]
}

func (m *Model) Columns() columns.Columns {
return columns.ForStructWithAlias(m.Value, m.TableName(), m.As, m.IDField())
}

func (m *Model) cacheKey(t reflect.Type) string {
return t.PkgPath() + "." + t.Name()
}
Expand Down
4 changes: 2 additions & 2 deletions sql_builder.go
View file
Open in desktop
Expand Up @@ -229,15 +229,15 @@ func (sq *sqlBuilder) buildColumns() columns.Columns {
if ok && cols.TableAlias == asName {
return cols
}
cols = columns.ForStructWithAlias(sq.Model.Value, tableName, asName)
cols = columns.ForStructWithAlias(sq.Model.Value, tableName, asName, sq.Model.IDField())
columnCacheMutex.Lock()
columnCache[tableName] = cols
columnCacheMutex.Unlock()
return cols
}

// acl > 0
cols := columns.NewColumns("")
cols := columns.NewColumns("", sq.Model.IDField())
cols.Add(sq.AddColumns...)
return cols
}