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.

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

Fix: Each updateFilter must have corresponding bindPlan #373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion db.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func (m *DbMap) readStructColumns(t reflect.Type) (cols []*ColumnMap, primaryKey
isPK = true
case "autoincrement":
isAuto = true
case "notnull":
case "notnull", "not null":
Copy link
Member

Choose a reason for hiding this comment

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

This change is not related to this PR, and it also doesn't have any tests.

isNotNull = true
default:
panic(fmt.Sprintf("Unrecognized tag option for field %v: %v", f.Name, arg))
Expand Down
31 changes: 30 additions & 1 deletion gorp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1586,8 +1586,37 @@ func TestColumnFilter(t *testing.T) {
if inv2.IsPaid {
t.Error("IsPaid shouldn't have been updated")
}
}

inv1.Memo = "d"
inv1.IsPaid = true
_updateColumns(dbmap, func(col *gorp.ColumnMap) bool {
return col.ColumnName == "IsPaid"
}, inv1)

inv3 := &Invoice{}
inv3 = _get(dbmap, inv3, inv1.Id).(*Invoice)
if inv3.Memo != "c" {
t.Errorf("Expected column to be updated (%#v)", inv3)
}
if !inv3.IsPaid {
t.Error("IsPaid should now be updated")
}

// classic update should work
inv1.Memo = "e"
inv1.IsPaid = false
_update(dbmap, inv1)

inv4 := &Invoice{}
inv4 = _get(dbmap, inv4, inv1.Id).(*Invoice)
if inv4.Memo != "e" {
t.Errorf("Expected column to be updated (%#v)", inv4)
}
if inv4.IsPaid {
t.Error("IsPaid should be false again")
}

}
func TestTypeConversionExample(t *testing.T) {
dbmap := initDbMap()
defer dropAndClose(dbmap)
Expand Down
3 changes: 2 additions & 1 deletion table.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ type TableMap struct {
SchemaName string
gotype reflect.Type
Columns []*ColumnMap
keys []*ColumnMap
keys []*ColumnMap // primary key column; can be autoIncrement
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment is related to this PR.

indexes []*IndexMap
uniqueTogether [][]string
version *ColumnMap
insertPlan bindPlan
updatePlan bindPlan
colFilter ColumnFilter // different column filters lead to different updatePlans
deletePlan bindPlan
getPlan bindPlan
dbmap *DbMap
Expand Down
8 changes: 8 additions & 0 deletions table_bindings.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,16 @@ func (t *TableMap) bindInsert(elem reflect.Value) (bindInstance, error) {
}

func (t *TableMap) bindUpdate(elem reflect.Value, colFilter ColumnFilter) (bindInstance, error) {

if colFilter == nil {
if t.colFilter != nil {
t.updatePlan = bindPlan{}
}
t.colFilter = nil
colFilter = acceptAllFilter
} else {
t.updatePlan = bindPlan{} // a new bindPlan everytime; since we cannot compare colFilter funcs
t.colFilter = colFilter
Copy link
Member

Choose a reason for hiding this comment

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

I'm a firm believer in avoiding else at nearly all costs. Branches are really harsh on the mental stack when reading code, and else statements are the worst (especially in go, where they can carry extra context). Can we try something more like:

if t.colFilter != nil || colFilter != nil {
    t.updatePlan = bindPlan{}
}
if colFilter == nil {
    colFilter = acceptAllFilter
}
t.colFilter = colFilter

}

plan := &t.updatePlan
Expand Down