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

Add validation for pre-flight checks #177

Closed
sambercovici opened this issue Jul 20, 2023 · 6 comments
Closed

Add validation for pre-flight checks #177

sambercovici opened this issue Jul 20, 2023 · 6 comments

Comments

@sambercovici
Copy link

Hi,

Currently, DecodePatch serialize the patch json.
IF the patch operation is not valid (ex: add without a value), it will lazy fail when applying the patch.
I suggest adding DecodePatchAndValidate or Validate that can be done after DecodePatch.
It will iterate on the Operations and validate the each one has a valid op and contains the fields needed for the op.

@sambercovici
Copy link
Author

@evanphx , If you agree that this may be done, we can implement this.

@evanphx
Copy link
Owner

evanphx commented Jul 23, 2023

Rather than have a new function, I'd rather that DecodePatch just did a validate pass always, rather than have folks use a separate validation.

@sambercovici
Copy link
Author

Makes perfect sense. We are working intently on this and will provide the fixe to DecodePatch ASAP.

@radwaretaltr
Copy link
Contributor

Rather than have a new function, I'd rather that DecodePatch just did a validate pass always, rather than have folks use a separate validation.

Hi @evanphx, thank you for the quick response.

I was looking into your suggestion and looks like it might have a side effect for users using "DecodePatch()" together with "ApplyWithOptions()". It seems "ApplyOptions" has a direct effect on the existing validations. For example, "ApplyOptions.AllowMissingPathOnRemove" dictates the validation flow of a "Remove" operation.
This means validations under "DecodePatch()" must consider "AllowMissingPathOnRemove" as well.

Wouldn't it be better to implement "Validate()" and "ValidateWithOptions()" methods for the "Patch" object returned by "DecodePatch()"? This should minimize this side effect and enable "pre-flight" validation before applying the patch.

What do you think?

@evanphx
Copy link
Owner

evanphx commented Jul 31, 2023

Hi @radwaretaltr,

I see your point, but I guess I'm wondering when would someone not want validation? Most folks would call Validate before hand, so why not do it by default. I understand that AllowMissingPathOnRemove will change the validations, but in a way that's exactly what that option is supposed to be doing.

@radwaretaltr
Copy link
Contributor

radwaretaltr commented Aug 3, 2023

Rather than have a new function, I'd rather that DecodePatch just did a validate pass always, rather than have folks use a separate validation.

Hi @evanphx, thank you for the quick response.

I was looking into your suggestion and looks like it might have a side effect for users using "DecodePatch()" together with "ApplyWithOptions()". It seems "ApplyOptions" has a direct effect on the existing validations. For example, "ApplyOptions.AllowMissingPathOnRemove" dictates the validation flow of a "Remove" operation. This means validations under "DecodePatch()" must consider "AllowMissingPathOnRemove" as well.

Wouldn't it be better to implement "Validate()" and "ValidateWithOptions()" methods for the "Patch" object returned by "DecodePatch()"? This should minimize this side effect and enable "pre-flight" validation before applying the patch.

What do you think?

Hi again @evanphx.

I raised a PR for this: #180
It seems that the "ApplyOptions" do not actually effect the validation behavior as they require the document for context:

  • SupportNegativeIndices: Looks like this option is used only when we know the numeric value in the path refers to an array element in the document (after resolving the element).
  • AccumulatedCopySizeLimit: Can only be done while applying the path on the document.
  • AllowMissingPathOnRemove and EnsurePathExistsOnAdd: Both refer to the existence of the specified path in the document.

Please let me know what you think or whether you have any comments.

Thank you.

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

3 participants