findSchemaDefinition doesn't detect cyclic references

[MarttiR]

Prerequisites

• I have searched the existing issues
• I understand that providing a SSCCE example is tremendously useful to the maintainers.
• I have read the documentation
• Ideally, I'm providing a sample JSFiddle, Codesandbox.io or preferably a shared playground link demonstrating the issue.

What theme are you using?

core

Version

5.17.1

Current Behavior

Consider a schema that (accidentally!) has a self-referencing definition like so:

{
  "schema": {
    "definitions": {
      "oops": {
        "$ref": "#/definitions/oops",
      },
    },
  },
}

During validation, findSchemaDefinition enters a recursive loop until the runtime aborts with Maximum call stack size exceeded (Chrome) or function nested too deeply (Firefox).

Expected Behavior

findSchemaDefinition could detect that the "resolved" $ref is in fact the same $ref and throw, alerting developers to the issue in their schema.

Steps To Reproduce

No response

Environment

No response

Anything else?

No response

[heath-freenome]

@MarttiR Good catch. We've dealt with cyclic $refs before by stopping once a recursion has been detected. There is many examples of this in the retrieveSchema() functionality. Are you willing to push a PR that does the following refactor with the necessary tests? NOTE: you may need to fix bugs here :)

// Modify existing docs, with addition of recurseList and description of how it is used
export function findSchemaDefinitionRecurse<S extends StrictRJSFSchema = RJSFSchema>(
  $ref?: string,
  rootSchema: S = {} as S,
  childSchema: S,
  recurseList,
): S {
  if (recurseList.includes($ref)) {
    return childSchema;
  }
  ... // All of the existing code except changing the recursion call to:
     const subSchema = findSchemaDefinitionRecurse<S>(theRef, rootSchema, current, [...recurseList, theRef]);
  ... // The remaining code
}
// Use existing docs
export default function findSchemaDefinition<S extends StrictRJSFSchema = RJSFSchema>(
  $ref?: string,
  rootSchema: S = {} as S
): S {
  const recurseList: string[] = [];
  return findSchemaDefinition($ref, rootSchema, rootSchema, recurseList);
}

[MarttiR]

Sure, I can do it.

I think the appropriate fix is to throw an error when a cycle is detected, so that developers are alerted to the actual issue.

[heath-freenome]

@MarttiR Cyclic dependencies like this with $refs are actually valid. So rather than throwing, you'll want to stop recursing

[MarttiR]

@heath-freenome Thanks! To me, it feels these definitions are degenerate, as they are self-referential. Could you give an example of a schema where such definitions would be valid, so that I can understand how to form the test cases?

Edit: nevermind, I see there are some example cases in testData. I will look into it.

[MarttiR]

@heath-freenome It looks like we are talking about two different cases.

It is indeed valid for a property schema to have child properties defined as a reference to itself.

For example, a menu structure is a classic example of that pattern:

- title: "First level menu item"
  children:
      - title: "Second level menu item"
      - title: "Another second level menu item"
        children:
            - title: "Third level menu item"

and so on.

For such a thing, it makes sense to have a schema like

{
  "definitions": {
    "MenuItem": {
      "type": "object",
      "properties": {
        "title": {
          "type": "string"
        },
        "children": {
          "$ref": "#/definitions/MenuItem"
        }
      }
    }
  }
}

However, the issue here is not with schema resolution, but definition resolution.

The problematic case is a definition property that refers to itself either directly, or indirectly through a chain of other definitions.

{
  "definitions": {
    "SelfReferential": {
      "$ref": "#/definitions/SelfReferential"
    }
  }
}

The crucial difference is that the references are not in child properties. Rather, the definition has become tautological, and is likely either a degenerate result of a bad schema transformation, or simply a user error.

In my mind, my current PR is a minimal and sufficient fix for the issue:

First, findSchemaDefinition never tries to recurse into the properties of a found definition, so it doesn't need the same recursion tests as retrieveSchema: it always returns the first level only, for any schema.

Secondly, resolveAllReferences of retrieveSchema already uses findSchemaDefinition for looking up references, so it should also be covered by the tests added here.

[heath-freenome]

@MarttiR Ah... You raise a good point, so it's only the immediate self-recursion that you have to solve for? Or does the nested circular reference also pose the same issue?

[MarttiR]

@heath-freenome Only self-referential references (as in "ultimately ends up defining itself") are detected, as that's what causes the accidental baseless recursion here.

The "nested" case (as in "defines its children using its own definition") fixed earlier in retrieveSchema is never handled recursively by findSchemaDefinition – no actions needed here.