Skip to content

Normalization format cleanup#192

Open
jdesrosiers wants to merge 1 commit intomainfrom
normalization-cleanup
Open

Normalization format cleanup#192
jdesrosiers wants to merge 1 commit intomainfrom
normalization-cleanup

Conversation

@jdesrosiers
Copy link
Collaborator

I wrote a @hyperjump/json-schema EvaluationPlugin to generate output directly in the normalized format we use in this project. The goal is to make using this with @hyperjump/json-schema more efficient. While doing that, I found a couple places where I think the format can be cleaned up a little. Please review the changes and let me know if you think there might be any issues. All the tests pass, but I'm not 100% sure there aren't edge cases we haven't thought of that might not pass.

  • json-schema-errors.js: This change removes empty nodes. I'm pretty sure this safe, but maybe there is some use for being able to easily get a list of all evaluated locations (using Object.keys)?
    {
      "#": {}, // <-- Removes this
      "#/foo": {
        "https://json-schema.org/keyword": {
          "https://example.com/main#/properties/foo/type": false
        }
      }
    }
  • if.js: The evaluation result of the if schemas is used to determine whether to evaluate then or else, but the keyword itself always reports true because it's ignored for determining if the schema passes. Therefore, if we process its schema, we could get it reporting that a keyword passes when it actually failed. It doesn't seem to matter either way. I wasn't able to come up with an example where that caused a bug, but it seems like the safe thing to do is to skip that schema evaluation when building the normalized output.
  • properties.js: This is the one I'm most unsure about, but I'm pretty sure it's safe. The original version includes a hack to include every declared property in the normalized output even if they aren't in the instance. At least with where we are now with the anyOf/oneOf work, that doesn't seem to be necessary. We only care about declared properties that are also in the instance. Hopefully, this doesn't end up being necessary, because it would require an uggly hack in the EvaluationPlugin to make it work.

@srivastava-diya
Copy link
Contributor

hey @jdesrosiers
the changes look really great and all the existing tests pass, but I think there might be a crash worth thinking about before merging.

The anyOf and oneOf error handlers both have this:

const typeErrors = alternative[instanceLocation]["https://json-schema.org/keyword/type"];

Previously this was safe because evaluateSchema always gave { [instanceLocation]: { } } in the output but with the empty node removal change, that is not assured . Have a look at this example

{
  $schema: "https://json-schema.org/draft/2020-12/schema",
  anyOf: [
    { properties: { foo: { type: "string" } } },
    { properties: { foo: { type: "boolean" } } }
  ]
}

const instance = { foo: 42 }; 

This gave:
Screenshot 2026-03-25 014244

The Minimal FIX that i can think of is adding optional chaining here in oneOf and anyOf error handlers:

const typeErrors = alternative[instanceLocation]?.["https://json-schema.org/keyword/type"];

@jdesrosiers
Copy link
Collaborator Author

Thanks, @srivastava-diya! I'll fix that.

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