[#174730128] Test cases for external definitions

[balanza]

This PR investigates how external definitions are resolved when an OpenAPI spec file is parsed by swagger-parser library before they result into generated typescript model definitions.
While using gen-api-models command in the wild is sometimes counter-intuitive how definitions will end in our generated codebase. The purpose of this is to provide a rationale for what's happening under the hood.

Example

For the sake of talking about near-real scenarios, we consider the following definition dependency graph:

dependency diagram

  +--------+      +---------+      +---------+
  | Person +----->| Address +----->| ZipCode |
  +--------+      +---------+      +---------+
       ^
       |
       +-------------+
                     |
  +------+      +----+---+
  | Book +----->| Author |
  +------+      +--------+

spec.yaml

 ----- operations are defined up here ------
definitions:
  Person:
    $ref: "definitions.yaml#/Person"
  Book:
    $ref: "definitions.yaml#/Book"  

definitions.yaml

Person:
  type: object
  properties:
    name:
      type: string
      description: |-
        name of the person
    address:
      $ref: "#/Address"
Address:
  type: object
  properties:
    location:
      type: string
    city:
      type: string
    zipCode:
      $ref: "#/ZipCode"
ZipCode:
  type: string
  pattern: '^\d{5}$'
Author:
    allOf:
      - type: object
        properties:
          isDead:
            type: boolean
      - $ref: "#/Person"
Book:
  type: object
  properties:
    title:
      type: string
      description: |-
        title of the book
    author:
      $ref: "#/Author"

Glossary

To better explain myself we need to be on the same page:

• internal definition is a definition inside the spec file. It can list its properties or reference a remote definition.
• remote definition is a definition which properties are defined in another file.
• transient definition is a remote definition which the spec file does not directly reference, still it is a dependency of another remote definition (being the latter either internal or transient itself). In the example: Address and ZipCode.
• root definition is a definition which has no parent. It must be an internal definition (that is: an internal definition might be root for a transient one, meaning the properties of the latter will be included in the properties of the root).

Rules

1. An internal definition will always result in a dedicated model file, which name is the name of the definition as it is listed in the spec. It is true both if the definition references a remote definition or not. In such case, the name of the definition in the referenced file is ignored (that is: the name of this definition in the spec is used) and the remote properties are "copied" as literal into the OpenAPIV2.SchemaObject (and consequently in the generated typescript model).
2. A transient definition will never result in a dedicated model file. Instead, its properties are resolved as literal values inside the OpenAPIV2.SchemaObject which represents its root definition. The name of the transient definition is lost in the parsed schema object.

Takeaways

• As for the example, only modules for Person and Book will be created; properties of Address and ZipCode will be included in Person's definition; Book's author field will contain a isDead field and a reference to Person.
• When an internal definition A references a remote definition A' which also references a remote definition B' which is refenced by an internal definition B, the OpenAPIV2.SchemaObject for A will only contain the name of B.
• When an internal definition A references a remote definition A' which also references a transient definition B, the OpenAPIV2.SchemaObject for A will only contain the properties of B, and the name B will be lost.
• It follows that, when in a chain of dependency of transient definitions we prefer to preserve the name of one of the definitions, we can just add an explicit reference to that in the spec file.

[pagopa-github-bot]

Affected stories

• ⚙️ #174730128: Gestire le definizioni esterne alle specifiche swagger durante la generazione dei tipi Typescript

Generated by 🚫 dangerJS

[codecov-commenter]

Codecov Report

Merging #197 into master will increase coverage by 23.81%.
The diff coverage is 70.66%.

@@             Coverage Diff             @@
##           master     #197       +/-   ##
===========================================
+ Coverage   46.85%   70.66%   +23.81%     
===========================================
  Files           1       10        +9     
  Lines         254      450      +196     
  Branches       81      153       +72     
===========================================
+ Hits          119      318      +199     
+ Misses        133      124        -9     
- Partials        2        8        +6     

Impacted Files	Coverage Δ
src/commands/gen-api-models/index.ts	14.28% <14.28%> (ø)
src/commands/gen-api-sdk/index.ts	30.00% <30.00%> (ø)
src/commands/bundle-api-spec/index.ts	33.33% <33.33%> (ø)
src/commands/gen-api-models/parse.ts	76.31% <76.31%> (ø)
src/lib/templating/filters.ts	77.77% <77.77%> (ø)
src/commands/gen-api-models/templateEnvironment.ts	81.48% <81.48%> (ø)
src/commands/gen-api-models/render.ts	93.90% <93.90%> (ø)
src/lib/templating/index.ts	95.23% <95.23%> (ø)
src/index.ts	100.00% <100.00%> (ø)
src/lib/utils.ts	100.00% <100.00%> (ø)
... and 10 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 49b2af2...fd73d2d. Read the comment docs.

[balanza]

In the end, is this a bug or a feature?

I think it's good enough for what we need at the moment, there are some caveats but the behaviour is pretty straightforward.
IMHO the optimum would be to have every transient definition to produce its own model file. To achieve that, we need to change the way we parse the specification file. More specifically, we may be able to configure swagger-parser library to do so. The challenge would be to handle name collisions once we import all external definitions, pretty hard to image.

I think it would be unnecessarily complicated for now.

[gunzip]

thank you for the in-depth analysis! AFAIK

we may be able to configure swagger-parser library

is an attempt I've made without success. I think we can proceeed as we're used to, probably we can add warnings (logs) in the parsing procedure (ie. "Type X looks like an external unresolved reference, add it to the specs if you want to resolve it inline") during gen-api-models

[balanza]

probably we can add warnings (logs) in the parsing procedure

I don't think it's feasible, the parsing mechanism happens in a black-box and we can tell nothing from the result.

[balanza]

Running e2e tests over the example definitions, I discovered a limitation on the generator.

When a transient definition is a composition (that is, it uses allOf combinator like this one), then the generator fails at creating the correct subtype. Indeed, it leaves an empty value that makes the model file unparsable, determining the generation script to crash (which is a good thing, at least).

As this is not crucial for the demonstration this PR is pursued to, I'd just choose another example. Therefore, I commit to provide another use-case to show such limitation.

[gunzip]

nice job! If I understand well, the same workaround will suffice even in the case of a composed transient definition (define the deps in the original doc)

[balanza]

nice job! If I understand well, the same workaround will suffice even in the case of a composed transient definition (define the deps in the original doc)

Not sure I got the question right but:
when you declare a transient definition in the original specification file, it becomes an internal definition. Hence the allOf limitation doesn't apply. In this sense, it is a possible workaround.