fix: Fixed the condition to use the default value #409
Conversation
|
@kemokemo can you add tests? |
|
@jbuchbinder Thank you for your approval. @nelsam Thank you for your comment. I'll try it. Please give me some time. |
|
@nelsam Sorry for delay. Would you please confirm adding the test? |
|
I see it. Thanks! I'm going to pull it down locally and do some extra verification. Something is nagging at the back of my mind about the Yeah, I'll have to double check. I'm a bit busy today, but I'll set reminders so hopefully I will check it out today or tomorrow. |
|
This code is not correct. Insert plan depends on the first row inserted. If that row uses default value all inserts will have default value. Current code also uses reflection for each column even when default value is not set. |
|
@Pomyk Thank you so much for your review. I'll try to fix issues. 👍 |
table_bindings.go
Outdated
| val := elem.FieldByName(col.fieldName).Interface() | ||
| s2.WriteString( | ||
| fmt.Sprintf("case when %t or %s = %s then %s else %s end", | ||
| val == nil, t.dbmap.Dialect.BindVar(x), getZeroValueStringForSQL(val), col.DefaultValue, t.dbmap.Dialect.BindVar(x))) |
There was a problem hiding this comment.
Using result of val == nil here doesn't seem right. You will end up with a plan that has case when true or ... or case when false or ...
There was a problem hiding this comment.
Sorry. It's my wrong push. 🙇
I'm still working on the fix...
|
@Pomyk I would be grateful if you could comment. |
|
I'm not a fan of this feature in an orm. The code looks good but I didn't have time to test it yet. |
|
Why this is not merged? This worked on v1. This is an obvious degrade. |
This PR is for correcting #408 .
The conditions for using the default value specified in the tag have been modified as follows.
before
If there is a default value specified by the tag, always insert it.
this fix
Only if the specified value is nil or Zero value, if there is a default value specified by the tag, it is inserted.