[typescript] Make additional properties access safer

[asmundg]

Instead of asserting that any key access returns a valid property, force
the consumer to check that the value is defined.

When declaring an object with additional properties

  /store/inventory:
    get:
      tags:
        - store
      summary: Returns pet inventories by status
      description: Returns a map of status codes to quantities
      operationId: getInventory
      produces:
        - application/json
      parameters: []
      responses:
        '200':
          description: successful operation
          schema:
            type: object
            additionalProperties:
              type: integer
              format: int32

The generated interface is unsafe, because it asserts that any property access on the response returns a value. In reality, a property access can return undefined, which will cause a runtime failure.

async getInventory(): Promise<{ [key: string]: number; }

If the compiler is running with strict null checking, we can avoid the problem by declaring that property access can return undefined, forcing the consumer to verity that they got an actual value:

async getInventory(): Promise<{ [key: string]: number | undefined; }>

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce @akehir @petejohansonxo

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[amakhrov]

@asmundg thanks for your PR!

Unfortunately this is a core issue in Typescript, which is currently not resolvable. E.g. see microsoft/TypeScript#23817 (which has a number of links to many related issues of the same kind).

A problem is that defining the values this way (| undefined) would negatively impact using this object in the contexts that only deal with existing object keys (for .. of, Object.values(), etc).

I don't believe it should be addressed in codegen at all. I would rather wait for the sufficient resolution in the Typescript compiler, or some community-wide workaround for that.

Any thoughts?

[asmundg]

Thanks @amakhrov,

You're right that this is a fundamental typescript problem. Changing this behaviour in codegen would be a breaking change.

While the current behaviour is fine if you always access the additional properties through Object.keys(), it's very bug prone if you're trying to access specific properties directly.

As we do have a mechanism for expressing that an object can have undefined properties, I'm not convinced that we unconditionally need to accept reduced safety of typescript's default behaviour.

Would it make sense to make the modified behaviour available behind a flag, so consumers can choose whether to enable stricter types (at the cost of superfluous checking when iterating over object properties)?

[amakhrov]

@asmundg introducing a flag for that sounds like a good idea. I believe many users can find the new behavior helpful - but probably not all of them.

Another consideration: this change doesn't have anything specific to the -fetch generator alone, right? Perhaps this logic (together with the flag driving it) should be defined in AbstractTypeScript codegen instead?

[asmundg]

@amakhrov good point about AbstractTypeScript. I'll take a look at making this shared and behind a flag.

[amakhrov]

A few more considerations (sorry, it didn't occur to me earlier):

• Since the new option is introduced for all Typescript codegens, you cleaned up TypescriptFetch codegen (getTypeDeclaration method). However, looks like TypeScriptReduxQuery and TypeScriptRxjs still have custom mapping that doesn't respect the new option. Probably needs the same clean up?
• I'm not too sure creating a new sample under /samples/client/petstore/typescript-fetch/builds/null-safe-additional-props just for this option makes sense. Maybe cover by a unit test instead? @macjohnny what do you think about that?

[macjohnny]

@amakhrov I agree a unit test for this "small" variation should be enough

[asmundg]

Thanks a lot for the feedback! This has been a bit of a monkey see, monkey do process on my part. :)

[macjohnny]

LGTM

[wing328]

@asmundg thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952