Skip to content

adding specification enhancement for additionalProperties 2020-12 #726

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 6, 2024
Merged

adding specification enhancement for additionalProperties 2020-12 #726

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
50a2028
adding specification enhancement for additionalProperties
suprith-hub Apr 3, 2024
003ac02
Test-schema including sub-schema for scpecification
suprith-hub Apr 3, 2024
340116e
Schema of specification in much structured
suprith-hub Apr 4, 2024
1362a8c
Pattern for para corrected
suprith-hub Apr 4, 2024
9b169be
specification takes array of objects having section and quote
suprith-hub Apr 5, 2024
51fc69c
meta data and property names constraints added, additional Items: string
suprith-hub Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions test-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@
"type": "array",
"items": { "$ref": "#/$defs/test" },
"minItems": 1
},
"specification":{
"description": "Location of the test case in the specification",
Copy link
Member

Choose a reason for hiding this comment

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

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.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got this 👍

"type": "object",
Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@suprith-hub suprith-hub Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. "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" } ] }
          ]
  1. 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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great

"minProperties": 1,
"properties": {
"jsonschema-core": { "$ref": "#/$defs/spec"},
Copy link
Member

Choose a reason for hiding this comment

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

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.

"jsonschema-validation": {"$ref": "#/$defs/spec"},
"ecma262": { "$ref": "#/$defs/spec"}
},
"patternProperties": {
"^rfc[0-9]{4}$": {"$ref": "#/$defs/spec"}
Copy link
Member

Choose a reason for hiding this comment

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

(RFCs are not always 4 digits long.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

},
"additionalProperties": false
}
},
"additionalProperties": false
Expand Down Expand Up @@ -56,6 +70,18 @@
}
},
"additionalProperties": false
},
"spec": {
"properties": {
"section": {
"type": "string",
"pattern": "^\\d+(\\.\\d+)*\\.$"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@gregsdennis gregsdennis Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

},
"para": {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@gregsdennis gregsdennis Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes definitely agree there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got this one too

"type": "string",
"pattern": "^\\d+(\\.\\d+)*$"
}
}
}
}
}
35 changes: 35 additions & 0 deletions tests/draft2020-12/additionalProperties.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
{
"description":
"additionalProperties being false does not allow other properties",
"specification":{
"jsonschema-core": {
"section": "10.3.2.3.",
"para": "1"
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"properties": {"foo": {}, "bar": {}},
Expand Down Expand Up @@ -43,6 +49,11 @@
},
{
"description": "non-ASCII pattern with additionalProperties",
"specification":{
"rfc8259":{
"section": "4."
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"patternProperties": {"^á": {}},
Expand All @@ -63,6 +74,12 @@
},
{
"description": "additionalProperties with schema",
"specification":{
"jsonschema-core": {
"section": "10.3.2.3.",
"para": "1"
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"properties": {"foo": {}, "bar": {}},
Expand Down Expand Up @@ -108,6 +125,12 @@
},
{
"description": "additionalProperties are allowed by default",
"specification":{
"jsonschema-core": {
"section": "10.3.2.3.",
"para": "5"
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"properties": {"foo": {}, "bar": {}}
Expand All @@ -122,6 +145,12 @@
},
{
"description": "additionalProperties does not look in applicators",
"specification":{
"jsonschema-core": {
"section": "10.2.",
"para": "2"
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"allOf": [
Expand All @@ -139,6 +168,12 @@
},
{
"description": "additionalProperties with null valued instance properties",
"specification":{
"jsonschema-core": {
"section": "10.3.2.3.",
"para": "1"
}
},
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"additionalProperties": {
Expand Down
Loading