search

ExodusMovement / schemasafe

A reasonably safe JSON Schema validator with draft-04/06/07/2019-09/2020-12 support.
https://npmjs.com/@exodus/schemasafe
MIT License
155 stars 12 forks source link

chore: simplify tracing of known type and required in nested rules #179

Closed ChALkeR closed 7 months ago

ChALkeR commented 7 months ago

After #177 and #178.

E.g. in allOf/oneOf, we just rely on the checks in parent rule, but only for type/required checks.

In the following schema, the nested allOf will just check for { required: ['yyy'] }:

{
  type: 'object',
  required: ['xxx','zzz'],
  allOf: [{
    type: 'object',
    required: ['xxx', 'yyy']
  }]
}

Properties/items can't be inherited as they affect evaluation checks, which should be isolated from the parent.

The new if (definitelyType(...typearr)) return null line which replaced manual check in history is now able to catch more cases, which can be seen in generated code diffs, when we got type information from a ref (compare after #178 is landed, or just review d997a64024298510d279ca39b850da30064beb93).

This also will make #176 (nested discriminators) stop complaining with just a single type: 'object' check on the top level.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c87a7cc) 99.54% compared to head (8057cf2) 99.54%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files | [Files](https://app.codecov.io/gh/ExodusMovement/schemasafe/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/compile.js](https://app.codecov.io/gh/ExodusMovement/schemasafe/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbXBpbGUuanM=) | `99.29% <100.00%> (-0.01%)` | :arrow_down: |
mvayngrib commented 7 months ago

@kklash could u review? i can't seem to request a review via the UI

mvayngrib commented 7 months ago

@ChALkeR do we need to rebase + edit this test? https://github.com/ExodusMovement/schemasafe/pull/177/files#diff-6208744208cefe72d8f280336376b2302056b2d145203d67bd97c35f86ac9846R18

ChALkeR commented 7 months ago

@mvayngrib Done!

ChALkeR commented 7 months ago

This will still need a formal rebase after #177 and #178 land.

ChALkeR commented 7 months ago

This apparently now needs a rebase and re-reviews.

ChALkeR commented 7 months ago

Rebased

mvayngrib commented 7 months ago

This apparently now needs a rebase and re-reviews.

@ChALkeR maybe we want to disable that requirement?