Added test cases from Foundations of JSON Schema research paper #128

iainbeeston · 2016-09-30T13:33:22Z

In the 25th internationl world wide web conference there was a paper
named "Foundations of JSON Schema", presented by:

Felipe Pezoa ([email protected])
Juan L. Reutter ([email protected])
Fernando Suarez ([email protected])
Martín Ugarte ([email protected])
Domagoj Vrgo ([email protected])

See:

http://www2016.net/proceedings/proceedings/p263.pdf

This paper compared a number of json schema implementations, using a set
of 5 tests which highlighted inconsistencies between the
implementations. I have reproduced those tests here. All credit should
go to the authors of the paper for highlighting these inconsistencies
and publishing the details of them online.

iainbeeston · 2016-09-30T13:33:56Z

Fixes #115

Julian · 2016-09-30T13:52:07Z

Awesome! Very nice work, really appreciated, will definitely have a look. I'm sure it's in the paper which I haven't read, so forgive me for being lazy, but just in case -- what's the license on the tests from the paper?

iainbeeston · 2016-09-30T14:12:22Z

That's a good question - there is no mention of a license in the research paper or in the source code from in the appendix. For all of the tests I've treated them as a bug report and written the smallest, cleanest test that can reproduce the issue, rather than copying any of them directly into the test suite.

Relequestual · 2016-10-03T09:43:19Z

As a note, I was in direct contact with several of the authors, and they wanted the tests to be integrated into the official test suite, but hadn't been able to put any time into it.

iainbeeston · 2016-10-09T07:37:52Z

@Relequestual @Julian Should we get formal approval from the authors? Is there any way to get one of them to 👍 this PR?

Furthermore, @Julian can you confirm that I (and the paper's authors) have interpreted the spec correctly in the tests in this pull request?

iainbeeston · 2016-10-09T07:39:48Z

Incidentally, these tests will fail on a lot of implementations, as shown by the paper. I've also tested them on the implementation I contribute to (https://github.com/ruby-json-schema/json-schema/) and even the latest version does not pass all of them

Relequestual · 2016-10-10T20:15:09Z

Somewhat worrying. I think @Julian will need to review for sure. As for contacting the paper authors, I'm not sure they use github... Would suggest emailing them.

DomagojVrgoc · 2016-10-11T15:56:39Z

Dear All,

this is Domagoj, one of the authors of the paper. Iain contacted me already, but just to restate what I said to him, we do maintain a webpage with more informal introduction to our formalization at http://cswr.github.io/JsonSchema/ in case you want to take a look. Over there, at http://cswr.github.io/JsonSchema/spec/why/, you can also find a bit more details on the tests you were discussing, and some thoughts on what we think was going on with the implementations.

Best,
Domagoj.

iainbeeston · 2016-10-12T08:06:36Z

Thanks @DomagojVrgoc !

iainbeeston · 2016-10-12T08:12:47Z

tests/draft4/uniqueItems.json

+                "description": "property order of array of objects is ignored",
+                "data": [{"foo": "bar", "bar": "foo"}, {"bar": "foo", "foo": "bar"}],
+                "valid": false
+            },


This (and the two array tests below) are my interpretation of the test T1 in the paper, which test that schemas ignore the ordering of properties in JSON objects (according to the JSON rfc, objcet properties are not ordered) but respect the ordering of arrays (which are ordered - this wasn't in the paper but it seems important and closely related)

iainbeeston · 2016-10-12T08:20:48Z

tests/draft4/ref.json

+            }
+        ]
+    },
+    {


This is my implementation of test T2 from the paper, which tests that $refs replace the current object with the contents of the ref (related to #129 )

Fwiw, the draft spec in the repository now specifically advises against schemas with this sort of infinite recursion. The update being released is intended to clarify and be reverse-compatible with draft-04.

I agree with @awwright, I don't think it should be included.

To clarify: The spec says that implementations MUST NOT fall into an infinite loop, but I don't know how we would test this correctly. We're testing that the schema is invalid, not for a particular valid/invalid result

I've removed the recursive tests now. It's good to know that the draft spec has been updated to include that.

iainbeeston · 2016-10-12T08:21:22Z

tests/draft4/refRemote.json

@@ -50,6 +50,26 @@
        ]
    },
    {
+        "description": "ref with other properties",


This also tests T2 but for remote references

iainbeeston · 2016-10-12T08:25:41Z

tests/draft4/ref.json

@@ -141,6 +141,59 @@
        ]
    },
    {
+        "description": "recursive refs",


This tests T4 from the paper, which checks that recsursive $refs are allowed. In this case I have made a schema that tests that implementations aren't thrown into an infinite loop, and can still validate parts of the schema which are not infinite.

iainbeeston · 2016-10-12T08:31:35Z

tests/draft4/dependencies.json

@@ -109,5 +109,43 @@
                "valid": false
            }
        ]
+    },
+    {


There was an additional test in the code in the appendix, which was not described in the paper, that tests what happens when dependences extend the schema with additonal rules. I think this isn't so much an edge case as something not covered by the test suite at present

iainbeeston · 2016-10-12T08:43:53Z

I didn't include test T3 from the paper because I couldn't see a succinct way of doing it... The original test checks that a schema for an object ignores value-based properties (eg. multipleOf)

I could add a test to every json schema property that refers to values and make sure they do nothing when used on an object or array? (eg minimum, maximum, multipleOf etc) but it would make for a lot of extra tests. I'd appreciate hearing opinions on the best course of action on that test.

Relequestual · 2016-10-13T08:51:23Z

I can't claim I've fully read and understood all of the above, but I would think that a validator would check the schema is valid against the schema for schemas, if that makes sense. So a check would be done to make sure that properties such as minimum are only where they are relevant.

iainbeeston · 2016-10-13T08:56:10Z

So - rather than simply ignore those properties, we would fail validation because the schema itself is invalid? (Is that correct?)

Is that how we handle invalid schemas in the test suite already? (Are there even any tests for schemas that are invalid?)

Relequestual · 2016-10-13T09:00:37Z

If I were writing a library, that's how I'd do it. If you get part way through and find it's partially invalid, you've wasted time. If you take a softer approach, and ignore some parts based on some rules you've made up, then you're asking for unexpected behaviour. I would think it's better to let the user know they have done something wrong.

I don't remember seeing any tests to confirm this behaviour, which is OK, as I also don't remember it being part of the spec. I feel it probably should / could be though. Maybe I should raise an issue to ask the question?

In terms of my experience developing a collaboriative international API, I tend to read rules in a stricter, less forgiving manner than others. Make of that what you will.

iainbeeston · 2016-10-13T09:12:17Z

Yeah I think it's worth raising as another issue.

FYI the ruby json-schema gem validates the user's schema against the appropriate meta schema (eg http://json-schema.org/draft-04/schema), before validating the data against the user's schema. It works well.

If you agree with that way of doing things, if the meta schema says a schema with type: object can have a multipleOf property, the it is valid and it's up to the validator (and the common test suite) to handle that appropriately.

Relequestual · 2016-10-13T09:21:10Z

New issue logged.

Hum. yeah, that's a good point. Should the spec say things like "unless the object is of type x, then properties y and z should be ignore."? If so, then it would make sense to bake that into the metaschema, I would feel.

epoberezkin

I think some tests should be removed.

epoberezkin · 2016-10-14T20:12:44Z

tests/draft4/dependencies.json

+            {
+                "description": "matches dependency",
+                "data": {"bar": 1},
+                "valid": false


I don't understand why this test should fail. By the spec, when the property foo is absent, the schema in "dependencies" keyword should not apply. Can you please explain?

@epoberezkin You're correct - that's a typo. I've corrected it now so that this data is valid.

epoberezkin · 2016-10-14T20:17:29Z

tests/draft4/ref.json

+            }
+        ]
+    },
+    {


I agree with @awwright, I don't think it should be included.

iainbeeston · 2017-02-08T09:58:18Z

Thanks for your very thorough feedback @Relequestual @epoberezkin and @awwright - I've removed several tests that used invalid schemas, and corrected one test that @epoberezkin pointed out had a mistake in it.

I believe this should be ready to merge now but please let me know if I should make further changes.

epoberezkin

I suggest removing $ref tests about ignoring other keywords.

epoberezkin · 2017-02-12T21:42:46Z

tests/draft4/ref.json

+                    "type": "number",
+                    "maximum": 5,
+                    "$ref": "#/definitions/a"
+                }


I would suggest not merging the test for $ref with other keywords in the same schema object, given the on-going discussion (@handrews) about whether they should be validated, ignored or prohibited (making the schema invalid).

I think the way we have in the spec now ("ignored") is the worst option, as it produces unexpected validation successes that are usually the most difficult to detect.

Probably, this test (and a similar one below) could be split into a separate PR so that everything else could be merged.

Thanks @epoberezkin - I will move these two tests into a separate pull request. Can you please confirm which issue you're referring to? (I believe #113 ?)

This, for example: json-schema-org/json-schema-spec#194

Also, should we add the remaining tests to draft-6 as well?

@epoberezkin The current behavior of "$ref" is clear and AFAIK not going to change in Draft 06. Tests that test its current behavior should be accepted.

@handrews @epoberezkin Can I confirm that the $ref tests here do not need to be removed from this PR before merging? (Is that the consensus?)

Or have I misunderstood the current behaviour for additonal properties alongside a $ref? These tests assume that any additonal properties are ignored/overwitten by those in a $ref, my reading of the json ref spec is that this is correct, but I'm not 100% sure. Either way if someone could explicitly clarify for me here I'll make the appropriate changes

Your understanding is correct as far as I undertsand. I haven't however reviewed this specific change.

Thanks @Relequestual

I think that's the final problem with this pull request?

Sorry I simply don't have time to digest the whole issue right now.

epoberezkin · 2017-02-12T21:43:19Z

tests/draft4/refRemote.json

+            "type": "number",
+            "maximum": 5,
+            "$ref": "http://localhost:1234/integer.json"
+        },


Same as above, I would suggest not merging this test

iainbeeston · 2017-02-23T08:30:09Z

@epoberezkin I've added these tests to draft6 now (in addition to draft4)

handrews · 2017-11-19T19:52:24Z

@iainbeeston @epoberezkin @Julian can anyone summarize what's going on here and where we are? We'll also want to copy these to draft-07 (which is backwards-compatible with draft-06, so that should be trivial.

Julian · 2017-11-20T13:32:05Z

The last time I looked I think there were a bunch of stuff here that needed cleaning up after review comments, but will have to have another look to see exactly what / how.

gregsdennis · 2023-04-18T22:16:44Z

Are these tests still desired? It's been... 6 years since anyone looked at them. A rebase is also required.

If not, we should close it.

iainbeeston · 2023-04-19T10:43:09Z

@gregsdennis I've rebased this, added the same tests to draft7 and removed the two ref-based tests that were controversial. This means there are only 2 new test cases added (to drafts 4, 6 and 7)

gregsdennis · 2023-04-19T20:09:39Z

Thanks, @iainbeeston. One last thing: could you add those tests to

draft2019-09
draft2020-12
draft-next

please? You'll need to change dependencies to either dependentSchemas or dependentProperties as appropriate and change any definitions to $defs.

In the 25th internationl world wide web conference there was a paper named "Foundations of JSON Schema", presented by: * Felipe Pezoa ([email protected]) * Juan L. Reutter ([email protected]) * Fernando Suarez ([email protected]) * Martín Ugarte ([email protected]) * Domagoj Vrgo ([email protected]) See: http://www2016.net/proceedings/proceedings/p263.pdf This paper compared a number of json schema implementations, using a set of 5 tests which highlighted inconsistencies between the implementations. I have reproduced those tests here. All credit should go to the authors of the paper for highlighting these inconsistencies and publishing the details of them online.

iainbeeston · 2023-04-21T08:40:07Z

@gregsdennis I've done that now, hopefully it's ready to merge? 🤞

gregsdennis · 2023-04-21T08:42:05Z

Thank you. I still need to run it on my implementation (or someone else can run it on theirs). I can probably do that tomorrow. For now, it's Friday night.

Thanks for updating the PR.

jdesrosiers

Works on my implementation.

gregsdennis

yep. works for me. Thanks for the additions.

Comments have been addressed

iainbeeston mentioned this pull request Sep 30, 2016

Paper suggests that the test suite is lacking #115

Closed

iainbeeston force-pushed the foundations-of-json-schema-paper branch from 7d6cc6b to 2541700 Compare September 30, 2016 13:58

iainbeeston mentioned this pull request Sep 30, 2016

Made sure all $ref objects contain only a $ref property #129

Closed

iainbeeston commented Oct 12, 2016

View reviewed changes

epoberezkin mentioned this pull request Oct 12, 2016

Add tests from from Foundations of JSON Schema research paper ajv-validator/ajv#321

Closed

Relequestual mentioned this pull request Oct 13, 2016

Should the spec suggest or recommend that validators only process json that validates against the metaschema? json-schema-org/json-schema-spec#93

Closed

epoberezkin requested changes Oct 14, 2016

View reviewed changes

Julian closed this Dec 26, 2016

Julian changed the base branch from develop to master December 26, 2016 23:50

Julian reopened this Dec 26, 2016

iainbeeston force-pushed the foundations-of-json-schema-paper branch from 2541700 to 569392f Compare February 8, 2017 09:42

iainbeeston force-pushed the foundations-of-json-schema-paper branch 2 times, most recently from 215981a to 0e1ee65 Compare February 8, 2017 09:56

epoberezkin previously requested changes Feb 12, 2017

View reviewed changes

iainbeeston force-pushed the foundations-of-json-schema-paper branch from 0e1ee65 to 5551a40 Compare February 23, 2017 08:28

handrews added draft support missing test A request to add a test to the suite that is currently not covered elsewhere. labels Nov 8, 2017

Julian removed the draft support label Oct 12, 2019

iainbeeston force-pushed the foundations-of-json-schema-paper branch from 5551a40 to 4d61c78 Compare April 19, 2023 10:32

iainbeeston requested a review from a team as a code owner April 19, 2023 10:32

iainbeeston force-pushed the foundations-of-json-schema-paper branch from 4d61c78 to dbe2cd3 Compare April 19, 2023 10:41

iainbeeston force-pushed the foundations-of-json-schema-paper branch from dbe2cd3 to 07fd389 Compare April 21, 2023 08:37

jdesrosiers approved these changes Apr 21, 2023

View reviewed changes

gregsdennis approved these changes Apr 21, 2023

View reviewed changes

gregsdennis merged commit 8025fc0 into json-schema-org:main Apr 21, 2023

Added test cases from Foundations of JSON Schema research paper #128

Added test cases from Foundations of JSON Schema research paper #128

Conversation

iainbeeston commented Sep 30, 2016

iainbeeston commented Sep 30, 2016

Julian commented Sep 30, 2016

iainbeeston commented Sep 30, 2016

Relequestual commented Oct 3, 2016

iainbeeston commented Oct 9, 2016

iainbeeston commented Oct 9, 2016

Relequestual commented Oct 10, 2016

DomagojVrgoc commented Oct 11, 2016

iainbeeston commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Oct 12, 2016

Relequestual commented Oct 13, 2016

iainbeeston commented Oct 13, 2016

Relequestual commented Oct 13, 2016

iainbeeston commented Oct 13, 2016

Relequestual commented Oct 13, 2016

epoberezkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Feb 8, 2017

epoberezkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epoberezkin Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainbeeston commented Feb 23, 2017

handrews commented Nov 19, 2017

Julian commented Nov 20, 2017

gregsdennis commented Apr 18, 2023

iainbeeston commented Apr 19, 2023

gregsdennis commented Apr 19, 2023

iainbeeston commented Apr 21, 2023

gregsdennis commented Apr 21, 2023

jdesrosiers left a comment

Choose a reason for hiding this comment

gregsdennis left a comment

Choose a reason for hiding this comment

awwright Oct 13, 2016 •

edited

Loading

awwright Oct 14, 2016 •

edited

Loading

epoberezkin Feb 13, 2017 •

edited

Loading