microsoft / typespec

https://typespec.io/
MIT License
4.49k stars 213 forks source link

[Bug]: @discriminator and @encodedName interact badly #3507

Open willmtemple opened 5 months ago

willmtemple commented 5 months ago

Describe the bug

In the OpenAPI emitter (and likely in other emitter), incoherence can occur when a discriminator property is also annotated with @encodedName differently in each variant model of a union.

Discriminator validation only checks that a TypeSpec property with the given name exists in variant models, but does not validate that those properties have the same encoded name in JSON. This leads to OpenAPI output that names a discriminator property that does not actually exist.

This has an obvious problem in OpenAPI, and other emitters are likely to get this wrong without some library support if the discriminator property is renamed in some variants but not others, or is renamed differently in each variant.

Reproduction

https://typespec.io/playground?c=QGRpc2NyaW1pbmF0b3IoImtpbmQiKQp1bmlvbiBGb28gewogIGE6IEEsCiAgYjogQiwKfQoKbW9kZWwgQcUdQGVuY29kZWROYW1lKCJhcHBsaWNhdGlvbi9qc29uIiwgxVQyIikKICDECjogIkEiOwp9x0VC30XMRTHMRULFRQppbnRlcmZhY2UgRXhhbXBsZcVQb3AgZcYPKGluOuQAySnFBjsKfQ%3D%3D&e=%40typespec%2Fopenapi3&options=%7B%7D

Checklist

witemple-msft commented 5 months ago

After some discussion we've decided not to allow discriminator properties to have different encoded names within the same protocol at least -- maybe not allowing encoded names on discriminator properties at all.

Checking for this will have to be deferred. We don't guarantee that @discriminator runs before @encodedName, so we can't check this until an emitter requests the discriminator for a given union or model.

markcowl commented 4 months ago
timotheeguerin commented 3 months ago

Reopening disallowing both was preventing certain usage @odata.kind

timotheeguerin commented 3 months ago

We need to decide what to do, disallowing doesn't seems like a good option.

Options are:

  1. Make sure every property redefines the @encodedName: Accurate but tedious
  2. Only allow it for the inhertiance case but allow @encodedName to be resolved from the base model: Limiting
  3. Mix of both