search

evanphx / json-patch

A Go library to apply RFC6902 patches and create and apply RFC7386 patches
BSD 3-Clause "New" or "Revised" License
1.06k stars 182 forks source link

Reason for skipping operations on empty documents? #155

Closed chmorgan closed 9 months ago

chmorgan commented 2 years ago

This one had me stuck for an hour or two or so so figured I'd raise it here.

v5 skips operating on documents of 0 length, there is a test for it as well to confirm that operations on "" result in "".

I reviewed https://datatracker.ietf.org/doc/html/rfc6902, and the docs for json-patch and it isn't clear why operations on an empty document wouldn't be considered valid.

I'd argue that "" is effectively "{}". In my use case I'm introducing a new database sub-field and using patches to alter it. As such it isn't present in the json object at all, so its effectively "" for existing rows.

Thoughts on treating "" as "{}"? Alternatively, thoughts on how to indicate dropping patches to the user? json-patch is silently dropping patches for op: "add" and "replace", which would seem to be valid in the initial case of an empty document.

lmk and I can see about pushing a PR up. Would like to save the next person a couple of hours of debug time.

kszafran commented 2 years ago

This is strange. I would have expected "" to cause an error, since it's not a valid JSON. I don't think "" should be treated as {}. That could also result in surprising behavior. In my opinion "" should return an error, and if someone wants to treat is as an empty JSON they can just add if input == "" { input = "{}" } before calling Apply.

But since there's a test for it, maybe there was a good reason to just ignore ""?

chmorgan commented 2 years ago

Yeah there is a test and inside of ApplyWithIndenting or something there is a if len(doc) == 0 check that returns early.

And to be clear by “” I mean an empty string, not a string with a single space in it.

On Tue, Jan 11, 2022 at 8:33 AM kszafran @.***> wrote:

This is strange. I would have expected "" to cause an error, since it's not a valid JSON. I don't think "" should be treated as {}. That could also result in surprising behavior. In my opinion "" should return an error, and if someone wants to treat is as an empty JSON they can just add if input == "" { input = "{}" } before calling Apply.

But since there's a test for it, maybe there was a good reason to just ignore ""?

— Reply to this email directly, view it on GitHub https://github.com/evanphx/json-patch/issues/155#issuecomment-1009969074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJH4ADDMC4AMHPLRWGAXZTUVQWUPANCNFSM5LVFVDWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

chmorgan commented 2 years ago

@evanphx thoughts on how to handle this? I can open a PR with whatever you think is a reasonable change (if you think a change should be made)

evanphx commented 9 months ago

I realize it's a bit ambiguous but I'm not interested in changing the behavior at this point because it will introduce a subtle deviation in behavior with no real upside.