Azure / oav

Tools for validating OpenAPI (Swagger) files.
MIT License
95 stars 54 forks source link

[INVALID_DISCRIMINATOR_TYPE] False positive if discriminator is optional $ref #1018

Closed mikeharder closed 10 months ago

mikeharder commented 10 months ago

As of 11/10/23, typespec was generating the following autorest:

"JobMatchingMode": {
  "properties": { "kind": { "$ref": "#/definitions/JobMatchingModeKind", } },
  "discriminator": "kind",
  "required": [ "kind" ]
},
  "JobMatchingModeCreateOrUpdate": {
  "properties": { "kind": {"$ref": "#/definitions/JobMatchingModeKind", } },
  "discriminator": "kind"
},
"JobMatchingModeKind": {
  "type": "string",
  "enum": ["queueAndMatch", "scheduleAndSuspend", "suspend"],

This caused two failures from OAV: DISCRIMINATOR_NOT_REQUIRED and INVALID_DISCRIMINATOR_TYPE. Example in spec PR: https://github.com/Azure/azure-rest-api-specs/pull/26658/checks?check_run_id=18572542508

The second failure is arguably a false positive, caused by the way oav processes optional ref types:

discriminatorProp: {
  "type":"string",
  "enum":["bestWorker","longestIdle","roundRobin"]
  ...
}

discriminatorProp: {
  "anyOf":[
    {"$ref":"_0#/definitions/DistributionModeKind"},
    {"type":"null","_skipError":true}
  ],
  "_skipError":true
}

The optional discriminator is an anyOf rather than a type, which causes this extra misleading error. You could argue it's a valid error, since types string and string | null can be different (in some languages). But I don't think rule INVALID_DISCRIMINATOR_TYPE fails if the discriminator is an optional literal string. So it's probably a bug related to optional $ref.

https://github.com/Azure/oav/blob/760a3352a04ebfcd2e9a1d05c7ad4925f20caa2c/lib/swaggerValidator/semanticValidator.ts#L358

In summary:

  1. If a discriminator is optional, rule DISCRIMINATOR_NOT_REQUIRED will fail.
  2. If a discriminator is optional and also a $ref, rule INVALID_DISCRIMINATOR_TYPE will also fail.
  3. If a discriminator is optional and a literal string, no additional rules fail (I believe).

Related: https://github.com/Azure/typespec-azure/issues/3833

mikeharder commented 10 months ago

Closing as won't fix, since this false positive only happens when DISCRIMINATOR_NOT_REQUIRED is already failing.