STITCH-1970 Optimize UPDATE and REPLACE to be $set and $unset #72
Conversation
…into STITCH-1970
…into STITCH-1970
| fun testUpdateDescriptionDiff() { | ||
| val originalJson = """ | ||
| { | ||
| "shop_name": "nkd pizza", |
There was a problem hiding this comment.
where did you get this from haha
There was a problem hiding this comment.
It's our local pizza shop in Dublin– I'll change it to not use real names 🍕
| } | ||
|
|
||
| @Test | ||
| fun testUpdateDescriptionToUpdateDoc() { |
There was a problem hiding this comment.
[request] Can you make a more involved test that hits more than just 1 field per updated and removed? I would add some nesting as well. It's nice to have a test where we have a literal string saying exactly what the diff should be.
| return newDocument | ||
| } | ||
|
|
||
| fun testUpdatedDocumentMatchesExpectation( |
There was a problem hiding this comment.
Seems like you can pull this out to a function outside of here so it can be reused. It would make it easier to see what this test is doing if it was just the tests were performing as succinctly as possible.
| collection.aggregate( | ||
| listOf( | ||
| BsonDocument( | ||
| "\$project", |
There was a problem hiding this comment.
[q] Why is this projecting instead of just doing a find?
There was a problem hiding this comment.
The projection was to eliminate the id fields in the subdocuments.
| expectedDocumentAfterUpdate: BsonDocument | ||
| ) { | ||
| // create an update description via diff'ing the two documents. | ||
| val updateDescription = diff(withoutId(originalDocument), withoutId(expectedDocumentAfterUpdate)) |
There was a problem hiding this comment.
[test] It would be better to write an actual update and not use expectedDocumentAfterUpdate. That will prove that we can update something with whatever operators and still produce an update description.
| event = changeEventForLocalUpdate( | ||
| namespace, | ||
| documentId, | ||
| ChangeEvent.UpdateDescription.diff(remoteEvent.getFullDocument(), document), |
There was a problem hiding this comment.
[q] I think we can still use documentAfterUpdate here?
There was a problem hiding this comment.
Ah yes, we should be able to.
|
|
||
| if (beforeResult != null) { | ||
| emitEvent(documentId, | ||
| changeEventForLocalUpdate( |
There was a problem hiding this comment.
[minor] When we replace from remote, it is really a replacement so you should be able to keep this code the same.
| /** | ||
| * Find the diff between two documents. | ||
| * | ||
| * NOTE: This does not do a full diff on [BsonArray]. If there is |
There was a problem hiding this comment.
[q] Why [] for [BsonArray]?
There was a problem hiding this comment.
I copied this over from the kotlin PoC code. Will replace with {@link BsonArray}
| * @return a description of the updated fields and removed keys between | ||
| * the documents | ||
| */ | ||
| private static UpdateDescription diff(final BsonDocument thisDocument, |
There was a problem hiding this comment.
[test] I would make several more unit tests for this for various edge cases. Empty doc vs non empty doc, rray vs non array field, Deeply nested docs, etc.
There was a problem hiding this comment.
These were covered in the main unit test, but I should break it up into multiple tests.
There was a problem hiding this comment.
Broken up in the same test, but with numbering.
| * @return a description of the updated fields and removed keys between | ||
| * the documents | ||
| */ | ||
| private static UpdateDescription diff(final BsonDocument thisDocument, |
There was a problem hiding this comment.
Instead of this and that, how about before and after?
There was a problem hiding this comment.
Could do– in the PoC, it was more general use case diffing. Since this is attached to UpdateDesc, I agree.
| // because they require more permissive Stitch rules. | ||
| final BsonDocument update, | ||
| final UpdateDescription update, | ||
| final BsonDocument document, |
There was a problem hiding this comment.
[nit/not your code] this argument should be renamed to fullDocumentAfterUpdate to make it clear that's what it is
| } else { | ||
| // ii. Otherwise, replace the local document with the resolved document locally, mark that | ||
| // there are pending writes for this document, and emit a REPLACE change event. | ||
| // there are pending writes for this document, and emit an UPDATE change event, or a |
There was a problem hiding this comment.
[nit] any changes you make to the comments with the the lettering/numbering should also be made to the spec document. we should try to keep the spec in sync with what the code is actually doing
| logicalT, | ||
| atVersion, | ||
| event); | ||
| event = changeEventForLocalUpdate( |
There was a problem hiding this comment.
[nit] I guess the proper name for this method would now be updateOrUpsertOneFromResolution
| ctx.namespace, | ||
| ctx.testDocumentId, | ||
| ctx.updateDocument, | ||
| ChangeEvent.UpdateDescription(BsonDocument("count", BsonInt32(2)), listOf()), |
There was a problem hiding this comment.
[nit] you reuse this exact update in a lot of places so maybe just set a testUpdateDescription constant?
There was a problem hiding this comment.
Could, though for the purpose of test code, I think it's better to write in line. Though it's reused, it's fairly arbitrary, and throughout the test, the count can increase.
| )) | ||
| ctx.verifyConflictHandlerCalledForActiveDoc(1, expectedLocalEvent, | ||
| ChangeEvent.changeEventForLocalUpdate(ctx.namespace, ctx.testDocumentId, ctx.updateDocument, ctx.testDocument, false)) | ||
| ChangeEvent.changeEventForLocalUpdate(ctx.namespace, ctx.testDocumentId, null, ctx.testDocument, false)) |
There was a problem hiding this comment.
[q] what does a change event for a local update with a null UpdateDescription mean? is it a noop update? seems a little odd that this is something our code is producing
There was a problem hiding this comment.
ah I see now that this is produced by our mock, but I think we should just have the mock produce a real update, because a local update change event with no actual things being updated seems like something that shouldn't actually exist
There was a problem hiding this comment.
Actually I think this is due to coalescence. I can add a note here that that is what this is testing.
| ChangeEvent.changeEventForLocalDelete(ctx.namespace, ctx.testDocumentId, true), | ||
| ChangeEvent.changeEventForLocalUpdate( | ||
| ctx.namespace, ctx.testDocumentId, ctx.updateDocument, ctx.testDocument, false | ||
| ctx.namespace, ctx.testDocumentId, null, ctx.testDocument, false |
| override fun queueConsumableRemoteUpdateEvent() { | ||
| `when`(dataSynchronizer.getEventsForNamespace(any())).thenReturn( | ||
| mapOf(testDocument to ChangeEvent.changeEventForLocalUpdate(namespace, testDocumentId, updateDocument, testDocument, false)), | ||
| mapOf(testDocument to ChangeEvent.changeEventForLocalUpdate(namespace, testDocumentId, null, testDocument, false)), |
See https://docs.google.com/document/d/1lvd02JFNI9Aa7Zely3xMJEct_N-HMn84tKIIhMq7Zes/ for more detailed information on the agreed design.