Combine required and dependentRequired error messages by Sam-61s · Pull Request #160 · hyperjump-io/json-schema-errors

Sam-61s · 2026-02-03T15:54:43Z

When required and dependentRequired fail at the same instance location, they previously produced multiple duplicate messages. This change consolidates those failures into a single required-message, combining and deduplicating missing properties while preserving schema locations.

Adds test coverage for the original issue scenario and keeps existing dependentRequired and dependency behavior unchanged.

Fixes #134

jdesrosiers

Try again. You should end up with one error handler that handles required, dependentRequired, and the array form of dependencies. There doesn't need to be a separate error handler for each keyword. They're defined separately specifically for that reason.

Sam-61s · 2026-02-04T01:12:25Z

Thanks for the clarification. I plan to unify required, dependentRequired, and the array-form of dependencies into required.js, while keeping schema-form dependencies handled separately through normal nested validation.

The unified handler will consolidate missing properties into a single required-message, deduplicate properties, and preserve schemaLocations. Since dependentRequired will be handled by the unified handler, I also plan to remove the separate dependentRequired.js handler.

Does that match your expectation?

jdesrosiers · 2026-02-04T05:58:09Z

Yep. That sounds right.

Sam-61s · 2026-02-06T12:06:54Z

I’ve updated the PR based on your guidance. There is now a single unified error handler in required.js that handles required, dependentRequired, and the array form of dependencies.
Could you please take another look when you have a moment? Thanks!

jdesrosiers

Looks good so far. I left a few comments inline, but there's one more thing I'd like you to update in general. When contributing to project, you should try to match the code style used in the project. The way you did it isn't wrong or bad, but it looks significantly different than the rest of the error handlers. It can make it harder to review when the patterns are different. So, I'd like you to rework this a bit so it looks more like other error handlers in the repo. This includes the JSON files. Especially avoid making a lot of unnecessary whitespace changes (like in dependencies.json).

src/error-handlers/required.js

src/normalization-handlers/draft-04/dependencies.js

src/test-suite/tests/required.json

Copilot

Pull request overview

This PR consolidates validation errors that previously produced multiple required-message entries when required and dependentRequired (and some dependency cases) fail at the same instance location, deduplicating the missing property list while preserving schema locations.

Changes:

Update required error handling to aggregate missing properties across required, dependentRequired, and array-form draft-04 dependencies into a single required-message.
Remove the standalone dependentRequired error handler and remove array-form dependencies required-message generation from the draft-04 dependencies error handler.
Update/expand the JSON test-suite expectations to cover the consolidated-message behavior (including the reported issue scenario).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test-suite/tests/required.json	Adjusts existing required expectations and adds a new combined required+dependentRequired test case.
src/test-suite/tests/dependencies.json	Updates expected errors to reflect consolidated required-message behavior for dependentRequired/array-form dependencies and some dependentSchemas cases.
src/normalization-handlers/draft-04/dependencies.js	Minor cleanup (removes blank lines) in dependencies normalization handler.
src/index.js	Unregisters the removed dependentRequired error handler.
src/error-handlers/required.js	Implements combined missing-property aggregation across required/dependentRequired/dependencies.
src/error-handlers/draft-04/dependencies.js	Removes array-form dependency required-message generation; keeps dependent-schema traversal behavior.
src/error-handlers/dependentRequired.js	Deletes the standalone dependentRequired error handler (logic moved into required handler).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/error-handlers/required.js

jdesrosiers · 2026-02-09T21:53:43Z

I'm curious, did you ask Copilot for a review, or did it happen automatically?

Sam-61s · 2026-02-09T22:35:55Z

Yep — that one’s on me
I had Copilot PR reviews enabled in my personal settings by accident. It was automatically triggered; I didn’t explicitly request it. I’ve disabled it now.

jdesrosiers · 2026-02-10T18:11:28Z

No worries. I just wanted to make sure it wasn't something new Github was doing that I needed to figure out how to turn off. I don't mind Copilot reviews, but only when it's asked for, not automatically. It can be helpful sometimes, but it really did a terrible job this time.

jdesrosiers · 2026-02-10T19:26:47Z

I just pushed a commit to show what I mean by matching the code style used in the project. I'll follow up with a review shortly.

jdesrosiers · 2026-02-10T19:32:32Z

src/error-handlers/required.js

+    }
+
+    const keyword = await getSchema(schemaLocation);
+    const dependencies = /** @type Record<string, string[] | object> */ (Schema.value(keyword));


Any time the value could be a schema (even if we're ignoring that case like we are here), you should be using the Schema AST functions to traverse the schema. In this case, you'll want to use Schema.entries(keyword) to iterate over the dependencies. Then when you know it's an array (Schema.typeOf), then you can get the value.

jdesrosiers · 2026-02-10T19:36:37Z

src/error-handlers/required.js

+  /** @type {string[]} */
+  const missingProperties = [...allMissingRequired].sort();
+  /** @type {string[]} */
+  const locations = [...allSchemaLocations].sort();


There shouldn't be any reason to sort these.

Sam-61s · 2026-02-11T04:29:28Z

I just pushed a commit to show what I mean by matching the code style used in the project. I'll follow up with a review shortly.

Thanks, I understand now. I’ll make sure to follow the project’s code style in future contributions.

jdesrosiers

Looks good. I just have a couple of small clean up things I'd like you to have a look at.

jdesrosiers · 2026-02-11T22:13:47Z

src/error-handlers/required.js

+      if (Schema.typeOf(dependencyNode) !== "array") {
+        continue;
+      }


This is depndentRequired. It's always an array. You don't need to check.

jdesrosiers · 2026-02-11T22:15:00Z

src/error-handlers/required.js

+      if (!Instance.has(propertyName, instance)) {
+        continue;
+      }
+
+      if (Schema.typeOf(dependency) !== "array") {
+        continue;
+      }


I think this can be combined to a single expression.

jdesrosiers · 2026-02-11T22:16:27Z

src/error-handlers/required.js

+  /** @type {string[]} */
+  const missingProperties = [...allMissingRequired];
+  /** @type {string[]} */
+  const locations = [...allSchemaLocations];


There doesn't seem to be any reason for these to be variables. They can be inlined without losing any functionality or clarity.

jdesrosiers · 2026-02-11T22:19:00Z

src/error-handlers/required.js

+      for (const requiredPropertyName of requiredProperties) {
+        if (!Instance.has(requiredPropertyName, instance)) {
+          allMissingRequired.add(requiredPropertyName);
+        }
+      }


This exact block is repeated for each keyword. Is there a better way we could handle this so there's less duplication? If not, that's ok. Sometimes duplication is worth the trade-offs. I just want to make sure we've considered those trade-offs.

jdesrosiers · 2026-02-12T20:50:54Z

src/error-handlers/required.js

+  /**
+   * @param {string[]} requiredProperties
+   * @param {import("@hyperjump/json-schema/instance/experimental").JsonNode} instance
+   * @param {Set<string>} missingSet
+   */
+  const addMissingProperties = (requiredProperties, instance, missingSet) => {
+    for (const propertyName of requiredProperties) {
+      if (!Instance.has(propertyName, instance)) {
+        missingSet.add(propertyName);
+      }
+    }
+  };


This isn't what I had in mind, but it's a good solution. Except, the function shouldn't be declared inside another function unless it needs to be a closure. So, let's move this out to the top level and let's put it at the bottom. I like the have help functions below the main code, not at the top.

jdesrosiers requested changes Feb 4, 2026

View reviewed changes

jdesrosiers requested changes Feb 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings February 7, 2026 01:41

Copilot started reviewing on behalf of Sam-61s February 7, 2026 01:42 View session

Copilot AI reviewed Feb 7, 2026

View reviewed changes

src/error-handlers/required.js Outdated Show resolved Hide resolved

Sam-61s force-pushed the feat/combine-required-message branch 2 times, most recently from 13273dd to 6b96e42 Compare February 8, 2026 05:24

jdesrosiers requested changes Feb 10, 2026

View reviewed changes

jdesrosiers reviewed Feb 11, 2026

View reviewed changes

Sam-61s force-pushed the feat/combine-required-message branch from e9ea194 to b29b3a2 Compare February 11, 2026 23:39

jdesrosiers requested changes Feb 12, 2026

View reviewed changes

Sam-61s and others added 8 commits February 13, 2026 10:18

combined required and dependentRequired error messages

7199125

unified error handler for each keyword

959669e

code style is fixed

b6d9376

Update code style

78699ad

Schema AST function is now used in all interations

f526987

clean up is done

705a19d

helper function is positioned at bottom now

a45ef84

Some cleanup

6f0d7a2

jdesrosiers force-pushed the feat/combine-required-message branch from e9ad103 to 6f0d7a2 Compare February 13, 2026 18:19

jdesrosiers approved these changes Feb 13, 2026

View reviewed changes

jdesrosiers merged commit e8dbcac into hyperjump-io:main Feb 13, 2026
1 check passed

Uh oh!

Conversation

Sam-61s commented Feb 3, 2026

Uh oh!

jdesrosiers left a comment

Choose a reason for hiding this comment

Uh oh!

Sam-61s commented Feb 4, 2026

Uh oh!

jdesrosiers commented Feb 4, 2026

Uh oh!

Sam-61s commented Feb 6, 2026

Uh oh!

jdesrosiers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

jdesrosiers commented Feb 9, 2026

Uh oh!

Sam-61s commented Feb 9, 2026

Uh oh!

jdesrosiers commented Feb 10, 2026

Uh oh!

jdesrosiers commented Feb 10, 2026

Uh oh!

jdesrosiers Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Sam-61s commented Feb 11, 2026

Uh oh!

jdesrosiers left a comment

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

jdesrosiers Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants