Skip to content

Linter: create rule to remove additional items when items is an object #1805

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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Karan-Palan
Copy link
Contributor

Implementation

Rule ID: dangling_additional_items
Description: "additionalItems is ignored when items is an object"
Condition: Detects additionalItems in JSON Schema objects where items is an object (not an array).
Transform: Removes the redundant additionalItems property.
Dialects: Supports draft4, draft6, draft7, and 2019-09. Excluded for draft-2020-12 (where additionalItems was removed from the spec).

  • Registered the rule in the Common rules section (correctness category).
  • Added 10 test cases across all relevant JSON Schema drafts.

Test Cases

Positive Cases: Removal of additionalItems when items is an object.
Negative Cases: No change for items as an array or when items is undefined.
Edge Cases: Complex items objects (e.g., with if/then conditions) - correctly handled.
Context Preservation: Tests with various other keywords to ensure proper behavior.

…en items is an objects

Signed-off-by: karan-palan <karanpalan007@gmail.com>
@Karan-Palan Karan-Palan marked this pull request as ready for review July 3, 2025 09:18
@@ -52,6 +52,7 @@ static auto every_item_is_boolean(const T &container) -> bool {
#include "linter/content_media_type_without_encoding.h"
#include "linter/content_schema_default.h"
#include "linter/content_schema_without_media_type.h"
#include "linter/dangling_additional_items.h"
Copy link
Member

@jviotti jviotti Jul 3, 2025

Choose a reason for hiding this comment

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

Can we all it additional_items_with_schema_items? The dangling part seems confusing at first and other rules follow the with and without convention. I think additional_items_with_schema_items is still short enough and very self-explanatory

Copy link
Member

Choose a reason for hiding this comment

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

Also, you have to update the CMakeLists.txt file to list this file too

{"https://json-schema.org/draft/2019-09/vocab/applicator",
"http://json-schema.org/draft-07/schema#",
"http://json-schema.org/draft-06/schema#",
"http://json-schema.org/draft-04/schema#"}) &&
Copy link
Member

Choose a reason for hiding this comment

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

I believe this rule applies to Draft 3 as well. Can you confirm on the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does, adding tests for it as well

@@ -1661,8 +1661,7 @@ TEST(AlterSchema_lint_2019_09, unnecessary_allof_ref_wrapper_5) {
sourcemeta::core::JSON document = sourcemeta::core::parse_json(R"JSON({
"$schema": "https://json-schema.org/draft/2019-09/schema",
"allOf": [
{
"type": "integer",
{ "type": "integer",
Copy link
Member

Choose a reason for hiding this comment

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

Unintended change?

@jviotti
Copy link
Member

jviotti commented Jul 3, 2025

I left a few comments. On testing:

  • Can you test cases where items is set to boolean schemas?
  • Can you test cases where items is set to an array and therefore confirm that additionalItems is NOT removed?
  • Can you test what happens if there is a $ref to additionalItems but then additionalItems gets removed?
    • What we can do here is tweak the condition of the existing rule to only match instances of additionalItems that are not referenced by anything (using SchemaFrame in the condition, which has a references() method to introspect on the schema references). And then we probably need another non-auto fixable rule that catches the additionalItems that must be removed but have references to them

@Karan-Palan
Copy link
Contributor Author

  • Renamed the rule, class, file, and all references.
  • Updated CMakeLists.txt
  • Added Draft 3 support with tests.
  • Added comprehensive tests for boolean items, array items, object items, and scenarios where the rule should not apply.

@jviotti , the PR is ready for review

"http://json-schema.org/draft-04/schema#",
"http://json-schema.org/draft-03/schema#"}) &&
schema.is_object() && schema.defines("items") &&
schema.defines("additionalItems") && schema.at("items").is_object();
Copy link
Member

Choose a reason for hiding this comment

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

The is_object check for items is still wrong, as the items keyword can be set to a boolean. Remember that in JSON Schema, a schema is either an object or a boolean. We have an is_schema function for this case

Copy link
Member

Choose a reason for hiding this comment

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

Also I think you are missing test cases covering that case: items set to a boolean

"http://json-schema.org/draft-04/schema#",
"http://json-schema.org/draft-03/schema#"}) &&
schema.is_object() && schema.defines("items") &&
schema.defines("additionalItems") && schema.at("items").is_object();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
schema.defines("additionalItems") && schema.at("items").is_object();
schema.defines("additionalItems") && is_schema(schema.at("items"));

…undo unintended change

Signed-off-by: karan-palan <karanpalan007@gmail.com>
@Karan-Palan Karan-Palan force-pushed the linter/dangling-additional-items branch from 7c994f5 to bb30244 Compare July 8, 2025 18:13
Signed-off-by: karan-palan <karanpalan007@gmail.com>
@Karan-Palan
Copy link
Contributor Author

Karan-Palan commented Jul 8, 2025

@jviotti

I guess I understood, this is how the rule works now:

  • When items is a schema object: additionalItems is removed (because it's ignored)
  • When items is a boolean schema: additionalItems is removed (because it's ignored)
  • When items is an array: additionalItems is preserved (because the rule doesn't apply)

@jviotti
Copy link
Member

jviotti commented Jul 8, 2025

That explanation makes sense!

@Karan-Palan
Copy link
Contributor Author

@jviotti , resolved merge conflicts. Any comments?
@Relequestual, any other tests that come into your mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants