fix!: Change reference resolution to use reference path instead of class name (fixes #342) #366

dbanty · 2021-03-27T22:40:34Z

This one touches a lot of code and has multiple breaking changes:

When looking up a reference, it will be found via reference path (e.g. #/components/schemas/whatever) instead of the generated class name. This fixes broken behavior where we were failing to find the right references sometimes but also could change generated clients unexpectedly on people.
Config was reworked to rely on fewer globals (there's still at least one 😢). The --config param should be passed in after the command (e.g. generate) now instead of before it. Class name and module name overrides may apply differently than they used to but are hopefully more deterministic now.

…n errors

codecov · 2021-04-04T16:53:44Z

Codecov Report

Merging #366 (e131d13) into main (bf575fb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #366   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        46    -1     
  Lines         1490      1508   +18     
=========================================
+ Hits          1490      1508   +18

Impacted Files	Coverage Δ
openapi_python_client/__init__.py	`100.00% <100.00%> (ø)`
openapi_python_client/cli.py	`100.00% <100.00%> (ø)`
openapi_python_client/config.py	`100.00% <100.00%> (ø)`
openapi_python_client/parser/__init__.py	`100.00% <100.00%> (ø)`
openapi_python_client/parser/errors.py	`100.00% <100.00%> (ø)`
openapi_python_client/parser/openapi.py	`100.00% <100.00%> (ø)`
...penapi_python_client/parser/properties/__init__.py	`100.00% <100.00%> (ø)`
...i_python_client/parser/properties/enum_property.py	`100.00% <100.00%> (ø)`
..._python_client/parser/properties/model_property.py	`100.00% <100.00%> (ø)`
openapi_python_client/parser/properties/schemas.py	`100.00% <100.00%> (ø)`
... and 1 more

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 bf575fb...e131d13. Read the comment docs.

…s,-when-components-title-is-different-from-the-name

dbanty · 2021-04-04T17:29:06Z

I think this is finally ready for review! @dongfangtianyu originally reported the error, so if you could do a review pass (or at least test the new code with your OpenAPI documents) that would be super helpful. As always, I'd also like @packyg / @dtkav / @forest-benchling to take a look as well since they have the largest collection of documents and generated clients that I know of (and since this will probably mess with their fork).

dbanty · 2021-04-04T17:31:18Z

Oh and also @p1-ra and @p1-alexandrevaney are probably interested since you have a few other PRs open related to references.

forest-benchling

Thanks for cc-ing us. This looks like a good change and I don't think it will break our use case. We don't use title on any models that are referenced by other models, and we don't use any config in generation.

cc @packyg @dbanty @bowenwr

openapi_python_client/config.py

openapi_python_client/parser/properties/model_property.py

forest-benchling · 2021-04-05T13:27:30Z

openapi_python_client/parser/properties/schemas.py

+
+@attr.s(auto_attribs=True, frozen=True)
+class Class:
+    """ Info about a generated class which will be in models """


I know what you mean here, but this docstring is a little confusingly worded

Do you have a suggestion? Maybe "Info about a class to be generated"?

Maybe "Represents Python class which will be generated from OpenAPI schema"?

openapi_python_client/parser/properties/schemas.py

dongfangtianyu · 2021-04-06T08:03:22Z

It looks great !
I have tested my OpenAPI file and everything is OK.

emann

LGTM - @p1-ra I'd be interested to hear if this fixes both issues you were trying to solve in #329. I would also imagine adding support for remote references will be far easier now.

…s,-when-components-title-is-different-from-the-name

p1-ra · 2021-05-05T10:10:09Z

Kind of old, but I've missed the notification. As for information, regarding #329 it almost fixes them all (tested on today main branch):

Model property indirect reference: OK

openapi: 3.0.2
info:
  title: Model property Indirect reference generation test
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithIndirectReferenceProperty:
      title: AModelWithIndirectReferenceProperty
      type: object
      properties:
        an_enum_indirect_ref:
          ref: "#/components/schemas/AnEnumIndirectReference"
    AnEnumIndirectReference:
      ref: "#/components/schemas/AnEnum"
    AnEnum:
      title: "AnEnum"
      enum:
        - FIRST_VALUE
        - SECOND_VALUE

Model property deeper indirect reference: OK

openapi: 3.0.2
info:
  title: Model property deeper Indirect reference generation test
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithIndirectReferenceProperty:
      title: AModelWithIndirectReferenceProperty
      type: object
      properties:
        an_enum_deep_indirect_ref:
          ref: "#/components/schemas/AnEnumDeeperIndirectReference"
    AnEnumDeeperIndirectReference:
      ref: "#/components/schemas/AnEnumIndirectReference"
    AnEnumIndirectReference:
      ref: "#/components/schemas/AnEnum"
    AnEnum:
      title: "AnEnum"
      enum:
        - FIRST_VALUE
        - SECOND_VALUE

Model property self reference: OK

openapi: 3.0.2
info:
  title: Model property self reference generation test
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithSelfReferenceProperty:
      title: AModelWithIndirectReferenceProperty
      type: object
      properties:
        self_ref:
          ref: "#/components/schemas/AModelWithSelfReferenceProperty"

Model property self indirect reference: NOK

openapi: 3.0.2
info:
  title: Model property self indirect reference generation test
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithSelfReferenceProperty:
      title: AModelWithIndirectReferenceProperty
      type: object
      properties:
        self_ref:
          ref: "#/components/schemas/AModelWithSelfReferencePropertyIndirectRef"
    AModelWithSelfReferencePropertyIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferenceProperty"

Model property self deeper indirect reference: NOK

openapi: 3.0.2
info:
  title: Model property self indirect reference generation test
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithSelfReferenceProperty:
      title: AModelWithIndirectReferenceProperty
      type: object
      properties:
        self_ref:
          ref: "#/components/schemas/AModelWithSelfReferencePropertyDeeperIndirectRef"
    AModelWithSelfReferencePropertyDeeperIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferencePropertyIndirectRef"
    AModelWithSelfReferencePropertyIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferenceProperty"

@emann @dbanty

p1-ra · 2021-05-05T10:19:33Z

By removing the title attribute of the two failure specs (who was invalid title attribute I thinks), it works as expected:

Model property self indirect reference: OK

openapi: 3.0.2
info:
  title: Model property self indirect reference generation test v2 -- no title
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithSelfReferenceProperty:
      type: object
      properties:
        self_ref:
          ref: "#/components/schemas/AModelWithSelfReferencePropertyIndirectRef"
    AModelWithSelfReferencePropertyIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferenceProperty"

Model property self` deeper indirect reference: OK

openapi: 3.0.2
info:
  title: Model property self indirect reference generation test v2 -- no title
  description: One bug to solves them all
  version: 9001

paths: {}

components:
  schemas:
    AModelWithSelfReferenceProperty:
      type: object
      properties:
        self_ref:
          ref: "#/components/schemas/AModelWithSelfReferencePropertyDeeperIndirectRef"
    AModelWithSelfReferencePropertyDeeperIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferencePropertyIndirectRef"
    AModelWithSelfReferencePropertyIndirectRef:
      ref: "#/components/schemas/AModelWithSelfReferenceProperty"

emann · 2021-05-05T13:33:54Z

Thanks for getting back to us @p1-ra! Glad to hear its working :)

dbanty added 5 commits March 23, 2021 18:59

refactorWIP refactor reference / class name handling to fix resolutio…

b58fe09

…n errors

refactorRevert field_prefix refactor, continue class name refactor [WIP]

da8a09e

test: Fix all the type errors and start fixing unit tests

ff8bdb9

test: Fix more broken tests

2bbbed6

test: Fix more unit tests

10163b2

dbanty mentioned this pull request Mar 31, 2021

Support references to non-object types #371

Closed

dbanty added 2 commits April 4, 2021 10:36

test: Finish fixing broken unit tests

af8d0ea

test: Fix errors found by E2E test

503a7a6

dbanty added 4 commits April 4, 2021 10:58

docs: Update usage.md

0648aa0

test: Add more unit test coverage

5005d82

Merge branch 'main' into 342-could-not-find-reference-in-parsed-model…

7f9051b

…s,-when-components-title-is-different-from-the-name

fix: nullable passthrough in single union refs

fed62b0

dbanty marked this pull request as ready for review April 4, 2021 17:25

dbanty requested a review from emann April 4, 2021 17:25

dbanty added this to the 0.9.0 milestone Apr 4, 2021

forest-benchling previously approved these changes Apr 5, 2021

View reviewed changes

dbanty commented Apr 5, 2021

View reviewed changes

openapi_python_client/parser/properties/schemas.py Outdated Show resolved Hide resolved

dbanty dismissed forest-benchling’s stale review via d457227 April 5, 2021 14:20

docs: Improve properties.Class docstring

d457227

emann previously approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'main' into 342-could-not-find-reference-in-parsed-model…

e131d13

…s,-when-components-title-is-different-from-the-name

dbanty dismissed emann’s stale review via e131d13 April 7, 2021 13:51

dbanty requested a review from emann April 7, 2021 13:51

dbanty merged commit b719316 into main Apr 12, 2021

dbanty deleted the 342-could-not-find-reference-in-parsed-models,-when-components-title-is-different-from-the-name branch April 12, 2021 14:51

p1-ra mentioned this pull request May 5, 2021

Feature/add-properties-local-reference-support #329

Closed

jkinkead mentioned this pull request May 5, 2021

Add support for recursively defined schemas #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: Change reference resolution to use reference path instead of class name (fixes #342) #366

fix!: Change reference resolution to use reference path instead of class name (fixes #342) #366

dbanty commented Mar 27, 2021

codecov bot commented Apr 4, 2021 •

edited

Loading

dbanty commented Apr 4, 2021

dbanty commented Apr 4, 2021

forest-benchling left a comment

forest-benchling Apr 5, 2021

dbanty Apr 5, 2021

forest-benchling Apr 5, 2021

dongfangtianyu commented Apr 6, 2021

emann left a comment

p1-ra commented May 5, 2021 •

edited

Loading

p1-ra commented May 5, 2021 •

edited

Loading

emann commented May 5, 2021

fix!: Change reference resolution to use reference path instead of class name (fixes #342) #366

fix!: Change reference resolution to use reference path instead of class name (fixes #342) #366

Conversation

dbanty commented Mar 27, 2021

codecov bot commented Apr 4, 2021 • edited Loading

Codecov Report

dbanty commented Apr 4, 2021

dbanty commented Apr 4, 2021

forest-benchling left a comment

Choose a reason for hiding this comment

forest-benchling Apr 5, 2021

Choose a reason for hiding this comment

dbanty Apr 5, 2021

Choose a reason for hiding this comment

forest-benchling Apr 5, 2021

Choose a reason for hiding this comment

dongfangtianyu commented Apr 6, 2021

emann left a comment

Choose a reason for hiding this comment

p1-ra commented May 5, 2021 • edited Loading

p1-ra commented May 5, 2021 • edited Loading

emann commented May 5, 2021

codecov bot commented Apr 4, 2021 •

edited

Loading

p1-ra commented May 5, 2021 •

edited

Loading

p1-ra commented May 5, 2021 •

edited

Loading