[AvoidAdditionalProperties] Current implementation may be overly strict, or causing more noise than value

[arthurwang2021]

Describe the bug False alert for AvoidAdditionalProperties.

To Reproduce It marked the AdditionalProperties in our dictionary properties in the request body. Based on https://swagger.io/docs/specification/data-models/dictionaries/, additionalProperties is just part of the dictionary syntax, not the real property that named AdditionalProperties.

Expected behavior It should not raise the flag for our additionalproperties that defined in our dictionary property

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: windows
• Browser: edge
• Version [e.g. 22]

Additional context Add any other context about the problem here.

[markcowl]

To clarify, there are two scenarios we should support, from the reference above:

• additionalProperties with a clearly defined primitive value type( string, number, etc.)
• AdditionalProperties with a value type that references a well-defined schema.
• We should specifically disallow

  "additionalProperties": true

  "additionalProperties": {}

"additionalProperties": {
 "type": "object"
}

"additionalProperties": {
  "properties": {
    "prop1": "string"
  }
}

[mikeharder]

Rule AvoidAdditionalProperties has been causing a lot of confusion in spec PRs. Recent example:

https://teams.microsoft.com/l/message/19:0351f5f9404446e4b4fd4eaf2c27448d@thread.skype/1717121195498?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1717121195498&teamName=Azure%20SDK&channelName=API%20Spec%20Review&createdTime=1717121195498

We should review the history of the rule, both its intentions and the implementation, and compare to the alternate proposal suggested by Mark above.

[rkmanda]

For ARM APIs, we consider additionalProperties as an anti-pattern because it allows service teams to add or remove support for certain keys in the dictionary without any warning. The only usecase where we want to allow the pattern is when we are reviewers determine through a scenario discussion that the keys of the dictionary are user defined, the keys are not subject to any validation by the service and that the contents of the dictionary are a pass through for the control plane service. This kind of constraint cannot be specified in swagger, so we want this rule to be flagged as an error that intentionally forces a scenario discussion.

[mikeharder]

@rkmanda: Azure SDK will not make any functional changes to this rule until there is consensus with ARM.

I did make a recent change to this rule, to only report errors at the source, rather than everywhere the definition is referenced: https://github.com/Azure/azure-openapi-validator/pull/700

This should reduce noise while ensuring all instances of additionalProperties in the source code are flagged as errors.