feat: don't require full top-level validation in non-cyclic-non-top-level refs

[ChALkeR]

Refs: #161 (comment)

This will allow e.g. this to compile in strong mode:

  {
    $schema: 'https://json-schema.org/draft/2020-12/schema',
    allOf: [
      {
        $ref: '#/definitions/base',
      },
    ],
    definitions: {
      base: {
        required: [],
        properties: {
          version: {
            const: '2',
          },
        },
        type: 'object',
       //  unevaluatedProperties: false,
      },
    },
    properties: {
      type: {
        const: 'acknowledge',
      },
    },
    required: ['type'],
    type: 'object',
    unevaluatedProperties: false,
  }

[codecov-commenter]

Codecov Report

Merging #162 (3167a72) into master (287f3a8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Files Changed	Coverage Δ
src/compile.js	99.26% <100.00%> (+<0.01%)	⬆️

[ChALkeR]

@RichAyotte this should be ready now

[RichAyotte]

I've checkout the branch and ran a few tests and observed a few new behaviours.

When a $ref is specified without unevaluatedProperties, I get the following error: [requireValidation] additionalProperties or unevaluatedProperties should be specified at #/properties/bh1

      "properties": {
        "kind": {
          "const": "double_baking_evidence"
        },
        "bh1": {
          "$ref": "#/definitions/TezosBlockHeader"
        },
        "bh2": {
          "$ref": "#/definitions/TezosBlockHeader"
        }

Adding unevaluatedProperties to each item resolves the issue.

      "properties": {
        "kind": {
          "const": "double_baking_evidence"
        },
        "bh1": {
          "unevaluatedProperties": false,
          "$ref": "#/definitions/TezosBlockHeader"
        },
        "bh2": {
          "unevaluatedProperties": false,
          "$ref": "#/definitions/TezosBlockHeader"
        }
      }

The second new behaviour is with items. The errors are [requireValidation] items rule should be specified at #/oneOf/3/items and [requireValidation] items rule should be specified at #/oneOf/3/properties/args/items for the following:

    "MichelineMichelsonV1Expression": {
      "type": ["array", "object"],
      "oneOf": [
       ...// removed 3 irrelevant items here
        {
          "type": "array",
          "items": {
            "$ref": "#/definitions/MichelineMichelsonV1Expression"
          }
        },
        {
          "type": "object",
          "properties": {
            "prim": {
              "type": "string",
              "maxLength": 256,
              "format": "anyString"
            },
            "args": {
              "type": "array",
              "items": {
                "$ref": "#/definitions/MichelineMichelsonV1Expression"
              }
            },
            "annots": {
              "type": "array",
              "items": {
                "type": "string",
                "maxLength": 256,
                "format": "anyString"
              }
            }
          }
        }
      ]
    }

[RichAyotte]

I forgot to add, the changes here resolve issue #161 🚀

[ChALkeR]

Re: refs -- thanks!

Yes, this did change the behavior -- previously, all $refs were checked to be fully validating everything on their own, and now that check was removed, but together with corresponding assumption that $refs fully validate everything`.

I'll take a closer look at/investigate schemas that were previously compiling and now don't due to this.

[ChALkeR]

Yes, I see a regression on

validator(
  {
    $schema: 'http://json-schema.org/draft-07/schema#',
    $defs: {
      A: {
        type: ['array', 'object'],
        oneOf: [
          {
            type: 'array',
            items: {
              $ref: '#/$defs/A'
            }
          },
          {
            type: 'object',
            required: [],
            additionalProperties: false,
            properties: {}
          }
        ]
      }
    },
    $ref: '#/$defs/A'
  },
  { mode: 'strong' }
)

Looks like we need actual propagation for cyclic refs.

I assume that bh1/bh2 in the first example are also cyclic? Because there should be proper tracing for non-cyclic refs that works independent on the change here.

[RichAyotte]

I'm not sure what cyclic means here but here's the bh1/bh2 sample with more context.

    "TezosDoubleBakingEvidenceOperation": {
      "type": "object",
      "allOf": [{ "$ref": "#/definitions/TezosBaseOperation" }],
      "required": ["kind", "bh1", "bh2"],
      "properties": {
        "kind": {
          "const": "double_baking_evidence"
        },
        "bh1": {
          "unevaluatedProperties": false,
          "$ref": "#/definitions/TezosBlockHeader"
        },
        "bh2": {
          "unevaluatedProperties": false,
          "$ref": "#/definitions/TezosBlockHeader"
        }
      }
    },

[ChALkeR]

That doesn't look like a regression.

Perhaps we could enable the new behavior as an opt-in or only on non-cyclic refs to avoid regressions

[ChALkeR]

@RichAyotte updated

[RichAyotte]

Everything looks really good now and all my tests pass. Thanks for putting this patch together so quickly 🙏

[ChALkeR]

@RichAyotte I added more tests/safeguards and fixed an issue that detected.
This should be ready/final now.

[ChALkeR]

(lint fixes / code formatting only in the last pushes)

[RichAyotte]

Everything still looks good 👍

[ChALkeR]

Merged and published as @exodus/[email protected].

[ChALkeR]

Upd: use @exodus/[email protected] instead 🤦🏻 (I should have tested the unrelated commit before including it in the release)

[RichAyotte]

I'm glad you found it and I just tested with v1.2.2 and it works well.