Patch with context (created with ADD_ORIGINAL_VALUE_ON_REPLACE) must fail on value mismatch

[Hubbitus]

You have useful com.flipkart.zjsonpatch.DiffFlags.ADD_ORIGINAL_VALUE_ON_REPLACE flag on patch creation. But such context fully ignored on apply time!

Please look at test:

	def "Test create patch with context, and fail on apply when original object changed"(){
		when:
			JsonNode source = toJson '{"k1":"v1","k2":"v2"}'
			JsonNode target = toJson '{"k1":"v1","k2": {"inner": 1}}'

			JsonNode patch = asJson(source, target, EnumSet.of(ADD_ORIGINAL_VALUE_ON_REPLACE))
		then:
			patch as String == '[{"op":"replace","fromValue":"v2","path":"/k2","value":{"inner":1}}]'

		when:
			source = toJson '{"k1":"v1","k2":"v3 changed"}' // Our external changes in source object (configuration in production by customer)
			JsonNode targetRecreated = apply(patch, source)

		then:
//			JsonPatchApplicationException e = thrown(JsonPatchApplicationException) // I want exception there, but it succeed!
			targetRecreated as String == '{"k1":"v1","k2":{"inner":1}}'
	}

Now it succeed, but I want mode when it failed with JsonPatchApplicationException and message like "Can't apply patch on path [/k2] because it have unexpected value [v3 changed]. Expected value tracked in patch [v2]".

I think there should be added corresponding value for that like com.flipkart.zjsonpatch.CompatibilityFlags#CHECK_ORIGINAL_VALUE.

Are you willing I provide PR for such fix?

[holograph]

com.flipkart.zjsonpatch.DiffFlags.ADD_ORIGINAL_VALUE_ON_REPLACE is there to aid in debugging, it's an extension to the RFC. I'm OK with adding a "check" flag (that should clearly indicate this is not standard), but IMO if what you need is data integrity, a much better approach would be to emit test nodes in the patch (instead of just the convenience value), which is part of the standard and would get you the behaviour your want.

[Hubbitus]

a much better approach would be to emit test nodes in the patch (instead of just the convenience value)

Sorry I do not understand that. Could you please provide example?
And yes, I could archive context for data integrity to ensure apply is not destructive. Some sort of optimistic locking. What I suggest implement is behaviour similar to GNU diff.

[holograph]

Try:

[
  {"op": "test", "path": "/k2", "value": "v2"},
  {"op":"replace", "path":"/k2", "value": {"inner": 1}}
]

Should work as expected. If this is what you mean, DiffFlags could be modified to include flavours of EMIT_TEST_OPERATIONS, which is similar to ADD_ORIGINAL_VALUE_ON_REPLACE but doesn't require custom extensions to the RFC. @vishwakarma what do you think? If it seems reasonable I can pick this up.

[holograph]

@Hubbitus I implemented this in #92, have a look if that works for you :-)

[Hubbitus]

It looks very well. Thank you very much! And if it is RFC then it indeed better approach then I suggest initially.

But I think it it have worth provide context on what path and value apply failed and extends tests on each type where such node may be present. Could you please look at comment https://github.com/flipkart-incubator/zjsonpatch/pull/92/files#r253562286

[holograph]

I'm working on that separately (see #94), but in the meantime I also realized we likely have defective generation of test ops for {"op":"test","path":"-",...} nodes. I'll add a test case and fix it soon.

[holograph]

@Hubbitus Take a look at the proposed changes in #94, I believe they'll make you happy :-) In the meanwhile, I believe this issue in general has been addressed...

[Hubbitus]

Thank you very much!