Replacer interface for more flexible NamingStrategy

[joelnordell]

• Do only one thing
• Non breaking API changes
• 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:

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:

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
• All the places where smap is used first check for a nil pointer.
• 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'll also check the benchmark tests to see if it actually does make a difference or not. (Update: running the benchmark tests before and after removing the name cache shows no difference.)

[joelnordell]

@jinzhu - is this good to merge?