Tests for propertyNames with additionalProperties/unevaluatedProperties #733

Era-cell · 2024-04-11T09:47:29Z

Addressing the issue: 305. Also inculded "specification" property,
I have added it such a way that property names cover pattern and length constraints too.
Please review my PR

…operties, also with specification property

gregsdennis

As I described in #732, were testing additionalProperties and unevaluatedProperties here, not propertyNames. These tests need to be moved to the appropriate files.

gregsdennis · 2024-04-11T09:52:51Z

tests/draft2020-12/propertyNames.json

@@ -1,6 +1,7 @@
 [
    {
        "description": "propertyNames validation",
+        "specification": [ { "core": "10.3.2.4" } ],


If we haven't finalized these additions (the discussion for which is elsewhere), we should hold off adding them here. Please remove until that discussion had come to a final decision.

Ok, cool with that 👍

gregsdennis · 2024-04-11T09:58:15Z

tests/draft2020-12/propertyNames.json

@@ -81,5 +84,60 @@
                "valid": true
            }
        ]
+    },
+    {
+        "description": "propertyNames with additionalProperties",


These test cases aren't actually testing whether additional properties are allowed.

This test is to actually check if additionalProperties affect propertyNames.. so can you please recheck according to the requirement of the issue #305 @gregsdennis

"In this test case, the validation of data fails. For example:

{ "description": "property name is invalid", "data": { "watermelon": 6 }, "valid": false }

This failure occurs due to the exceeded property length in the validation of propertyName, rather than due to unevaluated properties. Therefore, this test does not actually determine whether additional properties are allowed or not."

So, "propertyNames doesn't interact with other keywords." addressed in #305 this issue. This statement means that these test cases I have written should be included under additional/unevaluated Properties and not propertyNames??

This test is to actually check if additionalProperties affect propertyNames

No, that's backward. There's no reason additionalProperties would influence propertyNames, but the spec could be misread that propertyNames defines properties that additionalProperties would then skip. That's what the issue is about.

For example, your first test case with "apple" should be invalid because the property isn't allowed.

The tests are about additionalProperties.

So, we should be checking whether propertyNames affect adiditionalProperties. Got it, I feel we can go both the way on the tests if its ok.

I feel we can go both the way on the tests if its ok.

There's no need. There's nothing in the spec that suggests additionalProperties could affect propertyNames. We don't have tests that ensure maxLength and items are independent because nothing implies that they're connected.

I see. I get it! so tests are written when the information in ambiguous, especially in spec..

Yes, that and explicit requirements.

Cool cool! Can you review the latest changes...

gregsdennis · 2024-04-11T10:00:11Z

tests/draft2020-12/propertyNames.json

+        ]
+    },
+    {
+        "description": "propertyNames with unevaluatedProperties",


These aren't testing whether unevaluated properties are allowed.

Era-cell · 2024-04-11T12:20:53Z

As I described in #732, were testing additionalProperties and unevaluatedProperties here, not propertyNames. These tests need to be moved to the appropriate files.

H.. Actually

As I described in #732, were testing additionalProperties and unevaluatedProperties here, not propertyNames. These tests need to be moved to the appropriate files.

Huh.. this PR is related to a different issue than issue #626. The issue #305 description says:

Verify that "propertyNames" and "additionalProperties"/"unevaluatedProperties" don't interact.
This has come up as a question a few times. propertyNames doesn't interact with other keywords.
I feel this issue wants to address more on properttyNames rather than additionalProperties and unevaluatedProperties. So, can you please confirm on that...

MeastroZI · 2024-04-11T12:39:25Z

@Era-cell, propertyNames is functioning as expected, but there is change in the behavior of additionalProperties and unevaluatedProperties for propertyNames. So because of that tests related to these properties should be included in the additionalProperties and unevaluatedProperties files.

gregsdennis · 2024-04-12T07:13:08Z

but there is change in the behavior of additionalProperties and unevaluatedProperties for propertyNames.

No. There is no change in behavior. The tests are to verify that.

gregsdennis

These look good. Please add them to 2019 and "next".

…y for unevaluatedProps

Era-cell · 2024-04-12T09:45:43Z

These look good. Please add them to 2019 and "next".

I saw just now, that the tests are present already for unevaluatedProperties from 2019 - draft-next, already. So, I have considered to add tests only for additionalProperties

MeastroZI · 2024-04-12T12:17:42Z

but the spec could be misread that propertyNames defines properties that additionalProperties would then skip. That's what the issue is about.

So for this schema

{"propertyNames": {
    "maxLength": 5
  },
  "additionalProperties": false
}

Should the data { apple: "apple" } be considered valid or not? because It appears we don't have tests for this specific case also.

gregsdennis · 2024-04-12T23:09:50Z

Correct, { "apple": "apple" } is an invalid instance for that schema.

We do have that test with the latest update: see line 175 in the first file.

gregsdennis

Asked for another pair of eyes.

jdesrosiers

lgtm, but needs a little code style cleanup. I left suggestions inline.

tests/draft-next/additionalProperties.json

tests/draft2020-12/propertyNames.json

Co-authored-by: Jason Desrosiers <jdesrosi@gmail.com>

Era-cell · 2024-04-13T08:25:23Z

I have updated the changes, and Thank you

notEthan · 2024-04-17T08:46:36Z

tests/draft-next/additionalProperties.json

+                "valid": true
+            },
+            {
+                "description": "Valid against propertyNames, but not unevaluatedProperties",


this says unevaluatedProperties, should say additionalProperties I think.
same for the other two.

Yeah! let me just correct it and raise the pull request again.
I will have to be more careful... Thank you

added tests for propertyNames with additionalProperties/unevaluatedPr…

99864ff

…operties, also with specification property

Era-cell requested a review from a team as a code owner April 11, 2024 09:47

gregsdennis requested changes Apr 11, 2024

View reviewed changes

Cases go under additional and unevaluated Properties

f08b884

gregsdennis reviewed Apr 12, 2024

View reviewed changes

propertyNames doesn't affect additionalProperties, tests exist alread…

964efb8

…y for unevaluatedProps

gregsdennis requested a review from a team April 13, 2024 00:49

gregsdennis approved these changes Apr 13, 2024

View reviewed changes

jdesrosiers approved these changes Apr 13, 2024

View reviewed changes

tests/draft-next/additionalProperties.json Outdated Show resolved Hide resolved

tests/draft-next/additionalProperties.json Outdated Show resolved Hide resolved

tests/draft2020-12/propertyNames.json Outdated Show resolved Hide resolved

Era-cell and others added 3 commits April 13, 2024 13:53

Update tests/draft2020-12/propertyNames.json

c5f3e4e

Co-authored-by: Jason Desrosiers <jdesrosi@gmail.com>

Update tests/draft-next/additionalProperties.json

616240b

Co-authored-by: Jason Desrosiers <jdesrosi@gmail.com>

Update tests/draft-next/additionalProperties.json

3e0139a

Co-authored-by: Jason Desrosiers <jdesrosi@gmail.com>

Era-cell added 2 commits April 13, 2024 13:56

Update additionalProperties.json formatting it

6aa79c0

Update additionalProperties.json formatting it

2480edb

gregsdennis merged commit 3d5048e into json-schema-org:main Apr 14, 2024
1 check passed

notEthan reviewed Apr 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for propertyNames with additionalProperties/unevaluatedProperties #733

Tests for propertyNames with additionalProperties/unevaluatedProperties #733

Era-cell commented Apr 11, 2024 •

edited

Loading

gregsdennis left a comment

gregsdennis Apr 11, 2024

Era-cell Apr 11, 2024

gregsdennis Apr 11, 2024

Era-cell Apr 11, 2024 •

edited

Loading

MeastroZI Apr 11, 2024

Era-cell Apr 11, 2024 •

edited

Loading

gregsdennis Apr 11, 2024 •

edited

Loading

Era-cell Apr 11, 2024

gregsdennis Apr 11, 2024

Era-cell Apr 12, 2024 •

edited

Loading

gregsdennis Apr 12, 2024

Era-cell Apr 12, 2024

gregsdennis Apr 11, 2024

Era-cell commented Apr 11, 2024 •

edited

Loading

MeastroZI commented Apr 11, 2024 •

edited

Loading

gregsdennis commented Apr 12, 2024

gregsdennis left a comment

Era-cell commented Apr 12, 2024

MeastroZI commented Apr 12, 2024

gregsdennis commented Apr 12, 2024

gregsdennis left a comment

jdesrosiers left a comment

Era-cell commented Apr 13, 2024 •

edited

Loading

notEthan Apr 17, 2024

Era-cell Apr 17, 2024 •

edited

Loading

Tests for propertyNames with additionalProperties/unevaluatedProperties #733

Tests for propertyNames with additionalProperties/unevaluatedProperties #733

Conversation

Era-cell commented Apr 11, 2024 • edited Loading

gregsdennis 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 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Era-cell Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

gregsdennis Apr 11, 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 12, 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 commented Apr 11, 2024 • edited Loading

MeastroZI commented Apr 11, 2024 • edited Loading

gregsdennis commented Apr 12, 2024

gregsdennis left a comment

Choose a reason for hiding this comment

Era-cell commented Apr 12, 2024

MeastroZI commented Apr 12, 2024

gregsdennis commented Apr 12, 2024

gregsdennis left a comment

Choose a reason for hiding this comment

jdesrosiers left a comment

Choose a reason for hiding this comment

Era-cell commented Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Era-cell Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Era-cell commented Apr 11, 2024 •

edited

Loading

Era-cell Apr 11, 2024 •

edited

Loading

Era-cell Apr 11, 2024 •

edited

Loading

gregsdennis Apr 11, 2024 •

edited

Loading

Era-cell Apr 12, 2024 •

edited

Loading

Era-cell commented Apr 11, 2024 •

edited

Loading

MeastroZI commented Apr 11, 2024 •

edited

Loading

Era-cell commented Apr 13, 2024 •

edited

Loading

Era-cell Apr 17, 2024 •

edited

Loading