feat: Add support for one-one mutation from sec. side #1247
Conversation
AndrewSisley
added
feature
Codecov Report
@@ Coverage Diff @@
## develop #1247 +/- ##
===========================================
+ Coverage 70.57% 70.66% +0.08%
===========================================
Files 182 182
Lines 17221 17251 +30
===========================================
+ Hits 12154 12190 +36
+ Misses 4132 4127 -5
+ Partials 935 934 -1
|
There was a problem hiding this comment.
question: Some diffs were a bit odd this was showing as added and then after a bit of scroll I realized it was just moved: "Note: This test should probably not pass, as it contains a reference to a document that doesnt exist." So just wanted to confirm this isn't the byproduct of this PR right?
Rest looks G
| return cid.Undef, err | ||
| } | ||
|
|
||
| // If this field was a secondary relation ID the related document will have been |
There was a problem hiding this comment.
praise: appreciate this comment
db/collection_update.go
Outdated
| return relationFieldDescription, valid && !relationFieldDescription.IsPrimaryRelation() | ||
| } | ||
|
|
||
| // patchPrimaryDoc patches the primary document linked to from the given dockey via the given secondary |
There was a problem hiding this comment.
todo: reword please, this reads weird "... primary document linked to from the given dockey via the given secondary .."
There was a problem hiding this comment.
It is a bit hard to understand at the moment.
There was a problem hiding this comment.
will try and reword
- reword doc
There was a problem hiding this comment.
I have reworded the documentation slightly, but I have been finding it difficult to articulate what this function does in a more concise way than the code itself.
| // Note: This test should probably not pass, as it contains a | ||
| // reference to a document that doesnt exist. It is doubly odd | ||
| // given that saving from the secondary side errors as expected |
There was a problem hiding this comment.
question: Is there an issue for this?
There was a problem hiding this comment.
Is conceptually part of the problem that we dont really do much/any data validation on write, I'm not sure their is an existing issue
| // NOTE: We delay the final Clean() call until we know | ||
| // the commit on the transaction is successful. If we didn't | ||
| // wait, and just did it here, then *if* the commit fails down | ||
| // the line, then we have no way to roll back the state | ||
| // side-effect on the document func called here. | ||
| txn.OnSuccess(func() { | ||
| doc.Clean() | ||
| }) | ||
|
|
There was a problem hiding this comment.
question: I wonder why this was in the loop before? I feel like it would have been a redundant additions to the OnSuccess functions.
There was a problem hiding this comment.
I feel like it would have been a redundant additions to the OnSuccess functions.
yes :P
I wonder why this was in the loop before?
I assume it was just a historical accident and replaced some logic that predated the OnSuccess funcs without moving it
db/collection_update.go
Outdated
| return relationFieldDescription, valid && !relationFieldDescription.IsPrimaryRelation() | ||
| } | ||
|
|
||
| // patchPrimaryDoc patches the primary document linked to from the given dockey via the given secondary |
There was a problem hiding this comment.
It is a bit hard to understand at the moment.
This was noted in the description, at a PR level github has manged the diff, wasn't a problem last time I checked the commit-diffs. The issue mentioned predates this PR, and has been accidentally solved from the secondary side which now requires the doc to exist. |
Was inverted, and returned false for primary relations and true for secondaries.
There is no point doing this for each field, and it risks bugs by calling it so late within the loop where it may be skipped over by 'continue' controls
Should make sure both work.
Note: This does not affect the collection.Update function, as that goes via `c.save`, and not `c.applyPatch`. That behaviour will be changed with either mutation-create, or a different ticket for those calls. It might be nice to investigate unifying these at somepoint, but not now. The two new functions isSecondaryIDField and patchPrimaryDoc will be used as-is when adding support for wrong-side save calls. They are broken into two functions due to the differences in `c.save` and `c.applyPatch`. It may be worth reviewing these two functions within the context of both commits.
This includes support for request create mutations, and collection.Save calls.
AndrewSisley
force-pushed
the
sisley/feat/I1213-one-one-create-2
branch
from
e206e53
to
8e0858f
Compare
* Remove commented out code * Remove commented out code * Fix IsPrimaryRelation Was inverted, and returned false for primary relations and true for secondaries. * Remove doc.Clean from loop There is no point doing this for each field, and it risks bugs by calling it so late within the loop where it may be skipped over by 'continue' controls * Expand right-side create test to query both directions Should make sure both work. * Add test for update mutation from primary side * Add support for update from sec. side Note: This does not affect the collection.Update function, as that goes via `c.save`, and not `c.applyPatch`. That behaviour will be changed with either mutation-create, or a different ticket for those calls. It might be nice to investigate unifying these at somepoint, but not now. The two new functions isSecondaryIDField and patchPrimaryDoc will be used as-is when adding support for wrong-side save calls. They are broken into two functions due to the differences in `c.save` and `c.applyPatch`. It may be worth reviewing these two functions within the context of both commits. * Add support for c.save-mutations from sec. side This includes support for request create mutations, and collection.Save calls.
…1247) * Remove commented out code * Remove commented out code * Fix IsPrimaryRelation Was inverted, and returned false for primary relations and true for secondaries. * Remove doc.Clean from loop There is no point doing this for each field, and it risks bugs by calling it so late within the loop where it may be skipped over by 'continue' controls * Expand right-side create test to query both directions Should make sure both work. * Add test for update mutation from primary side * Add support for update from sec. side Note: This does not affect the collection.Update function, as that goes via `c.save`, and not `c.applyPatch`. That behaviour will be changed with either mutation-create, or a different ticket for those calls. It might be nice to investigate unifying these at somepoint, but not now. The two new functions isSecondaryIDField and patchPrimaryDoc will be used as-is when adding support for wrong-side save calls. They are broken into two functions due to the differences in `c.save` and `c.applyPatch`. It may be worth reviewing these two functions within the context of both commits. * Add support for c.save-mutations from sec. side This includes support for request create mutations, and collection.Save calls.
Relevant issue(s)
Resolves #1213 #1214
Description
Adds support for one-one mutation (create/update requests,
clientAPI) from the secondary side of a one-one relationship.Suggest reviewing commit by commit, there is also additional context within the commit bodies that may be useful to reviewers. It also should make reading the tests easier as github has mangled the diff whilst reading them at a PR level.