Add additional validation to the input patch during parsing

[swaranga]

Adding some additional validation during parsing of an input JSON patch. Also added tests to validate the different test cases.

[swaranga]

Ping

[vishwakarma]

Apologies, I am traveling and will be back home in next 2 days. I will look into it soon.

On Sat, Sep 10, 2022, 7:24 AM Swaranga Sarma @.***> wrote:

Ping

— Reply to this email directly, view it on GitHub https://github.com/flipkart-incubator/zjsonpatch/pull/145#issuecomment-1242644006, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPZRKOSFMV2XUNYAGHVRXTV5QSSFANCNFSM6AAAAAAQCCGYCU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[vishwakarma]

Thanks @swaranga What are the thoughts behind adding these validations?

[swaranga]

What are the thoughts behind adding these validations?

Did not understand this comment? Could you please clarify? Do you mean I should add more detailed motivations on why these additional validations should be added to the library?

[vishwakarma]

What are the thoughts behind adding these validations?

Did not understand this comment? Could you please clarify? Do you mean I should add more detailed motivations on why these additional validations should be added to the library?

Apologies for not being clear.

I meant to say what were the motivations behind these changes.

[swaranga]

Well, the general motivation for adding these patches is that each of these patches are invalid patches. And it is better to reject them and fail fast rather and some undefined error later.

For instance, consider the patch from the test [{"path": "/field1", "value": {"id":123}}] - this patch is invalid because it does not have the "op" attribute which is a required field for a Json patch. Similarly a patch like {"op": "test", "path": 1, "value": {"id":123}} is invalid because the "path" attribute must be a string but in this cane it is a number. Another patch like {"op": "replace", "path": "/field1/a"} is invalid because for a "replace" operation you need the "value" to replace the old value without which the patch is meaningless. Each patch in the test cases have comments on why they are invalid.

I can talk a little bit about our use case and why I think this validation is needed. I have a service where callers can "register" Json patches in the system and later they can "apply" the patch for some Json configuration.

Now from a service owner perspective, I would like to fail the initial "register" operation if the Json patch that the caller is trying to add to the system is invalid. It is better to fail fast at this point rather than at the point when the "apply" API is called. This is because failing at the "register" call allows the caller to fix the patch right then instead of discovering the error later at which point it might be difficult to go back and fix the issue.

In terms of implementation, I can certainly add these validation in my own application code before returning success to the caller but the zjsonpatch library has a nice validate method which seems like the right place to host these instead of the application code.

[swaranga]

Also please note that most of these are already handled by the existing code. Following patches are already considered invalid by the current implementation of the api:

  // no operation
  [{"path": "/field1", "value": {"id":123}}],

  // TEST operation with no path
  [{"op": "test", "value": {"id":123}}],

  // TEST operation with no value
  [{"op": "test", "path": "/field1"}],

  // ADD operation with no 'path' attribute
  [{"op": "add", "value": "b"}],

  // ADD operation with no 'value' attribute
  [{"op": "add", "path": "/field1/a"}],

  // REPLACE operation patch with no 'value' attribute
  [{"op": "replace", "path": "/field1/a"}],

  // MOVE operation with no 'from' attribute
  [{"op": "move", "path": "/field1/a"}],

  // COPY operation with no 'from' attribute
  [{"op": "move", "path": "/field1/a"}],

The following invalid patches were not detected which is being fixed in this PR:

  // TEST operation with non-text 'path' attribute
  [{"op": "test", "path": 1, "value": {"id":123}}],

  // ADD operation with non-text 'path' attribute
  [{"op": "add", "path": 123, "value": "b"}],

  // MOVE operation with no non-text 'from' attribute
  [{"op": "move", "path": "/field1/a", "from": 123}],

  // COPY operation with no non-text 'from' attribute
  [{"op": "copy", "path": "/field1/a", "from": 123}]

I added tests for all cases since I thought that would help future refactoring.

[vishwakarma]

Thanks @swaranga