-
Notifications
You must be signed in to change notification settings - Fork 66
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
Breaking changes for auto increment/pk fields #19
Comments
Thank you for your feedback. Actually, I was aware this potential problem will be occurred. Therefore I set the Release Tag, previous version is v1.0.0 and this time is v1.1.0. (Sorry, there is not enough announcement) Let me explain why I made this modification. In previous code, there was a problem that insertion was not possible unless the primary key is auto_increment. Because when the column is primary_key, the return value of There is no problem with auto_increment columns. This is because the database itself will give a number. But what about natural keys that aren't surrogate keys? if it's a natural key, it will fail to insert because this will not be the target of insertion for the reason of primary_key, even though the value has been set by manually. After all, in my opinion, only way to make both natural key and surrogate key compatible is to set the But if there are other good ideas, I'd like to adopt them. I'd be glad to know that there was such a context for this change. |
Thank you for your response! Regarding the new release tag I did a similar mistake myself recently which made me re-visit semver.org. If you're doing any changes that's not in a backwards compatible manner (which this isn't) you should probably bump your major version and release I'm currently not really too familiar with the gorm source code but it's obvious that there are underlying mechanics to figure out which field(s) are primary and that seems to be based on the name Regarding the issue you had, see my proposed solution. That is, not only look for primary key but also ensure the name of the field is I'm not sure I fully understand your thoughts around surrogate keys. If I would need a surrogate key I would specify it manually, definitely not name it type MyType struct {
gorm.Model // Gives me PRIMARY KEY id
Value string
CustomSurrogate int `gorm:"AUTO_INCREMENT"`
PrimaryValue string `gorm:"PRIMARY_KEY"`
} With your current version this gives me a duplicate key error, with the proposed diff this works as intended and allocates an ID both for my ID field (from MySQL only supports one primary key but PostgreSQL supports multiple so I'm using multiple primary keys in my example to show that you can set a value to With the change you made, what's your proposed solution for people using In my opinion I think it's of importance to support embedding |
Thank you for your ideas about the release version and auto_increment. I agree with all of these. I think this library should follow the gorm ecosystem, and respect the default behavior. In that viewpoint, my perception was not enough. I don't have enough time today because of my private situation, so I will think about it carefully and reply tomorrow. I'm grateful for your continuous effort. |
I confirmed the gorm behavior again and I came up with one idea for this issue. To explain why it was introduced, I'd like to summarize the point of issues.
|
I think it looks good! The only difference here is that you'll also leave out blank primary keys even if it's not the id field. I think this is OK because a blank primary key would be set to it's default value (e.g. This might not be the perfect solution but on the other hand I don't see any use case where you would use this package and omit your primary keys. At least I wouldn't expect that to work. |
Thank you for confirmation. (Sorry for the late reply🙏) As you say, there is an issue that primary_key it's not the ID field is also excluded, but such a case should not be assumed originally. I will implement this fix today. If you don't mind, would you please review at that time? |
Sounds great, just tag me when ready and I'll review your implementation! |
Closing this, solved in 8a8235c. Thanks! |
The commit eab1a9e introduces breaking changes.
Click to see diff for the commit
If you use the default
gorm.Model
for your ID/primary key the field won't have the tagAUTO_INCREMENT
. Since the field is missing the tagfieldIsAutoIncrement
will always returnfalse
and thus not be ignored. This will result in a duplicate key error. The field does however have theprimary_key
tag:I know that a field doesn't auto increment by default just because it's a primary key but even if the table is created with gorm migrates or not I think it's common to use
gorm.Model
for your ID field and want it to be auto incremented.Code to reproduce
Proposed fix
The text was updated successfully, but these errors were encountered: