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

feat: support context-aware tablenames #614

Merged
merged 5 commits into from Jan 18, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
38 changes: 15 additions & 23 deletions model.go
Expand Up @@ -4,14 +4,16 @@ import (
"context"
"fmt"
"reflect"
"strings"
"sync"
"time"

nflect "github.com/gobuffalo/flect/name"

"github.com/gobuffalo/pop/v5/columns"
"github.com/pkg/errors"

"github.com/gobuffalo/flect"
nflect "github.com/gobuffalo/flect/name"
"github.com/gofrs/uuid"
)

Expand Down Expand Up @@ -107,6 +109,7 @@ func (m *Model) TableName() string {
if s, ok := m.Value.(string); ok {
return s
}

if n, ok := m.Value.(TableNameAble); ok {
return n.TableName()
}
Expand All @@ -118,21 +121,13 @@ func (m *Model) TableName() string {
return n.TableName(m.ctx)
}

m.isSlice()

if m.tableName != "" {
return m.tableName
}

t := reflect.TypeOf(m.Value)
name, cacheKey := m.typeName(t)

defer tableMapMu.Unlock()
tableMapMu.Lock()

if tableMap[cacheKey] == "" {
m.tableName = nflect.Tableize(name)
tableMap[cacheKey] = m.tableName
}
return tableMap[cacheKey]
Comment on lines -103 to -113
Copy link
Member Author

Choose a reason for hiding this comment

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

@stanislas-m the cache was completely ineffective. The only thing that was cached was the m.tableName = nflect.Tableize(name) statement here.

This does not reflect any value in my opinion as nflect.Tableize() is only doing some simple string parsing and concatenation.

Instead, this caused several hard to find bugs, and introduced a global lock which might even cause lock contestion and congestion in high traffic envs.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, dropping this cache makes sense. :)

return m.typeName(reflect.TypeOf(m.Value))
}

func (m *Model) Columns() columns.Columns {
Expand All @@ -143,7 +138,7 @@ func (m *Model) cacheKey(t reflect.Type) string {
return t.PkgPath() + "." + t.Name()
}

func (m *Model) typeName(t reflect.Type) (name, cacheKey string) {
func (m *Model) typeName(t reflect.Type) (name string) {
if t.Kind() == reflect.Ptr {
t = t.Elem()
}
Expand All @@ -159,26 +154,23 @@ func (m *Model) typeName(t reflect.Type) (name, cacheKey string) {
if el.Implements(reflect.TypeOf(tableNameAble).Elem()) {
v := reflect.New(el)
out := v.MethodByName("TableName").Call([]reflect.Value{})
name := out[0].String()
if tableMap[m.cacheKey(el)] == "" {
tableMap[m.cacheKey(el)] = name
}
return out[0].String()
}

// validates if the elem of slice or array implements TableNameAbleWithContext interface.
var tableNameAbleWithContext *TableNameAbleWithContext
if el.Implements(reflect.TypeOf(tableNameAbleWithContext).Elem()) {
v := reflect.New(el)
out := v.MethodByName("TableName").Call([]reflect.Value{reflect.ValueOf(m.ctx)})
name := out[0].String()
if tableMap[m.cacheKey(el)] == "" {
tableMap[m.cacheKey(el)] = name
}
return out[0].String()

// We do not want to cache contextualized TableNames because that would break
// the contextualization.
}

return el.Name(), m.cacheKey(el)
return nflect.Tableize(name)
default:
return t.Name(), m.cacheKey(t)
return nflect.Tableize(t.Name())
}
}

Expand Down
25 changes: 25 additions & 0 deletions model_context_test.go
Expand Up @@ -17,6 +17,8 @@ type ContextTable struct {
}

func (t ContextTable) TableName(ctx context.Context) string {
// This is singular on purpose! It will checck if the TableName is properly
// Respected in slices as well.
return "context_prefix_" + ctx.Value("prefix").(string) + "_table"
}

Expand Down Expand Up @@ -81,4 +83,27 @@ func Test_ModelContext(t *testing.T) {
t.Fatalf("Expected error to contain indicator that table does not exist but got: %s", err.Error())
}
})

t.Run("cache_busting", func(t *testing.T) {
r := require.New(t)

var expectedA, expectedB ContextTable
expectedA.ID = "expectedA"
expectedB.ID = "expectedB"

cA := PDB.WithContext(context.WithValue(context.Background(), "prefix", "a"))
r.NoError(cA.Create(&expectedA))

cB := PDB.WithContext(context.WithValue(context.Background(), "prefix", "b"))
r.NoError(cB.Create(&expectedB))

var actualA, actualB []ContextTable
r.NoError(cA.All(&actualA))
r.NoError(cB.All(&actualB))

r.Len(cA, 1)
r.Len(cB, 1)

r.NotEqual(cA.ID, cB.ID, "if these are equal context switching did not work")
})
}
12 changes: 6 additions & 6 deletions model_test.go
Expand Up @@ -61,13 +61,13 @@ func Test_TableNameCache(t *testing.T) {

// A failing test case for #477
func Test_TableNameContextCache(t *testing.T) {
ctx := context.WithValue(context.Background(), "name", "context-table")
ctx := context.WithValue(context.Background(), "name", "context_table")

r := assert.New(t)
r.Equal("context-table-usera", (&Model{Value: ac.User{}, ctx: ctx}).TableName())
r.Equal("context-table-userb", (&Model{Value: bc.User{}, ctx: ctx}).TableName())
r.Equal("context-table-usera", (&Model{Value: []ac.User{}, ctx: ctx}).TableName())
r.Equal("context-table-userb", (&Model{Value: []bc.User{}, ctx: ctx}).TableName())
r.Equal("context_table_useras", (&Model{Value: ac.User{}, ctx: ctx}).TableName())
r.Equal("context_table_userbs", (&Model{Value: bc.User{}, ctx: ctx}).TableName())
r.Equal("context_table_useras", (&Model{Value: []ac.User{}, ctx: ctx}).TableName())
r.Equal("context_table_userbs", (&Model{Value: []bc.User{}, ctx: ctx}).TableName())
}

func Test_TableName(t *testing.T) {
Expand All @@ -86,7 +86,7 @@ func Test_TableName(t *testing.T) {
func Test_TableNameContext(t *testing.T) {
r := require.New(t)

tn := "context table name"
tn := "context_table_names"
ctx := context.WithValue(context.Background(), "name", tn)

cases := []interface{}{
Expand Down
2 changes: 1 addition & 1 deletion testdata/models/ac/user.go
Expand Up @@ -5,5 +5,5 @@ import "context"
type User struct{}

func (u User) TableName(ctx context.Context) string {
return ctx.Value("name").(string) + "-usera"
return ctx.Value("name").(string) + "_useras"
}
2 changes: 1 addition & 1 deletion testdata/models/bc/user.go
Expand Up @@ -5,5 +5,5 @@ import "context"
type User struct{}

func (u User) TableName(ctx context.Context) string {
return ctx.Value("name").(string) + "-userb"
return ctx.Value("name").(string) + "_userbs"
}