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

A bit different approach to external validators plus validation against definition in a schema #53

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

arentrue
Copy link

Hi, I'd propose a bit different approach to external validators: allow pluggable validator modules which follow validator interface, the same as used for draft3 and draft4 validators, and can be built on top of those default validators as well as override any rule or implement totally independent validation logic. I think, that is rather more flexible approach, than just allowing some post validation after all the default rules have been successfully validated. It will also allow to update jesse_validator_draft4 code, removing unnecessary code duplication with jesse_validator_draft3.

This PR also introduces jesse:validate_definition/3/4 interface to validate a value against a particular definition in the definitions section of a schema.

I see, that you've just merged alternative implementation for external validators (I should have created this PR some weeks ago!), thou I hope you'll consider this PR. If you find this PR interesting and I'll prepare it for the merge (rebase, etc).

@@ -150,6 +152,44 @@ validate(Schema, Data, Options) ->
throw:Error -> {error, Error}
end.

%% @doc Equivalent to {@link validate_definition/4} where `Options' is an empty list.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Elvis:

Line 155 is too long: %% @doc Equivalent to {@link validate_definition/4} where `Options' is an empty list..

@@ -45,6 +62,21 @@ validate(JsonSchema, Value, Options) ->
NewState = validate_with_state(JsonSchema, Value, State),
{result(NewState), Value}.

%% @doc Validates json `Data' against `Definition' in `JsonSchema' with `Options'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Elvis:

Line 65 is too long: %% @doc Validates json Data' against Definition' in JsonSchema' with Options'..

init_validator_state(undefined, _) ->
undefined;
init_validator_state(Validator, Opts) ->
Validator:init_state(Opts).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Elvis:

Remove the dynamic function call on line 287. Only modules that define callbacks should make dynamic calls.

@andreineculau
Copy link
Member

thanks for finally opening a PR ! :)

it will take a while to look through it, but I've always wanted to do this but time never knocked on its door.

as for the functionality to validate against a schema under definitions - I don't see why it cannot/shouldn't be generic i.e. validate JSON instance against the nested schema available at this JSON pointer (basically revert one of your commits and go back to validate_ref or similar).

If you have the time, I suggest this:

  • split custom_validator and validate_ref changes into 2 PRs (I haven't paid attention if one is dependant on the other; ignore the split request if so)
  • if you agree about the request to go back to validate_ref, do so
  • squash your commits (it will make next steps easier)
  • rebase your work on the no-external-validator branch (and open PRs towards that branch instead of master) where I've reverted @lazedo 's validator just so that we have a clean diff to review

ping @lazedo : feedback is most welcome!

@arentrue
Copy link
Author

@andreineculau sure, I'll split the PR into two and follow the rest of the proposed points.

A note regarding the validate_definition vs validate_ref, though. Current jesse_database implementation doesn't seem reliable for production use:

  1. That's just a public named ets table, owned by a process first created it. That could lead to unexpected unpleasant effects in runtime, when the owner process crashes. At least I'd expect it to be embedded into a gen_server with a child_spec interface, provided for the library users.
  2. Even with the gen_server implemetation jesse_database is still a single point of failure, which is to be avoided when possible.

Since in the application where I use jesse, it is possible to reliably keep the schema (essentially a jesse state) by the application, I've changed jesse API from validate_ref (definition validation analogue to jesse:validate which uses a schema in jesse_database) to validate_definition (definition validation analogue to jesse:validate_with_schema providing the schema to validate against as a function parameter).

Suggestions for a better approach to implementing that API for validation against a subschema available at particular JSON pointer, are very welcome!

@lazedo
Copy link
Contributor

lazedo commented May 30, 2017

@arentrue i think the initial external validators may not be fully explained.

we have draft3/draft4 schemas that validate data, and.., for business extensions (validate a customer in a database, ....) we use the extension validator by using the callback. we look for annotated schema elements and perform whatever we want as an extra validation.
we still want to use v3/v4 validators for types, ranges, whatever. we are only interested in providing an extra layer of validation not tied to jesse core library.

@andreineculau andreineculau added this to the next major milestone May 17, 2018
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

Successfully merging this pull request may close these issues.

None yet

9 participants