Circular References may be incorrectly handled

[TristanSpeakEasy]

In this document blob:https://docs.attentive.com/b6583e28-b7cf-40d0-a044-b6eef49eff3a

I get a circular reference error:

Circular reference detected: OrderProductModifier: Order -> OrderProduct -> OrderProductModifier -> OrderProductModifierCustomField [11250:31]

Looking into the document the actual circular reference seems to be Order -> OrderProduct -> OrderProductModifier -> OrderProductModifier through the modifiers field on line 11287

But it is listing OrderProductModifierCustomField as being where the circular reference is occurring but that is a simple object with no references in it.

Also from my understanding this is a "valid" circular reference as the modifiers field isn't a required field so the circular chain will break at some point (when that field is unset in the next level down from what we understand). So it won't infinitely be populated.

My understanding of this relationship between fields that cause circular references and required fields comes from this comment OAI/OpenAPI-Specification#822 (comment)

[daveshanley]

I can't access the attentive link, I get a 404

[TristanSpeakEasy]

@daveshanley make sure to include the blob: prefix

[TristanSpeakEasy]

On my second point as well, and after reading your blog post I could ignore some of the circular reference errors if I deemed them safe, but would be good to get some help with that from the lib, so in the case where the data won't be infinitely recursive would be good to tell it is good recursion vs bad

[daveshanley]

What am I doing wrong?

[daveshanley]

On my second point as well, and after reading your blog post I could ignore some of the circular reference errors if I deemed them safe, but would be good to get some help with that from the lib, so in the case where the data won't be infinitely recursive would be good to tell it is good recursion vs bad

What would you expect to see from the lib? What would you want to know? How would you want it to be exposed?

[TristanSpeakEasy]

apologies seems it is a short lived URL, grab it from here by clicking the download button https://docs.attentive.com/openapi/reference/overview/

What would you expect to see from the lib? What would you want to know? How would you want it to be exposed?

Maybe the recursive error that is returned contains a flag that is set to true when the recursion isn't infinite?

[daveshanley]

I'm not quite sure how to detect 'non-infinite' circles as the resolver will stop traversing the graph once it finds a single loop, it records the loop and gives up. It won't keep going - I am unsure how to stop it again unless we set some kind of hard loop limit.

As in, anything that loops more than x times is infinite. Thoughts?

[TristanSpeakEasy]

so the way I was doing this before leaning on libopenapi was detecting if the ref lives within a property, array, map etc and determining from its location whether it could be optional.

So if it was in a property check if the property is required or not.

If its an item of an array and minItems can be zero then an empty array could stop the recursion etc.

This is the way I understand this needs to be handled from the comment here OAI/OpenAPI-Specification#822 (comment)

[BenjaminNolan]

Hmm... We're unable to import this v2 spec for Shortcut, which contains the following threaded comment definition:

{"ThreadedComment": {
  "description": "Comments associated with Epic Discussions.",
  "type": "object",
  "properties": {
    // snipped irrelevant stuff
   "comments": {
      "description": "A nested array of threaded comments.",
      "type": "array",
      "items": {
        "$ref": "#/definitions/ThreadedComment"
      }
    }
    // snipped irrelevant stuff
  },
  "additionalProperties": false,
  "required": [
    // snipped irrelevant stuff
    "comments"
    // snipped irrelevant stuff
  ]
}}

I note they have incorrectly marked comments as required, however if I remove "comments" from the required list, I still get the circular references error, so my guess is that the code that checks for circular references when building the models doesn't check to see whether it's required or not before throwing the error. The current version of their spec should throw an error IMO, however with the field name removed from the required list, I think it should import.

[daveshanley]

The library isn't currently looking at anything other than references that loop, regardless of whether they are required. So adding some intelligence here to check that state should solve that. It's part of the resolver.

[BenjaminNolan]

As GitHub's noted above, I've opened a PR for an initial run at fixing this, with a couple of places for further improvements noted within the PR. Looking forward to your feedback! :)

[daveshanley]

Now that #64 has been merged in and released in v0.4.14 (thank you for your contribution @BenjaminNolan). We should be able to close off this issue.