Replace op should error out if path doesn't exist

[abhishekdwivedi3060]

Hi,

As per Json RFC, path must exist for the operation to be successful.
Refer this section from the RFC doc:

   The "replace" operation replaces the value at the target location
   with a new value.  The operation object MUST contain a "value" member
   whose content specifies the replacement value.

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

   For example:

   { "op": "replace", "path": "/a/b/c", "value": 42 }

   This operation is functionally identical to a "remove" operation for
   a value, followed immediately by an "add" operation at the same
   location with the replacement value.

But right now if the path doesn't exist, then json-patch doesn't error out and instead add the specified value at the given path.

Eg:

	original := []byte(`{"name": "John", "age": 24}`)
	patchJSON := []byte(`[
		{"op": "replace", "path": "/height", "value": 1.23}
	]`)
	patch, err := jsonpatch.DecodePatch(patchJSON)
	if err != nil {
		panic(err)
	}

	modified, pErr := patch.Apply(original)
	if pErr != nil {
		panic(pErr)
	}
	fmt.Print(modified)
}

output --> {"age":24,"height":1.23,"name":"John"}

Here /height path doesn't exist but replace operation instead of throwing error, adds height field in the json doc.

I'm not sure if this is the required behaviour or error is masked intentionally not to break things if path doesn't exist.

[abhishekdwivedi3060]

Looking at the json-patch code, it looks like there is an error check to catch above condition but error returned is always nil.

_, ok := con.get(key)
	if ok != nil {
		return errors.Wrapf(ErrMissing, "replace operation does not apply: doc is missing key: %s", path)
	}

and the get func is

func (d *partialDoc) get(key string) (*lazyNode, error) {
	return (*d)[key], nil
}

I think we can check the existence of key in the map first and accordingly return the error.
Let me know if this makes sense and I'll create a PR for the same.

[kszafran]

This has been reported a couple of times, e.g. #78 I'm myself using an older fork of this repository and replace does indeed fail if the field does not exist. IIRC this was reverted at some point, because kubernetes was relying on the incorrect behavior, but I can't find the relevant issue or PR now. I was hoping that maybe v5 has the correct replace behavior, but I'd need to check that (I want to update my fork at some point to the most recent version).

[kszafran]

I have checked this since my last comment and v5 behaves correctly.