Skip to content

filter anyOf and oneOf alternative#180

Open
animeshsahoo1 wants to merge 4 commits intohyperjump-io:mainfrom
animeshsahoo1:filter-anyOf-oneOf-alternatives
Open

filter anyOf and oneOf alternative#180
animeshsahoo1 wants to merge 4 commits intohyperjump-io:mainfrom
animeshsahoo1:filter-anyOf-oneOf-alternatives

Conversation

@animeshsahoo1
Copy link

The anyof and oneof error handlers now filter alternatives based on the instance's properties. For object instances, an alternative is excluded from the error report if none of its declared properties exist on the instance (Rule 1), or if none of the instance's properties pass validation in that alternative (Rule 2). For non-object instances, the original type-keyword filtering is done.

fixes #175

@animeshsahoo1
Copy link
Author

hi @jdesrosiers I have implemented what we discussed about, I tried to not add anything unnecessary but if there is need of changes let me know, I made sure to run the linting and tests as well.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Please add tests. They need to cover all the edge cases we discussed and more if you can think of them.

@animeshsahoo1
Copy link
Author

I did run it on tests we talk about but I was unsure if I had to add the code for that, I will add it soon.

@animeshsahoo1
Copy link
Author

I have added the test case for matchcount=0 for oneOf and anyOf test cases that were discussed, I tried to think of a failing case but didn't really find any let me know if I could add any other test case

@animeshsahoo1
Copy link
Author

@jdesrosiers tagging you in case you didn't get notification since I didn't tag last time

@jdesrosiers
Copy link
Collaborator

You don't need to tag me. I'll get the notification. In fact, bumping the issue will likely make it take longer before I get to it. I generally work on the most stale issue on my plate first. So, if you bump the issue, it effectively moves your issue to the back of the queue.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Please try to match the code style used in the rest of the project. Please turn off Prettier or at least give it a much more reasonable line length limit.

I think this works! But, getting it to work is just the first step. This needs some refactoring.

Every time you do a filter operation, you're looping over everything again and creating a new array. Creating a bunch of arrays and objects gets expensive at scale. See if you can refactor so that you can loop over the alternatives only one time.

Also try to avoid things like [...instanceProps]. Again, we're unnecessarily creating an array. Have a look at @hyperjump/pact. It allows you to use filter, map and bunch of other things that work on any iterator (not just arrays) and don't create unnecessary intermediate representations. It looks like it could come in handy in a few places.


/** @type (alternative: NormalizedOutput, propLocation: string) => boolean */
const propertyPasses = (alternative, propLocation) => {
const propOutput = alternative[propLocation];
Copy link
Collaborator

Choose a reason for hiding this comment

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

alernatative and propLocation aren't used individually. Should we just pass propOutput directly instead?

Comment on lines 60 to 72
filtered = filtered.filter((alternative) => {
const typeResults
= alternative[instanceLocation]?.[
"https://json-schema.org/keyword/type"
];
return (
!typeResults || Object.values(typeResults).every((isValid) => isValid)
);
});

if (alternatives.length === 0) {
for (const alternative of anyOf) {
alternatives.push(await getErrors(alternative, instance, localization));
if (filtered.length === 0) {
filtered = anyOf;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it right for this to be in an else? It seems to me that we should be doing this check always and doing it first. It's the most generic check and the best chance to filter out potential cases early.

Copy link
Author

Choose a reason for hiding this comment

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

yeah that's true I focused too much on object cases this should be done at start to filter out many cases

@animeshsahoo1
Copy link
Author

hi I have done the changes and used hyperjump pact to avoid creating intermediate representations, let me know if there is still somewhere I should change

@animeshsahoo1 animeshsahoo1 force-pushed the filter-anyOf-oneOf-alternatives branch from ba77800 to 803ece5 Compare March 22, 2026 22:46
@animeshsahoo1
Copy link
Author

Hi I have fixed the failing npm run lint command with new oxlint linting, please check if its ok.

Comment on lines +54 to 60
filtered = [];
for (const alternative of anyOf) {
alternatives.push(await getErrors(alternative, instance, localization));
const typeResults = alternative[instanceLocation]["https://json-schema.org/keyword/type"];
if (!typeResults || Object.values(typeResults).every((isValid) => isValid)) {
filtered.push(alternative);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is now duplicated code. You don't need the if/else. Remove the if wrapper and the else block entirely.

Copy link
Author

Choose a reason for hiding this comment

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

So basically remove if else wrapper put the type check at top for each alternative then if its an object we check rule1 and rule2 right

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Comment on lines +39 to +41
const declaredProps = Object.keys(alternative)
.filter((loc) => loc.startsWith(prefix))
.map((loc) => /** @type {string} */ (Pact.head(JsonPointer.pointerSegments(loc.slice(prefix.length - 1)))));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Pact here to avoid creating intermediate arrays.


/** @type (propOutput: NormalizedOutput[string] | undefined) => boolean */
const propertyPasses = (propOutput) => {
if (!propOutput || Object.keys(propOutput).length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of the check for an empty object and the tests don't cover it. Either remove it or add a test showing it's needed.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

I found a bug. Here's a test that exposes it.

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",

  "anyOf": [
    {
      "type": "object",
      "properties": {
        "a": { "type": "string" }
      },
      "required": ["a"]
    },
    {
      "type": "object",
      "properties": {
        "b": { "type": "string" }
      },
      "required": ["b"]
    }
  ]
}
{ "a": 42 }

The first alternative is getting filtered by the discriminator check (rule 2), but "a" is not a discriminator because it doesn't have a passing value in any other alternatives.

@animeshsahoo1
Copy link
Author

animeshsahoo1 commented Mar 25, 2026

yeah this is definitely an edge case I was thinking we should add to rule 2: only apply this filter if the failing properties all have passing constraints in at least one other alternative
this works on the previous example as well as the current example:

{
  "anyOf": [
    {
      "type": "object",
      "properties": {
        "type": { "const": "a" },
        "x": { "type": "string" }
      },
      "required": ["type", "x"]
    },
    {
      "type": "object",
      "properties": {
        "type": {
          "allOf": [
            { "type": "string" },
            { "minLength": 2 }
          ]
        },
        "x": { "type": "number" }
      },
      "required": ["type", "x"]
    }
  ]
}

Instance: { "type": "a", "x": 42 }
here both don't get filtered as suggested earlier and

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",

  "anyOf": [
    {
      "type": "object",
      "properties": {
        "a": { "type": "string" }
      },
      "required": ["a"]
    },
    {
      "type": "object",
      "properties": {
        "b": { "type": "string" }
      },
      "required": ["b"]
    }
  ]
}

instance: { "a": 42 } this also works

so the new rule2 is:
Filter out an alternative if no instance property passes in it, AND all of its failing properties have a passing constraint in at least one other alternative. I wanna look at its edge case a bit more would love your views too.

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.

Filter anyOf/oneOf object alternative results by the discriminator

2 participants