-
Notifications
You must be signed in to change notification settings - Fork 162
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
Upgrade gORM to V2 #237
Upgrade gORM to V2 #237
Conversation
Please do not merge this PR until discussion is finished and explicit note mentions it. This PR might depend on new changes in toolkit |
@@ -23,12 +23,12 @@ message User { | |||
uint32 age = 5 [(gorm.field).drop = true]; // synthetic field | |||
uint32 num = 6; | |||
CreditCard credit_card = 7; // has one | |||
repeated Email emails = 8; // has many | |||
repeated Task tasks = 9 [(gorm.field).has_many = {position_field: "priority" foreignkey_tag: {not_null: true}}]; | |||
repeated Email emails = 8 [(gorm.field).has_many = {disable_association_autocreate: true disable_association_autoupdate: true preload: true}]; // has many |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the default preload setting changing to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it always was like this and had to be enabled like following: gORMv1
README.md
Outdated
@@ -135,7 +135,7 @@ Within the proto files, the following types are supported: | |||
level. | |||
- custom wrapper type `gorm.types.JSONValue`, which wraps a string in protobuf | |||
containing arbitrary JSON and converts to `postgres.Jsonb` GORM type | |||
(https://github.com/jinzhu/gorm/blob/master/dialects/postgres/postgres.go#L59) | |||
(https://gorm.io/gorm/blob/master/dialects/postgres/postgres.go#L59) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /dialects
directory doesn't exist in GormV2, I don't think
@@ -876,8 +879,8 @@ func (b *ORMBuilder) parseBasicFields(msg *protogen.Message, g *protogen.Generat | |||
fieldType = "*" + generateImport("Time", stdTimeImport, g) | |||
} else if rawType == protoTypeJSON { | |||
if b.dbEngine == ENGINE_POSTGRES { | |||
typePackage = gormpqImport | |||
fieldType = "*" + generateImport("Jsonb", gormpqImport, g) | |||
typePackage = gtypesImport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leverage the new JSON datatype from upstream, or is the custom one better (maybe simpler)? https://github.com/go-gorm/datatypes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally concerned over the fact, that we have a pointer to the struct. Thus I can assume, that some application code might rely on checking for nil
field value, while new JSON
datatype will hold "null"
in case scanned from DB and we can't easily distinguish empty slice value from nil
slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay, but I haven't tested any conversions to GormV2 personally
Is there an intent to land this change any time soon? It would be great to get gorm2 support. |
Any update? |
Upgrade gORM to V2
This PR introduces new version of
gORM
in accordance to release notes:association_autocreate
,association_autoupdate
,preload
tags from generated code, however replaces them with required code generation:association_autocreate
&association_autoupdate
are replaced withOmit(<field_name>
callspreload
is replaced withPreload(<field_name>)
callsassociation_save_reference
is removed as it is missing equivalent ingORMv2
Jsonb
fieldsNOTE: Currently this PR is dependent on toolkit:gorm_v2 branch, which is not master, merges should be performed with @Calebjh agreement/review