Recursive/circular references approach

[maz808]

Hello! I'd like to tackle the implementation of recursive and circular references, so I've been trying to gather an understanding of how this library generates its schema models.

From my understanding the model classes are generated from information stored in instances of the ModelProperty class. This class also holds information for model properties that refer to other model classes. I'm thinking it would be a good idea to separate the first set of information out into another class called Model. My reasoning for this is that model properties created by reference currently depend on the existence of a model property defined in the schemas, which obviously doesn't work with recursive and circular referencing. I want to remove this dependency by creating Model instances from the schema definitions and ModelProperty instances from reference property definitions. The ModelProperty instances could then reference the Model instance they will be based on via string rather than via attrs.evolve like they currently are.

I'm not entirely sure what implications this could have on the rest of the code, but it seems to me like the two functionalities accomplished by the current ModelProperty class don't overlap, so this separation should be pretty clean. Please let me know if there's something I'm missing.

[dbanty]

Hey @maz808, thanks for offering to contribute this much-requested feature! Unfortunately the changes required to implement this feature end up being so large that it takes me more time to do a review cycle on them than I have available. I'm trying really hard not to be one of "those maintainers", but this would be the 4th (I think) attempt at implementing this, so I think it might have to be something I eventually implement myself. The worse news is that I have very little time to dedicate to this project right now, so I spend what I can trying to maximize the payoff for the time commitment—which means mostly answering questions and reviewing smaller PRs.

I'm trying not to be too negative here 😅 I just don't want you to spend a bunch of time working on this only for it to never merge. Multiple people have gotten burned out of this project from trying to achieve this, and I've gotten pretty burned out from reviewing the attempts.

Hopefully someday I can convince a company to do a targetted sponsorship for implementing this feature, and then I'll dedicate the time to do it one last time.

Just for reference, this is for issue #338 / #466 .

[maz808]

I meant like the first example, where a model refers to itself within its allOf section, even if there are other objects listed in the allOf. Like this:

ModelA:
  allOf:
    - $ref: "#/components/schemas/ModelA"
    - $ref: "#/components/schemas/ModelB"
    - type: object
      properties:
        name:
          type: string
ModelB:
  type: object
  properties:
    color:
      type: string

Another case I'm curious about is circular references in allOf sections:

ModelA:
  allOf:
    - $ref: "#/components/schemas/ModelB"
    - type: object
      properties:
        name:
          type: string
ModelB:
  allOf:
    - $ref: "#/components/schemas/ModelA"
    - type: object
      properties:
        color:
          type: string

This case would also be invalid. Right?

The reason I ask is to make sure I can rely on the current while making_progress loop for allOf references at the top level. allOf references for submodels may be a different story.

[dbanty]

Hmm, that's effectively making the two types the same—annoying but not invalid. If that particular case is hard to cover but you're able to cover other ones, something is still better than nothing. Just need to make sure we're emitting warnings when there are cases we can't handle yet.

Unfortunately I think that making progress loop is going to have to be replaced in order to solve all circumstances.

[maz808]

@dbanty I've completed my implementation on a branch off of my main branch. How should I go about submitting a PR? Should I merge it into my main and then submit a PR with my main or just submit directly from my recursive-references branch?

[dbanty]

Whatever works easiet for you. Which branch it's on in your fork won't impact the PR process at all.

[maz808]

Thank you! I've submitted the PR and will be keeping it up to date with main. If you get the time, let me know and we can discuss the code through whatever means you find appropriate.