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

feature request: patch validator #29

Closed
joshua-mcginnis opened this issue May 9, 2014 · 16 comments
Closed

feature request: patch validator #29

joshua-mcginnis opened this issue May 9, 2014 · 16 comments

Comments

@joshua-mcginnis
Copy link
Contributor

Thanks for this project.

A nice feature would be to validate that a patch document is a valid patch doc. This would allow users to pre-validate that the client sent a valid patch document before the server uses it.

Something like:
jsonpatch.isValid(patch); //true = valid, false = invalid

Or you could return errors, but that might be difficult. Ultimately, if a client sends a bad patch, we're just going to return an http bad request.

@joshua-mcginnis joshua-mcginnis changed the title Feature request: patch validator feature request: patch validator May 9, 2014
@jamesplease
Copy link
Contributor

@joshua-mcginnis, this shouldn't be too difficult to add. You should make up a PR if you've got the time :)

@joshua-mcginnis
Copy link
Contributor Author

@jmeas There's my initial crack at it.

@warpech
Copy link
Collaborator

warpech commented Jan 12, 2015

I have pushed to master my take on this, which is a generalised version of changes proposed by @joshua-mcginnis, @sonnyp and others.

Please share your thoughts. Below is the description of the new method (excerpt from README.md)


Validating a sequence of patches:

var obj = {user: {firstName: "Albert"}};
var patches = [{op: "replace", path: "/user/firstName", value: "Albert"}, {op: "replace", path: "/user/lastName", value: "Einstein"}];
var errors = jsonpatch.validate(patches, obj);
if (errors.length == 0) {
 //there are no errors!
}
else {
  for (var i=0; i<errors.length; i++) {
    if (!errors[i]) {
      console.log("Valid patch at index", i, patches[i]);
    }
    else {
      console.error("Invalid patch at index", i, errors[i], patches[i]);
    }
  }
}

jsonpatch.validate (patches Array, tree Object (optional)) : errors Array

Available in json-patch-duplex.js

Validates a sequence of operations. If tree parameter is provided, the sequence is additionally validated against the object tree.

If there are errors, returns an array with error codes located at the array index of the operation. If there are no errors, returns an empty array (length 0).

Possible errors:

Error code Description
SEQUENCE_NOT_AN_ARRAY Sequence of operations is not an array
OPERATION_NOT_AN_OBJECT Operation is not an object
OPERATION_OP_INVALID Operation op property is not one of operations defined in RFC-6902
OPERATION_PATH_INVALID Operation path property is not a string
OPERATION_FROM_REQUIRED Operation from property is not present (applicable in move and copy operations)
OPERATION_VALUE_REQUIRED Operation value property is not present (applicable in add, replace and test operations)
OPERATION_PATH_ALREADY_EXISTS Cannot perform an add operation at a path that already exists
OPERATION_PATH_UNRESOLVABLE Cannot perform a replace, remove, move or copy operation at a path that doesn't exist

@sonnyp
Copy link
Contributor

sonnyp commented Jan 12, 2015

@warpech cool.

I think the initial multiple option from my PR remains useful. User might not be interested in errors and might want break validation after the first error. Also having an options argument allow to extend the API later on.

I'd even go further and make tree a property of the options argument.
jsonpatch.validate (patch, {multiple: true, tree: document});

"Cannot perform an add operation at a path that already exists" actuall IIRC specs says that add will replace value if path exists.

Next time you ask for comments could you make a PR instead? Much easier to review. Also http://alblue.bandlem.com/2011/06/git-tip-of-week-rebasing.html 😉

@warpech
Copy link
Collaborator

warpech commented Jan 12, 2015

@sonnyp, you are right about the add operation:

If the target location specifies an object member that does exist,
that member's value is replaced.

I will fix that.

My motivation for removing the multiple option was to have only one behaviour that suits all needs. If you just want to know if there are no errors, check errors.length === 0. Otherwise you can iterate over the array to use the detailed information about what and where is the error. I think providing a wider range of options would make it both harder to learn (for users) and maintain (for me).

Thanks for mentioning git rebase, of course I know it but did not want to mess with the merge. My changes are in these commits:

  • 46162f4 (changes over your code)
  • 5513294 (validate the patch against an object tree)

@sonnyp
Copy link
Contributor

sonnyp commented Jan 12, 2015

@warpech the use case is to stop validation after first error, so removing multiple doesn't suit my needs.
What's wrong with returning an array of length 1 if multiple is set to false?

"both harder to learn " just a flag
"and maintain (for me)" how hard is it to maintainif (options.multiple === false) ... break; plus I wrote tests

@warpech
Copy link
Collaborator

warpech commented Jan 13, 2015

I appreciate your contribution so I want to reflect your needs. Could you describe your use case a little bit more?

For me, invalid patches are rare and fatal errors, so the efficiency of returning just the first error is irrelevant.

@sonnyp
Copy link
Contributor

sonnyp commented Jan 13, 2015

If you accept incoming patches from untrusted source it allow to prevent process from blocking if a large faulty patch is being validated, so on production you'd probably turn multiple/iterate/whatever to false.

@warpech
Copy link
Collaborator

warpech commented Jan 13, 2015

I start to get the point. So I guess you are using it on server? Is it in Redbooth?

I think we can:

  • add a stopOnFirstError boolean parameter
  • expose _validate method so you can overwrite it if you want to add custom validations (I find it useful for myself)

@sonnyp
Copy link
Contributor

sonnyp commented Jan 13, 2015

@warpech we don't currently use it at redbooth. Yes I use it server side.
Sounds good, thanks. Would you like me to do the PR?

@warpech
Copy link
Collaborator

warpech commented Jan 13, 2015

No worries, I will do it and let you do the testing :)

warpech added a commit that referenced this issue Jan 13, 2015
- add third parameter for `jsonpatch.validate` (stopOnFirstError)
- make `jsonpatch.validator` public, which allows to extend it with custom validations
(#29)
@warpech
Copy link
Collaborator

warpech commented Jan 13, 2015

@sonnyp Pushed. Would you take a look at the commit b64474b and share your thoughts?

warpech added a commit that referenced this issue Jan 29, 2015
@warpech
Copy link
Collaborator

warpech commented Jan 29, 2015

I have reworked the validation. The result is a faster and simpler API.

Summary of changes:

  • jsonpatch.validate returns and error object now
  • jsonpatch.apply with third parameter set to true throws an error if it encounters an invalid patch

This should please @sonnyp - only the first error is returned now.

See the changes at branch https://github.com/Starcounter-Jack/JSON-Patch/tree/issue-29-simplified
(last 2 commits)

The intention is to release it soon as version 0.6.0

@warpech
Copy link
Collaborator

warpech commented Feb 9, 2015

Merged to master

@warpech
Copy link
Collaborator

warpech commented Feb 16, 2015

Released in 0.5.1.

See https://github.com/Starcounter-Jack/JSON-Patch/blob/master/README.md for API and tests for examples

@warpech warpech closed this as completed Feb 16, 2015
@gdibble
Copy link

gdibble commented Apr 17, 2015

👍

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

No branches or pull requests

5 participants