Conversation
mergePatch returns nil only when the target is null, and null is itself the merge patch (RFC 7386 Appendix A, wI2L#11). The guard read it as an absence of differences and returned no patch, so a caller applying the result kept the source document.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MergePatchandMergePatchJSONreturn no patch at all when the target is JSONnull, whereRFC 7386 says the patch is
null.The guard reads as "there were no differences", but
mergePatchnever returns nil for that.Two equal objects give
{}, equal scalars give the scalar.merge.go:49is its only nilsource:
So
patch == nilholds in exactly one case - the target isnull- which is the one case theguard must not swallow. On
master:An empty patch means "apply nothing", so a caller round-tripping
{"a":"foo"}through the patchkeeps
{"a":"foo"}where the RFC requiresnull.This is already asserted by the repo's own test data -
testdata/tests/mergepatch/rfc.jsoncase#11,{"a":"foo"} -> null => null, straight from RFC 7386 Appendix A. It passes becausemerge_test.go:36calls the unexportedmergePatchdirectly and never goes through the exportedwrappers. The public surface of merge-patch generation is covered only by the two happy-path
examples in
example_test.go.The change
Drop the guard from both wrappers -
json.Marshal(nil)already emitsnull.Observable change, stated plainly: for a null target these now return
[]byte("null")andnilerror, where before they returnednil, nil. Anything treating an empty result as "nochange" will see 4 bytes instead. That is the point of the fix, but it is a behaviour change on
that one input. Every other input is byte-identical.
Test
Test_mergePatchNullTargetinmerge_test.go, drivingMergePatchJSONrather than theunexported function, with RFC
#12alongside as a control. Reverting onlymerge.go:rfc #12passes either way, confirming the non-null path is untouched.CI's own command passes with the change:
go vetandgofmt -lclean.Disclosure: found and prepared with AI assistance (Claude). Every figure above is from a run on
this branch.