adding specification enhancement for additionalProperties 2020-12 #726

Era-cell · 2024-04-03T17:54:19Z

For some test-cases I could not find the exact location where test-case can refer perfectly
I am making this first PR on Test-Suite please guide me if I need correction or even completely wrong

karenetheridge · 2024-04-03T18:08:32Z

test-schema.json

+                    "jsonschema": {"type": "string"},
+                    "rfc": {"type": "string"}


Are these both URIs?

They are URL's for a particular section in the JSON Schema Spec. They will redirect to the location where test-case is more relevant.
They are supposed to be, can add regex if pattern matching for scheme is required

Is the structure of "specification" property supposed to be something else

These are supposed to be informative comments. It's not really anything that needs to be linked to. We really just need some sort of reference. I think just the spec name and section number will suffice.
"additionalProperties": { "type": "string", "pattern": "^[0-9](\.[0-9])*$"} }
which would give something like
{ "core": "8.5.2", "validation": "6.3" }

What are "jsonschema" and "rfc" intended to refer to?

2.. Actually each section of the draft has many paragraphs, and each paragraph has unique URI's.
like, https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-01#section-10.3.2.3-2 URI will redirect to paragraph 2 of "additionalProperties" section.
So, I thought why not that can be made use. And rfc -> too an URI for any section of RFC the description of the test located in.

Wouldn't it make test cases bigger. And also we can provide more precise location using URLs. So, why not URI's?

as we may need to provide multiple uris?

usability of

{ "core": "8.5.2", "validation": "6.3" } ``` this will be greater??

https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-01#section-10.3.2.3-2

If you look at this format it has a common pattern.

https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-01#section-

<section>-<para>

I think it'd be better just to include the last part of that, the section and paragraph. If desired, tooling can construct the URL. The spec name (core/validation) indicates which document were referring to.

Again, this is just a reference for documentation purposes. It doesn't need to be functional.

I totally got that. We can have spec name: core/validation and for rfc: rfc3456/rfc4567 or ...
Then we can have---> section
---> para
So, mostly 3-4 indicators. Right? ❤️‍🔥

@Julian what is the philosophy of tests for requirements like "behaves per X spec, section Y"? Do we include tests checking behavior against other specs?

Like why would we include that info? Yeah we definitely have lots of that, see e.g. all the format tests, all of which essentially are testing behavior specified in some other specification and generally RFC.

But def fine to leave it out to start I think.

(I haven't read the rest of the thread yet!)

Can the example structure for a test-case be:

{ "specification":{ "jsonschema-validation":{ "section": "6.3.1", "para": "3" }, "rfc8259":{ "section": 7 }, // "jsonschema-core":{...} } }

and because there may be more than one place in spec, we can make for example:

"jsonschema-validation":[{ "section": "6.3.1", "para": "3" }, { "section": "6.3.1", "para": "4" }]

can keep it such that "jsonschema-validation" can take both array and object..

And instead of asking can I make as many commits as possible and then take comments?

Julian

Some comments from me, though I agree with the other 2 as well, though the schema here is obviously what we need to work out to start.

Julian · 2024-04-04T18:04:49Z

test-schema.json

@@ -27,6 +27,20 @@
                "type": "array",
                "items": { "$ref": "#/$defs/test" },
                "minItems": 1
+            },
+            "specification":{
+                "description": "Location of the test case in the specification",


It's not just "the" specification, given it can either be the JSON Schema specification or some other specification. Probably we should elaborate here with some guidance for how to use this, specifically:

Suggested change

"description": "Location of the test case in the specification",

"description": "A reference to a specification document which defines the behavior tested by this test case. Typically this should be a JSON Schema specification document, though in cases where the JSON Schema specification points to another RFC it should contain *both* the portion of the JSON Schema specification which indicates what RFC (and section) to follow as *well* as information on where in that specification the behavior is specified.",

Got this 👍

Julian · 2024-04-04T18:06:44Z

test-schema.json

+            },
+            "specification":{
+                "description": "Location of the test case in the specification",
+                "type": "object",


I suspect an array is more correct here actually , which would allow referencing multiple locations, perhaps even within one spec.

(Or if we want to get fancy at the expense of making it harder to parse we could do oneOf: [{items: spec}, spec] and allow either a spec or else an array of specs. But always requiring an array seems fine to me personally.)

Concluding this, which of them is better to be chosen:

"specifications": [ { "core": "6.7.1", "quote": "Lorem" }, { "core": "7.4.3" }, { "validation": "12.2.1" } ]

"specification": [ { "core" : [ {"section": "6.7.1", "quote": "Lorem" }, {"section": "7.4.3", "quote": "LoremEpsom" } ] }, { "validation": [ {"section": "12.2.1", "quote": "Example quote" } ] } ]

specification is an object:

"specification": { { "core" : [ {"section": "6.7.1", "quote": "Lorem" }, {"section": "7.4.3", "quote": "LoremEpsom" } ] }, { "validation": [ {"section": "12.2.1", "quote": "Example quote" } ] } }

I felt first one is simpler, but doesn't mention its value is a "section" explicitely

Always having an array is easier to handle in some languages.

Concluding this, which of them is better to be chosen:

I'd go with number 1 personally.

Always having an array is easier to handle in some languages.

Makes sense.

Julian · 2024-04-04T18:07:14Z

test-schema.json

+                "type": "object",
+                "minProperties": 1,
+                "properties": {
+                    "jsonschema-core": { "$ref": "#/$defs/spec"},


core and validation are going to be the most common values here so I think it's fine to leave off the jsonschema.

Also I doubt this reference (spec) will be useful anywhere else except within this subschema, so I'd put it here for readability personally, rather than below.

Julian · 2024-04-04T18:07:23Z

test-schema.json

+                    "ecma262": { "$ref": "#/$defs/spec"}
+                },
+                "patternProperties": {
+                    "^rfc[0-9]{4}$": {"$ref": "#/$defs/spec"}


(RFCs are not always 4 digits long.)

I see some RFC names are defines like this: RFC 3162 (IPv6), RFC 1058 (v.1), RFC 2080 (v.ng), RFC 23

All RFC's have 2- 4 digits, most RFC's used in spec arent using aany of first three. So, just matching for digits is good?

Yes, probably just "1 or more digits" is fine.

Julian · 2024-04-04T18:08:23Z

test-schema.json

+            "properties": {
+                "section": {
+                    "type": "string",
+                    "pattern": "^\\d+(\\.\\d+)*\\.$"


This seems wrong (in that it ends in a period -- presumably that's simply a formatting period if it appears in the doc) but also too specific to me. Not all specifications will have this form of heading sections, it's perfectly believable that one will use e.g. letters in the section names, so I.2.IV or whatever.

I see @gregsdennis left you some helpful comments, I'd read those, and then probably we could even consider having different definitions of section depending on the allowed specification values if we really want to make sure they're correct.

The regex I left was an example, if we wanted to even enforce such a thing.

Julian has a good point about section numbering not being consistent across specs.

So, sections can also contain 1.A, 1.2.B, I.V.1.a, anything right ? so we can check for only periods in between?

Julian · 2024-04-04T18:10:52Z

test-schema.json

+                    "type": "string",
+                    "pattern": "^\\d+(\\.\\d+)*\\.$"
+                },
+                "para": {


I don't like abbreviations personally, but also I don't think paragraph numbers are that much fun, they're hard both for the person writing the thing as well as the reader, both of whom have to count to find what they want.

Ideally everything should have its own anchor and/or section link though certainly they do not today, but I think probably the right thing to do there is quote -- i.e. a field that simply allows someone to copy-paste some substring from the spec to put here, and then they copy paste a sentence or so.

Maybe this should be used sparingly, like only when the section alone isn't sufficient.

I also agree with the abbreviation thing, especially considering that users of the test suite don't necessarily understand English. Abbreviations in foreign language are really hard, in general.

Yes definitely agree there!

Got this one too

Era-cell · 2024-04-05T17:36:55Z

Hi @Julian @gregsdennis @karenetheridge I have updated my PR

test-schema.json

Julian · 2024-04-05T17:40:34Z

test-schema.json

+                "type": "array",
+                "minItems": 1,
+                "items":{
+                    "properties": {


I think all the values here will be (should be) strings, so probably you can/should use propertyNames here (probably with a oneOf + enum + pattern).

test-schema.json

Julian · 2024-04-05T17:42:42Z

tests/draft2020-12/additionalProperties.json

@@ -2,6 +2,7 @@
    {
        "description":
            "additionalProperties being false does not allow other properties",
+        "specification": [ { "core":"10.3.2.3", "quote": "The value of \"additionalProperties\" MUST be a valid JSON Schema. Boolean \"false\" forbids everything." } ],


A good follow up (to this ticket, perhaps even before we go much further) would be to make CI annotate pull requests with links to the section.

It'd be a lot easier to check whether this was correct if something were annotating the review with a link to this section.

So, for each version of the draft, we should add different "baseURI's" right which should append and check whether the the redirection to the section is failing or not?
And Ci script should be written separately for that.. I am new to this, may take time. But i'll do it

Probably custom logic for each specification which knows what the right URL is given what we have put in the corresponding field, yes, which the simplest version is just "a base URI".

But exactly right. Some CI script for that, which annotates PRs, and warns for broken links.

Can you perhaps immediately open an issue at least so we don't forget about it? Then yeah if you give it a shot that'd be amazing.

test-schema.json

Era-cell · 2024-04-05T20:38:02Z

For the validation and core specifications metadata: "url" there can be multiple versions of drafts: because the test-schema is same for all dialects.
So, I have included the link to all specification links is that okay?
And for rfc's I have mentioned the base URL's telling that is numbers

gregsdennis

I'm happy with this. @Julian has final say.

Julian · 2024-04-06T13:03:24Z

test-schema.json

+                                "pattern": "^((iso)|(rfc))[0-9]+$"
+                            },
+                            {
+                                "enum": [ "core", "validation", "ecma262", "perl5", "quote" ]


Considering we need this enum and that IIRC propertyNames doesn't affect unevaluatedProperties (meaning we can't use that instead of the additionalProperties above) my suggestion (to use propertyNames was probably bad, but whatever, we can evolve the schema later if need be. Thanks for putting this in.

Julian · 2024-04-06T13:04:56Z

Thanks! This looks good to me too, well done.

For now we do nothing with this, maybe at some point there will be some way it's useful to use in Bowtie itself. Refs: json-schema-org/JSON-Schema-Test-Suite#726

Era-cell · 2024-04-06T18:21:54Z

Thanks! This looks good to me too, well done.

Cool, then lets go ahead.❤️‍🔥

adding specification enhancement for additionalProperties

50a2028

Era-cell requested a review from a team as a code owner April 3, 2024 17:54

Test-schema including sub-schema for scpecification

003ac02

karenetheridge reviewed Apr 3, 2024

View reviewed changes

Era-cell added 2 commits April 4, 2024 20:05

Schema of specification in much structured

340116e

Pattern for para corrected

1362a8c

Julian reviewed Apr 4, 2024

View reviewed changes

specification takes array of objects having section and quote

9b169be

Julian reviewed Apr 5, 2024

View reviewed changes

test-schema.json Outdated Show resolved Hide resolved

Julian reviewed Apr 5, 2024

View reviewed changes

test-schema.json Show resolved Hide resolved

Julian reviewed Apr 5, 2024

View reviewed changes

gregsdennis requested changes Apr 5, 2024

View reviewed changes

test-schema.json Outdated Show resolved Hide resolved

meta data and property names constraints added, additional Items: string

51fc69c

gregsdennis approved these changes Apr 5, 2024

View reviewed changes

gregsdennis requested a review from Julian April 5, 2024 23:27

Julian reviewed Apr 6, 2024

View reviewed changes

Julian merged commit 57617f2 into json-schema-org:main Apr 6, 2024
1 check passed

Julian mentioned this pull request Apr 6, 2024

Add a specification property to test cases + tests #699

Open

adding specification enhancement for additionalProperties 2020-12 #726

adding specification enhancement for additionalProperties 2020-12 #726

Conversation

Era-cell commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Era-cell Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Era-cell Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

gregsdennis Apr 3, 2024 • 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

Era-cell Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Julian 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

Era-cell Apr 4, 2024 • 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

Choose a reason for hiding this comment

gregsdennis Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsdennis Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Era-cell commented Apr 5, 2024

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

Era-cell commented Apr 5, 2024 • edited Loading

gregsdennis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julian commented Apr 6, 2024

Era-cell commented Apr 6, 2024

Era-cell commented Apr 3, 2024 •

edited

Loading

Era-cell Apr 3, 2024 •

edited

Loading

Era-cell Apr 3, 2024 •

edited

Loading

gregsdennis Apr 3, 2024 •

edited

Loading

Era-cell Apr 4, 2024 •

edited

Loading

Era-cell Apr 4, 2024 •

edited

Loading

gregsdennis Apr 4, 2024 •

edited

Loading

gregsdennis Apr 4, 2024 •

edited

Loading

Era-cell commented Apr 5, 2024 •

edited

Loading