oneOf tests for success-caused failures

[RitoG09]

Added test cases for oneOf for now focusing on edge cases where validation fails due to multiple successful matches.
Related to issue: #120
Planning to add similar tests for not and contains after review.

[jdesrosiers]

Thanks. I think that covers the basics. I've converted this to a draft because we can't commit it until we have an implementation or it breaks the test automation.

[jdesrosiers]

This description doesn't match the test. I don't see any nesting.

[RitoG09]

I've updated the description and added a not with nested schema.

[jdesrosiers]

I see no difference between this test and the one before it. The not schemas are different, but there's nothing fundamentally different about what it's testing.

[RitoG09]

sorry for late reply I was travelling, yes this one and the previous one is fundamentally same except the not schema used here is required.

[jdesrosiers]

This one's not quite correct. The oneOf will fail in this example, which means not will pass and there should be no errors. You'd have to remove either "a" or "b" from the instance to get a failure.

My first thought was that I expected there to be a oneOf node in the output. But, then I realized that might not make sense in this case. Let's explore this case a bit more. Try writing out the error message and let's see if we can come up with something that's helpful and computable.

[RitoG09]

Yes, I got it. I should change the instance to { "a": 1 } which makes oneOf pass and not fail but I think the oneOf node in the output will be too abstract. not does not care about oneOf internal logic. It only cares that the subschema matched. Error message can be something like "The value matches a schema that it should not".

"errors": [
    {
      "messageId": "not-message",
      "instanceLocation": "#",
      "schemaLocations": ["#/not"],
      "alternatives": [
        [
          {
            "messageId": "required-success",
            "messageParams": { "property": "a" },
            "instanceLocation": "#",
            "schemaLocations": ["#/not/oneOf/0/required"]
          }
        ]
      ]
    }
  ]

[jdesrosiers]

Yes, I think I agree. If oneOf passes, it means one of the schemas passed and it's effectively a simple applicator in that case.

Error message can be something like "The value matches a schema that it should not".

We can't say "matches a schema". That's the kind of thing we're trying to avoid with this project. The reader shouldn't have to know anything about schemas to understand what's wrong with the data.

We can probably come up with a message for not that works ok, but I wonder if it would be better to provide a negated message for each keyword. For example, not-required would be something like The '{property}' property should not be present. It would probably make for a better experience, but it makes the handling even more complicated and we still don't really have a great solution for success messaging. Adding negation messaging might not be that simple.

[RitoG09]

Ok, How i am thinking about the negated message is ->

let keywords optionally define a negated form of their message at the localization level, rather than introducing a global success or failure mode flag.

In this case:
current format: The '{property}' property is required.
what we need: The '{property}' property should not be present

Then not wouldn’t need to understand the internal logic of required or oneOf it would just delegate to the inner keyword and request its negated form when available. If a keyword doesn’t define a negated message, we could fall back to the generic not message plus structured alternatives.

So we are avoiding: requiredErrorHandler(normalizedErrors, instance, localization, { negated: true })
(it introduces the boolean flag)
Rather we can do: getRequiredNegatedMessage(property)
(let localization decide)

I’m not sure if this fully avoids complexity, but it seems more localized than introducing parallel success or negated handlers everywhere.

[jdesrosiers]

I don't think any of that helps us avoid needing to pass a negated flag around. I'm thinking we'll need some sort of getPassing analog of getErrors. Then, getPassing would have to have a negated flag that could be passed to each keyword so it can be passed to the Localization message function that decides which version of the message to send.

But I'm pretty sure that requires a separate pass and fail handler for each keyword. So, the thing we have to figure out is if we can make handlers be able to handle both pass and fail cases given a flag. That might not be be too hard, but applicator keywords might have some complications.

[RitoG09]

That makes sense.
Current flow -> jsonSchemaErrors → normalizedOutput → getErrors

Right now handlers only produce messages when the keyword fails. For example, requiredErrorHandler gathers missing properties and calls localization.getRequiredErrorMessage(...). If the keyword passes it simply returns [].
To support cases like not as you said we’d need something like a getPassing analog of getErrors that traverses the same normalized output but collects explanations for successful assertions instead of failures.
Code sample (conceptual):

async function getPassing(normalizedErrors, instance, localization, { negated = false }) {
  const passing = [];

  for (const handler of handlers) {
    const messages = await handler(normalizedErrors, instance, localization, {
      mode: "pass",
      negated
    });
    passing.push(...messages);
  }

  return passing;
}

Then handlers could support both modes:

const requiredHandler = async (normalizedErrors, instance, localization, { mode, negated }) => {
  const required = ["a"]; // simplified

  const present = required.filter((p) => Instance.has(p, instance));

  if (mode === "pass" && present.length) {
    if (negated) {
      return [{
        message: localization.getRequiredNegatedMessage(present),
        instanceLocation: Instance.uri(instance)
      }];
    }

    return [{
      message: localization.getRequiredSuccessMessage(present),
      instanceLocation: Instance.uri(instance)
    }];
  }

  if (mode === "fail") {
    // existing required error logic
  }

  return [];
};

And not could then call something like:
await getPassing(subschemaOutput, instance, localization, { negated: true });

And for applicator keywords as you have said it likely have more complications since they need to coordinate nested schemas, i would suggest to implement incrementally keyword by keyword starting with simple one to see how well the approach works before tackling applicators.