Repository: go-gorm/gorm
Pull request #4042 (2021-02-02 22:45:09)

Replacer interface for more flexible NamingStrategy
<!--
Make sure these boxes checked before submitting your pull request.

For significant changes, please open an issue to make an agreement on an implementation design/plan first before starting it.
-->

- [x] Do only one thing
- [x] Non breaking API changes
- [x] Tested

### What did this pull request do?

This pull request makes `NamingStrategy` more flexible by allowing the user to provide a custom `Replacer` with an arbitrary implementation of `Replace()`. This change is fully backwards compatible: `strings.NewReplacer` continues to work as before. 

Additionally, there is a new field named `NoLowerCase` which, when set `true`, will cause the `NamingStrategy` to skip converting names to snake_case. This is also backwards compatible: the default, when unspecified, is to keep the existing behavior. Definitely open to suggestions for naming that field if you prefer something different!

### User Case Description

I am using GORM to interface with an existing sqlite database schema (Apple's Photos.app library) which uses naming conventions like `ZGENERICASSET` and `ZFILENAME` for its tables and fields. I would like to define my structs very simply like this, without requiring `gorm` tags to specify table or column names:

```go
type GenericAsset struct { // No need to implement TableName(), this will become "ZGENERICASSET"
    FileName string        // No need for a tag, this will become "ZFILENAME"
}
```

With this change, I can achieve the above by using a simple `NamingStrategy` and no need to re-implement the full `Namer` interface:

```go
type CustomReplacer struct{}
func (r CustomReplacer) Replace(name string) string {
    return "Z" + strings.ToUpper(name)
}
ns := schema.NamingStrategy{
    NameReplacer: CustomReplacer{},
    NoLowerCase: true,
}
...
```


jinzhu: Hello @joelnordell 

I think the globally `smap` should not exists or be part of the `NamingStrategy` struct, it will cause bug if your application using different `NamingStrategy`?

Could you make the change accordingly? 

Thank you.
joelnordell: Hi @jinzhu, yes I can make that change.
joelnordell: @jinzhu - I've implemented it. I ran into a couple of issues, though:

* The `NamingStrategy` methods are all received by value, not by pointer.
* The `sync.Map` cannot be copied by value, because it contains a `Mutex`.
* The `NamingStrategy` is directly instantiated, and does not use a factory function.

Given the above constraints, and wanting to not change the public API at all, I had to do something a bit strange:

* The `smap` is a pointer to a `sync.Map` which defaults to `nil`
* The places where `smap` is used will tolerate a `nil` pointer without a `panic`.
* There is a new method `Init()` on `NamingStrategy` which is _optional_ but will initialize the `smap` pointer.
* `gorm.Open` detects whether `schema.NamingStrategy` was given as the `Namer`, and if so, calls `Init()` on it in order to initialize the `smap`.

It is a little bit ugly for `gorm.Open` to cast the `Namer` like that in order to call `Init()` - but I couldn't think of any other way to move `smap` into the struct without making a breaking change to the public API. (Ideas I considered and rejected were: changing all NamingStrategy method receivers to be pointers, or requiring a factory function to initialize NamingStrategy.)

Your thoughts?
jinzhu: Change `NamingStrategy` to pointer? then we can use `sync.Map` for `smap`
joelnordell: > Change `NamingStrategy` to pointer? then we can use `sync.Map` for `smap`

I thought about that, but it would be a breaking API change.
jinzhu: > > Change `NamingStrategy` to pointer? then we can use `sync.Map` for `smap`
> 
> I thought about that, but it would be a breaking API change.

Yes, sounds not good, but sounds like a better choice then currently...

Or maybe we should just remove the name caches, I don't think this helped the performance much if any. As we already cached the model's schema, which has the table/column name already, so seems not necessary to have a second level caches.
joelnordell: Removing the name cache sounds like a good plan. I'll make that change. I could also do some benchmarking to see if it actually does make a difference or not.
joelnordell: @jinzhu - is this good to merge?

Patch:
diff --git a/schema/naming.go b/schema/naming.go
index f6d15f5..e10c921 100644
--- a/schema/naming.go
+++ b/schema/naming.go
@@ -4,7 +4,6 @@ import (
 	"crypto/sha1"
 	"fmt"
 	"strings"
-	"sync"
 	"unicode/utf8"
 
 	"github.com/jinzhu/inflection"
@@ -20,11 +19,17 @@ type Namer interface {
 	IndexName(table, column string) string
 }
 
+// Replacer replacer interface like strings.Replacer
+type Replacer interface {
+	Replace(name string) string
+}
+
 // NamingStrategy tables, columns naming strategy
 type NamingStrategy struct {
 	TablePrefix   string
 	SingularTable bool
-	NameReplacer  *strings.Replacer
+	NameReplacer  Replacer
+	NoLowerCase   bool
 }
 
 // TableName convert string to table name
@@ -42,7 +47,7 @@ func (ns NamingStrategy) ColumnName(table, column string) string {
 
 // JoinTableName convert string to join table name
 func (ns NamingStrategy) JoinTableName(str string) string {
-	if strings.ToLower(str) == str {
+	if !ns.NoLowerCase && strings.ToLower(str) == str {
 		return ns.TablePrefix + str
 	}
 
@@ -81,7 +86,6 @@ func (ns NamingStrategy) formatName(prefix, table, name string) string {
 }
 
 var (
-	smap sync.Map
 	// https://github.com/golang/lint/blob/master/lint.go#L770
 	commonInitialisms         = []string{"API", "ASCII", "CPU", "CSS", "DNS", "EOF", "GUID", "HTML", "HTTP", "HTTPS", "ID", "IP", "JSON", "LHS", "QPS", "RAM", "RHS", "RPC", "SLA", "SMTP", "SSH", "TLS", "TTL", "UID", "UI", "UUID", "URI", "URL", "UTF8", "VM", "XML", "XSRF", "XSS"}
 	commonInitialismsReplacer *strings.Replacer
@@ -98,14 +102,16 @@ func init() {
 func (ns NamingStrategy) toDBName(name string) string {
 	if name == "" {
 		return ""
-	} else if v, ok := smap.Load(name); ok {
-		return v.(string)
 	}
 
 	if ns.NameReplacer != nil {
 		name = ns.NameReplacer.Replace(name)
 	}
 
+	if ns.NoLowerCase {
+		return name
+	}
+
 	var (
 		value                          = commonInitialismsReplacer.Replace(name)
 		buf                            strings.Builder
@@ -143,6 +149,5 @@ func (ns NamingStrategy) toDBName(name string) string {
 		buf.WriteByte(value[len(value)-1])
 	}
 	ret := buf.String()
-	smap.Store(name, ret)
 	return ret
 }


Test patch:
diff --git a/schema/naming_test.go b/schema/naming_test.go
index b7a3216..08f8d49 100644
--- a/schema/naming_test.go
+++ b/schema/naming_test.go
@@ -72,3 +72,99 @@ func TestNamingStrategy(t *testing.T) {
 		t.Errorf("invalid column name generated, got %v", columdName)
 	}
 }
+
+type CustomReplacer struct {
+	f func(string) string
+}
+
+func (r CustomReplacer) Replace(name string) string {
+	return r.f(name)
+}
+
+func TestCustomReplacer(t *testing.T) {
+	var ns = NamingStrategy{
+		TablePrefix:   "public.",
+		SingularTable: true,
+		NameReplacer: CustomReplacer{
+			func(name string) string {
+				replaced := "REPLACED_" + strings.ToUpper(name)
+				return strings.NewReplacer("CID", "_Cid").Replace(replaced)
+			},
+		},
+		NoLowerCase: false,
+	}
+
+	idxName := ns.IndexName("public.table", "name")
+	if idxName != "idx_public_table_replaced_name" {
+		t.Errorf("invalid index name generated, got %v", idxName)
+	}
+
+	chkName := ns.CheckerName("public.table", "name")
+	if chkName != "chk_public_table_name" {
+		t.Errorf("invalid checker name generated, got %v", chkName)
+	}
+
+	joinTable := ns.JoinTableName("user_languages")
+	if joinTable != "public.user_languages" { // Seems like a bug in NamingStrategy to skip the Replacer when the name is lowercase here.
+		t.Errorf("invalid join table generated, got %v", joinTable)
+	}
+
+	joinTable2 := ns.JoinTableName("UserLanguage")
+	if joinTable2 != "public.replaced_userlanguage" {
+		t.Errorf("invalid join table generated, got %v", joinTable2)
+	}
+
+	tableName := ns.TableName("Company")
+	if tableName != "public.replaced_company" {
+		t.Errorf("invalid table name generated, got %v", tableName)
+	}
+
+	columdName := ns.ColumnName("", "NameCID")
+	if columdName != "replaced_name_cid" {
+		t.Errorf("invalid column name generated, got %v", columdName)
+	}
+}
+
+func TestCustomReplacerWithNoLowerCase(t *testing.T) {
+	var ns = NamingStrategy{
+		TablePrefix:   "public.",
+		SingularTable: true,
+		NameReplacer: CustomReplacer{
+			func(name string) string {
+				replaced := "REPLACED_" + strings.ToUpper(name)
+				return strings.NewReplacer("CID", "_Cid").Replace(replaced)
+			},
+		},
+		NoLowerCase: true,
+	}
+
+	idxName := ns.IndexName("public.table", "name")
+	if idxName != "idx_public_table_REPLACED_NAME" {
+		t.Errorf("invalid index name generated, got %v", idxName)
+	}
+
+	chkName := ns.CheckerName("public.table", "name")
+	if chkName != "chk_public_table_name" {
+		t.Errorf("invalid checker name generated, got %v", chkName)
+	}
+
+	joinTable := ns.JoinTableName("user_languages")
+	if joinTable != "public.REPLACED_USER_LANGUAGES" {
+		t.Errorf("invalid join table generated, got %v", joinTable)
+	}
+
+	joinTable2 := ns.JoinTableName("UserLanguage")
+	if joinTable2 != "public.REPLACED_USERLANGUAGE" {
+		t.Errorf("invalid join table generated, got %v", joinTable2)
+	}
+
+	tableName := ns.TableName("Company")
+	if tableName != "public.REPLACED_COMPANY" {
+		t.Errorf("invalid table name generated, got %v", tableName)
+	}
+
+	columdName := ns.ColumnName("", "NameCID")
+	if columdName != "REPLACED_NAME_Cid" {
+		t.Errorf("invalid column name generated, got %v", columdName)
+	}
+}