Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conform to RFC6902 replacement semantics #85

Merged
merged 4 commits into from
Dec 20, 2019
Merged

Conform to RFC6902 replacement semantics #85

merged 4 commits into from
Dec 20, 2019

Conversation

brettbuddin
Copy link
Contributor

@brettbuddin brettbuddin commented Aug 7, 2019

A "replace" patch operation referencing a key that does not exist in the target object are currently being accepted; ultimately becoming an "add" operation on the target object. This is incorrect per the specification:

From https://tools.ietf.org/html/rfc6902#section-4.3 section about
"replace":

The target location MUST exist for the operation to be successful.

This corrects the behavior by returning an error in this situation.

📎 #78

@evanphx
Copy link
Owner

evanphx commented Nov 3, 2019

Sorry for the delay.

Could you add some tests about how this changes the functionality of test and copy as well, which use get.

@brettbuddin
Copy link
Contributor Author

brettbuddin commented Nov 3, 2019 via email

A "replace" patch operation referencing a key that does not exist in the
target object are currently being accepted; ultimately becoming "add"
operations on the target object. This is incorrect per the
specification:

From https://tools.ietf.org/html/rfc6902#section-4.3 section about
"replace":

    The target location MUST exist for the operation to be successful.

This corrects the behavior by returning an error in this situation.
@brettbuddin
Copy link
Contributor Author

Alrighty. I've added some additional tests. For the most part, the tests you have are adequate to test for any change in behavior for these other verbs. The only one that is going to change seems to be copying from a non-existent key. That's okay though, because the spec specifies a similar MUST exist condition for from for copy.

@brettbuddin
Copy link
Contributor Author

Any thoughts about this?

@logicalhan
Copy link
Contributor

Hey @evanphx, is there any way we can talk you into cutting a 4.5.1 patch release with just the fix from #87? It turns out that it is going to be non-trivial for Kubernetes to upgrade due to this particular change..

@logicalhan
Copy link
Contributor

Scratch that last comment, we've figured out a way of bridging the fix without requiring a patch release :)

@evanphx
Copy link
Owner

evanphx commented Jun 8, 2020

@logicalhan I could do a 4.5.1 if that makes it easier.

@logicalhan
Copy link
Contributor

@logicalhan I could do a 4.5.1 if that makes it easier.

:) thanks, but I think we can actually use the git sha, it seems that the exponential fix went in prior to the RFC change thing, so we can directly peg against that. @liggitt to verify though.

@evanphx
Copy link
Owner

evanphx commented Jun 9, 2020 via email

@logicalhan
Copy link
Contributor

Yeah, I think we've decided to use the sha that includes the exponential fix but not the RFC one, so it won't be necessary. Really appreciate it though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants