JSON-Schema-Test-Suite icon indicating copy to clipboard operation
JSON-Schema-Test-Suite copied to clipboard

comprehensive test of keyword independence

Open karenetheridge opened this issue 5 years ago • 19 comments
trafficstars

Keywords that are only in effect with instance data of one type should never interfere with keywords that require a different instance type. In particular, implementations should not attempt to infer the type of the instance data based on the keywords used by the schema.

The draft2019-09 data was generated with the script below. Drafts 6 and 7 were slightly adjusted by swapping out 'dependentRequired' and 'dependentSchemas' for 'dependencies'; draft4 removed the use of exclusiveMaximum and exclusiveMinimum because in this draft they require the presence of maximum or minimum. All combinations of keywords appear here, so long as the keywords require a different core data type; for each schema, four sets of data are created, representing the two data types expected by the schema, with a passing and failing test for each.

#!/usr/bin/env perl
use strict;
use warnings;

# data format:
# {
#   $type => { $keyword => { schema => $schema_value, data => [ $failing, $passing ] } },
#   ...,
# }

my $data = {
  number => {
    multipleOf => { schema => 2, data => [ 1, 2 ] },
    maximum => { schema => 2, data => [ 3, 2 ] },
    exclusiveMaximum => { schema => 2, data => [ 2, 1 ] },
    minimum => { schema => 2, data => [ 1, 2 ] },
    exclusiveMinimum => { schema => 2, data => [ 2, 3 ] },
  },
  string => {
    maxLength => { schema => 2, data => [ 'hello', 'hi' ] },
    minLength => { schema => 2, data => [ 'x', 'hi' ] },
    pattern => { schema => 'hi', data => [ 'hello', 'hihi' ] },
  },
  array => {
    maxItems => { schema => 1, data => [ [1,2], [1] ] },
    minItems => { schema => 2, data => [ [1], [1,2] ] },
    uniqueItems => { schema => \1, data => [ [ 1, 1 ], [1] ] },
    items => { schema => \0, data => [ [1], [] ] },
    contains => { schema => \1, data => [ [], [1] ] },
  },
  object => {
    maxProperties => { schema => 1, data => [ {x=>1,y=>2}, { x=>1 } ] },
    minProperties => { schema => 1, data => [ {}, { x=>1 } ] },
    required => { schema => ['x'], data => [ {}, {x=>1} ] },
    dependentRequired => { schema => {x=>['y']}, data => [ {x=>1}, {x=>1,y=>2} ] },
    dependentSchemas => { schema => {x=>\0}, data => [ {x=>1}, {} ] },
    properties => { schema => {x=>\0}, data => [ {x=>1}, {} ] },
    patternProperties => { schema => {hi=>\0}, data => [ {hihi=>1}, {hello=>1} ] },
    additionalProperties => { schema => \0, data => [ {x=>1}, {} ] },
    propertyNames => { schema => \0, data => [ {x=>1}, {} ] },
  },
};

my @tests;

foreach my $type1 (sort keys %$data) {
  foreach my $type2 (sort keys %$data) {
    next unless ($type1 cmp $type2) == -1;                                                                           

    foreach my $keyword1 (sort keys %{$data->{$type1}}) {
      foreach my $keyword2 (sort keys %{$data->{$type2}}) {
        push @tests, {
          description => "$keyword1 + $keyword2",
          schema => {
            $keyword1 => $data->{$type1}{$keyword1}{schema},
            $keyword2 => $data->{$type2}{$keyword2}{schema},
          },
          tests => [
            map {
              my $keyword = $_;
              my $type = $keyword eq $keyword1 ? $type1 : $type2;
              map {
                my $valid = $_;
                +{
                  description => "$type, $keyword ".($valid?'':'in').'valid',
                  data => $data->{$type}{$keyword}{data}[$valid],
                  valid => $valid ? \1 : \0,
                }
              } 0, 1
            } $keyword1, $keyword2
          ],
        },
      }
    }
  }
}

use JSON::MaybeXS;
print JSON::MaybeXS->new(canonical => 1, pretty => 1, indent_length => 4)->encode(\@tests);

karenetheridge avatar Jun 03 '20 03:06 karenetheridge

I know at least one impl which thinks that [1, 1] conforms to { required: [], uniqueItems: true }, because of required.

Could you include this test too, please?

ChALkeR avatar Jun 03 '20 03:06 ChALkeR

Oh, I missed how large this test is. ~Let me recheck if it covers that, will update this comment.~

Upd: apparently this test does not cover it, but that impl also fails other combinations.

ChALkeR avatar Jun 03 '20 03:06 ChALkeR

I forgot that boolean schemas weren't allowed in draft4, which the CI tests helpfully spotted. fix squashed!

karenetheridge avatar Jun 03 '20 03:06 karenetheridge

Why is this testing uniqueItems: {} in draft4?

ChALkeR avatar Jun 03 '20 03:06 ChALkeR

Why is that false in draft4? It's also not an object.

ChALkeR avatar Jun 03 '20 03:06 ChALkeR

Upd: apparently this test does not cover it, but that impl also fails other combinations.

uniqueItems + required is one of the combinations tested. it starts on line 2450.

karenetheridge avatar Jun 03 '20 04:06 karenetheridge

Something still looks very wrong with draft4, in either my impl or this test. And I'm blaming the test :smile:.

    {
        "description" : "contains + additionalProperties",
        "schema" : {
            "additionalProperties" : {},
            "contains" : true
        },
        "tests" : [
            ...
            {
                "data" : {
                    "x" : 1
                },
                "description" : "object, additionalProperties invalid",
                "valid" : false
            },
            ...
        ]
    },

Why is that false?

draft6/draft7 look fine against the impl, draft2019-09 untested.

Upd: same with ajv, it seems.

ChALkeR avatar Jun 03 '20 04:06 ChALkeR

@karenetheridge Ah. I noticed that it was labeled as ready for review :slightly_smiling_face:

ChALkeR avatar Jun 03 '20 04:06 ChALkeR

No, sorry, that was a mistake. I was still running tests and diffs on my end and fixing the last remaining errors from removing boolean schemas for draft4.

karenetheridge avatar Jun 03 '20 04:06 karenetheridge

ok, I think this is good to go now. I have run all the files locally and they all check out (as they should, because they are dead-simple.. except for the errors I made swapping out the boolean schemas for draft4) 👯‍♂️

karenetheridge avatar Jun 03 '20 04:06 karenetheridge

Seems to work for me (draft2019-09 untested).

Also, this is beneficial, I am aware of implementations which are triggered by these checks. E.g. http://npmjs.com/is-my-json-valid

ChALkeR avatar Jun 03 '20 04:06 ChALkeR

@ChALkeR thanks for the review! With large changes like this, especially across multiple drafts, it's quite tricky to ensure there are no mistakes and that the files are reasonably consistent. Even programmatically generating the data like I did this time, I still made several errors when translating between drafts, as you saw :)

With the number of test cases now, and the speed in which new ones are coming in, we are rapidly getting closer to needing an extra verification step where the tests are run across multiple implementations... e.g. #314 .

karenetheridge avatar Jun 03 '20 15:06 karenetheridge

For reference: https://github.com/mafintosh/is-my-json-valid/issues/179

ChALkeR avatar Jun 04 '20 17:06 ChALkeR

I cant' review a 20000 line PR, but thanks for including the script, that I can review, so if that seems correct this should be fine, will see if I can Perl hard enough to see what it does.

Julian avatar Jun 05 '20 19:06 Julian

yeah, the patch is a bit big :D

karenetheridge avatar Jun 05 '20 20:06 karenetheridge

Could this perhaps include the code to rebuild those files, in some form?

ChALkeR avatar Jun 09 '20 00:06 ChALkeR

I'll find the code that generated this and add it (we probably need a new directory to store these sorts of things, since we're doing it in other PRs too now).

karenetheridge avatar Jul 15 '20 16:07 karenetheridge

I'll find the code that generated this and add it (we probably need a new directory to store these sorts of things, since we're doing it in other PRs too now).

Yeah @ChALkeR had that good idea -- we should (whatever language is fine probably as long as it's reasonably easy to run. Perl I'm sure counts which is what I think you said you wrote this in originally?)

But as in https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/414#issuecomment-670727745 can you pick some reasonably small subset of these (5 say) that we'll merge to the regular suite and cover a majority of what these'd cover, and then the rest we document by saying if you want the full comprehensive generated thing run the script you'll add?

Julian avatar Aug 07 '20 21:08 Julian

I cant' review a 20000 line PR - @Julian

I've run these on my implementation. They all pass.

gregsdennis avatar Apr 18 '23 22:04 gregsdennis