Added support for non-serial pField mutation #39
Conversation
Yet this implememnation gidoes not protect from accidental mutation of pk
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 31 31
Lines 883 884 +1
=========================================
+ Hits 883 884 +1
Continue to review full report at Codecov.
|
Removed unnecessary break
changeset.go
Outdated
| @@ -87,9 +97,14 @@ func (c *Changeset) Apply(doc *rel.Document, mut *rel.Mutation) { | |||
| c.applyAssocMany(doc, field, mut, v) | |||
| } | |||
| default: | |||
| if field != pField && scannable(c.types[field]) { | |||
| if pField != field && scannable(c.types[field]) { //if not PK - try to set | |||
There was a problem hiding this comment.
only change to this is enough?
if (pField != field || pField == field && !c.ignorePrimary) && scannable(c.types[field]) {There was a problem hiding this comment.
and after that we maybe we don't need mutablePField?
There was a problem hiding this comment.
Sounds reasonable, however, mutable serial and bigserial PK does not make sense. Hence, the mutable PK is here to ensure that PK is something other than that.
There was a problem hiding this comment.
mutable serial and bigserial PK does not make sense
sorry I don't understand this part
if what you mean is to allow non autogenerated id only for field other than integer (or bigint), then you won't be able to accommodate user that use library like snowflake (go version: https://github.com/sony/sonyflake)
so it's perfectly fine to accept any type as primary, no need to think about whether it's autogenerated or not
There was a problem hiding this comment.
Makes sense. Actually, what do think about this change in general? Maybe, it worth to just enforce users to set the PK using explicit rel.Set than to introduce such complicated logic? Or suggested changes are valid from perspective of library's idea?
There was a problem hiding this comment.
I think the suggested change is not complicated at all 🤔 :
- add
ignorePrimaryFieldtoChangesetstruct - set
ignorePrimaryFieldto true whenConvertis used - Update
Applyto useif (pField != field || pField == field && !c.ignorePrimary) && scannable(c.types[field]) {condition
There was a problem hiding this comment.
Ok then. In general it would be nice to revisit the changeset logic, because it does not allow for composite PK, and maybe some rules about it's mutation should be introduced - again I think that PK can be inserted, but not updated.
There was a problem hiding this comment.
not sure why composite pk is not allowed, but feel free to open other issue if it still happening after this pr merged
changeset.go
Outdated
| @@ -87,9 +97,14 @@ func (c *Changeset) Apply(doc *rel.Document, mut *rel.Mutation) { | |||
| c.applyAssocMany(doc, field, mut, v) | |||
| } | |||
| default: | |||
| if field != pField && scannable(c.types[field]) { | |||
| if pField != field && scannable(c.types[field]) { //if not PK - try to set | |||
There was a problem hiding this comment.
and after that we maybe we don't need mutablePField?
|
I wonder if we can deduce the primary key in the Suppose we have a PK, which can be muted. That induces that it is not SERIAL or BIGSERIAL - maybe UUID or a natural PK. Then it would only be logical to set it during the INSERT statement. This assumption makes the PK immutable in general sense - it can be set once and never be updated. So this is kind of behaviour I expect to resolve the #38. |
changeset_test.go
Outdated
| @@ -206,3 +215,110 @@ func TestChangesetApply_constraint(t *testing.T) { | |||
| UpdatedAt: now, | |||
| }, user) | |||
| } | |||
|
|
|||
| func TestChangesetApply_MutablePK(t *testing.T) { | |||
There was a problem hiding this comment.
for the name case, the convention in this repo is to start with lower case
| func TestChangesetApply_MutablePK(t *testing.T) { | |
| func TestChangesetApply_mutablePK(t *testing.T) { |
maybe mutablePK can be changed as well since we are not using that variable anymore
There was a problem hiding this comment.
Tests are updated
changeset.go
Outdated
| @@ -87,7 +88,7 @@ func (c *Changeset) Apply(doc *rel.Document, mut *rel.Mutation) { | |||
| c.applyAssocMany(doc, field, mut, v) | |||
| } | |||
| default: | |||
| if field != pField && scannable(c.types[field]) { | |||
| if (pField != field || pField == field && !c.ignorePrimary) && scannable(c.types[field]) { //if not PK - try to set | |||
There was a problem hiding this comment.
maybe comment can be removed or updated?
There was a problem hiding this comment.
Comment is removed
Yet this implementation does not protect from accidental mutation of pk, it assumes the possibility of explicit, non-serial PK mutation in the changeset.
Probably this mutation should only occur during the insertion, however, the
Mutatorinterface does not specify the action, unlike in Ecto, and I did not find a way to deduce the action fromApply method.Closes #38