Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
arthurwang2021 opened this issue Jan 30, 2024 · 4 comments

Comments

@arthurwang2021
Copy link

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.

@mikeharder mikeharder self-assigned this Jun 1, 2024
@mikeharder mikeharder removed their assignment Jun 1, 2024
@mikeharder mikeharder changed the title False alert for AvoidAdditionalProperties Allow well-typed dictionary types to contain additionalProperties Jun 1, 2024
@mikeharder mikeharder self-assigned this Jun 1, 2024
@mikeharder mikeharder moved this from 🤔 Triage to 🐝 Dev in Azure SDK EngSys 🚢🎉 Jun 1, 2024
@markcowl
Copy link
Member

markcowl commented Jun 1, 2024

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 mikeharder changed the title Allow well-typed dictionary types to contain additionalProperties [AvoidAdditionalProperties] Current implementation may be overly strict, or causing more noise than value Jun 1, 2024
@mikeharder
Copy link
Member

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.

@mikeharder mikeharder moved this from 🐝 Dev to 📋 Backlog in Azure SDK EngSys 🚢🎉 Jun 3, 2024
@rkmanda
Copy link
Member

rkmanda commented Jun 5, 2024

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 mikeharder removed their assignment Jun 5, 2024
@mikeharder mikeharder moved this from 📋 Backlog to 🤔 Triage in Azure SDK EngSys 🚢🎉 Jun 5, 2024
@mikeharder
Copy link
Member

mikeharder commented Jun 5, 2024

@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: #700

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

4 participants