Feature/add-properties-local-reference-support

[p1-ra]

Title

Feature/add-properties-local-reference-support

Description

This PR aims to bring support of properties local references resolution. It could happen that in a Document a local reference is pointing to one or multiple other virtual references instead to its property definition, as for exemple:

(...)
    ModelWithAnyJsonProperties:
      title: ModelWithAnyJsonProperties
      type: object
      additionalProperties:
        anyOf:
        - type: object
          additionalProperties:
            type: string
        - type: array
          items:
            type: string
        - type: string
        - type: number
        - type: integer
        - type: boolean
    ModelLocalReferenceLoop:
      "$ref": "#/components/schemas/ModelWithAnyJsonProperties"
    ModelDeeperLocalReferenceLoop:
      "$ref": "#/components/schemas/ModelLocalReferenceLoop"
 (...)

During properties parsing, those virtual reference are resolved by making them pointing to their target ModelProperty or EnumProperty definition.

Thus, only the target model will be generated and any path referencing them (the virtual reference) will use the target model.

Also add support for inner property direct and indirect reference to its parent model resolution, as:

(...)
AModel:
  title: AModel
  type: object
  properties:
    direct_ref_to_itself:
      $ref: '#/components/schemas/AModel'
    indirect_ref_to_itself:
      $ref: '#/components/schemas/AModelDeeperIndirectReference'
AModelIndirectReference:
  $ref: '#/components/schemas/AModel'
AModelDeeperIndirectReference:
  $ref: '#/components/schemas/AModelIndirectReference'
(...)

QA Notes & Product impact

Add support for properties local reference ($ref) resolution
Add support for inner property direct and indirect reference to its parent model resolution.

@dbanty

[codecov]

Codecov Report

Merging #329 (2e28978) into main (422e993) will decrease coverage by 0.06%.
The diff coverage is 99.32%.

❗ Current head 2e28978 differs from pull request most recent head a5d8be4. Consider uploading reports for the commit a5d8be4 to get more accurate results

@@             Coverage Diff             @@
##              main     #329      +/-   ##
===========================================
- Coverage   100.00%   99.93%   -0.07%     
===========================================
  Files           47       47              
  Lines         1410     1549     +139     
===========================================
+ Hits          1410     1548     +138     
- Misses           0        1       +1     

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	99.74% <99.32%> (-0.26%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 422e993...a5d8be4. Read the comment docs.

[p1-ra]

I started working on fixing #338 on this branch, wip

[p1-ra]

I've added the support for the resolution of inner property direct and indirect reference to its parent model. It resolve #338

The PR is ready for review @sp-schoen @emann

[sp-schoen]

@p1-ra This solves my bug, but it seems like a new one was introduced. Now I get the error Attempted to generate duplicate models with name "FooBar".
This occurs for all schemas where "FooBar" and "FooBar[]" is defined.

[p1-ra]

@p1-ra This solves my bug, but it seems like a new one was introduced. Now I get the error Attempted to generate duplicate models with name "FooBar".
This occurs for all schemas where "FooBar" and "FooBar[]" is defined.

Your issue seem to come from the string sanitizer function used to generate python friendly class name:
https://github.com/triaxtec/openapi-python-client/blob/422e993e35fc34cd295a63dfe88e5e346aa20ca6/openapi_python_client/utils.py#L37-L38

FooBar and FooBar[] will lead to FooBar as output which will create a naming collision. Not sure how to properly fix it, but you can definitly apply a tiny patch there for your specific use case

[dbanty]

Sorry I haven't gotten around to giving feedback on this yet. There are some other problems with references that need to be solved as well and I think the ultimate solution will end up being related.

Having just glanced through this code (so I may very well be missing something) I don't think we need lazy/late resolution to solve it. Similar to my findings in #312, there is a top level loop which will keep running and attempting to parse previously failed properties until it gets stuck. So when attempting to construct a model, if there's a reference that doesn't exist yet, it'll fail then but get retried later when, presumably, the reference has been registered.

I also successfully managed to remove most globals earlier in this project and would like to keep it that way since they have a nasty way of causing unexpected issues. Globals and exceptions 😅.

Anyway, my point is just that I haven't forgotten this issue & PR, but I think I need to do some bigger refactors & fixes related to references which will probably include fixing this elsewhere.

[p1-ra]

The two issues are solved by #366 , I'm closing this PR, thanks!

[p1-ra]

I'm re-opening it for tracking purpose. Looks I've mess up with my tests (Oopsy!) #366 do not resolve it.

[dbanty]

I'm going to close this just because it's pretty far behind main. We obviously want complete reference support at some point but I doubt this PR will be the end solution (I think a couple other related ones have come through since?)