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

first version of defaults injection #1

Open
wants to merge 14 commits into
base: master
from
Open

first version of defaults injection #1

wants to merge 14 commits into from

Conversation

@rkuhn
Copy link
Member

rkuhn commented Feb 27, 2020

No description provided.

Copy link
Member Author

rkuhn left a comment

@wngr I have self-reviewed, should be good to go. See some default_for_schema test for an example of conjuring a whole instance from thin air. (real code should then validate that instance)

@rkuhn

This comment has been minimized.

Copy link
Member Author

rkuhn commented Feb 28, 2020

It would also be great if you could give it a spin and compare the performance to version 3.2.0, with and without making use of defaults in a reasonably large example schema. And not that I’m overly worried, but this is some complex stuff, so it might be warranted to let it keep crunching schemas and validations for a few hours while observing memory usage.

Copy link

wngr left a comment

First round of reviews. Still wrapping my head around this ..

src/json_schema/keywords/items.rs Show resolved Hide resolved
@@ -37,10 +40,10 @@ impl fmt::Debug for dyn Keyword + 'static {
macro_rules! keyword_key_exists {

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

I'm confused by this macro. Looking at the usage, maybe_val is &serde_json::Value. How can this work, given that if and else have incompatible types?

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 1, 2020

Author Member

The trick is that the else branch does not yield a value but returns from the enclosing function — these macros are purely lexical, typechecking happens only after the program has been modified.

@@ -8,7 +8,7 @@ fn visit_specs<F>(dir: &path::Path, cb: F)
where
F: Fn(&path::Path, Value) + Copy,
{
let contents = fs::read_dir(dir).ok().unwrap();
let contents = fs::read_dir(dir).expect(&*format!("cannot list directory {:?}", dir));

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

very helpful for newcomers indeed 🎩

src/json_schema/schema.rs Outdated Show resolved Hide resolved
schema.add_defaults_recursive(top, id, scope);
}

// step 2: use explicit default if present

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

why not do this before step 1? If you have a default, you don't need to go down into tree.

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 1, 2020

Author Member

The point of the traversal is not to get a default for this node, it is to mutate the whole subtree to get defaults, too.

src/json_schema/schema.rs Show resolved Hide resolved
@@ -222,6 +325,8 @@ impl Schema {
if let Some(validator) = keyword.keyword.compile(def, context)? {
if is_exclusive_keyword {
validators = vec![validator];
} else if keyword.keyword.place_first() {
validators.splice(0..0, std::iter::once(validator));

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

What's this for? This will remove an existing validator.

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 1, 2020

Author Member

Nope, it replaces the empty range at the beginning of the vector, i.e. pushes to the front.


// necessary due to object being mutated in the loop
let keys = object.keys().cloned().collect::<Vec<_>>();
'main: for key in keys.iter() {

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

does that need to be named? you don't seem to break anyway

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 1, 2020

Author Member

I left it in because it clarifies the continue that appears many lines down.

for schema in schemas.iter() {
let mut result = schema.validate_in(&val, path);
if result.is_valid() && result.replacement.is_some() {
*val.to_mut() = result.replacement.take().unwrap();

This comment has been minimized.

Copy link
@wngr

wngr Feb 28, 2020

Shouldn't all possible replacements be merged into val instead of it being overwritten?

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 1, 2020

Author Member

Yes, indeed that is the case: notice how the updated &val will be passed to the next schema two lines up.

rkuhn and others added 3 commits Mar 1, 2020
Co-Authored-By: Oliver Wangler <oliver@wngr.de>
Co-Authored-By: Oliver Wangler <oliver@wngr.de>

for url in self.schemes.iter() {
let schema = scope.resolve(url);
// second pass if defaults are enabled to check that the result is stable

This comment has been minimized.

Copy link
@diego-actyx

diego-actyx Mar 3, 2020

What is an example of a schema where you might have divergent defaults? And can we add it as a test case?

This comment has been minimized.

Copy link
@rkuhn

rkuhn Mar 12, 2020

Author Member

good point, I shall try to make a small one

@wngr
wngr approved these changes Mar 9, 2020
Copy link

wngr left a comment

This is good to go from my point of view.

@rkuhn

This comment has been minimized.

Copy link
Member Author

rkuhn commented Mar 12, 2020

Performance measurements (debug build):

  • this PR with supply_defaults: 16.7ms compilation, 4.4ms validation
  • this PR without supply_defaults: 14.5ms compilation, 1.9ms validation
  • 3.2.0: 15.7ms compilation, 1.7ms validation

measurements with release build:

  • this PR with supply_defaults: 1.9ms compilation, 560µs validation
  • this PR without supply_defaults: 1.7ms compilation, 170µs validation
  • 3.2.0: 1.8ms compilation, 150µs validation

Conclusion: about 10% slow-down when not using the new feature.

@wngr

This comment has been minimized.

Copy link

wngr commented Mar 12, 2020

what about a feature flag? .. but that's probably up to the maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.