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.

Sign up for GitHub

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

Jump to bottom

[GetCollectionResponseSchema] Does not disambiguate paths that share the same "/providers" suffix #750

Open
mikeharder opened this issue Oct 16, 2024 · 3 comments · May be fixed by #765
Open

[GetCollectionResponseSchema] Does not disambiguate paths that share the same "/providers" suffix #750

mikeharder opened this issue Oct 16, 2024 · 3 comments · May be fixed by #765
Assignees
@bdefoy

Comments

@mikeharder
Copy link
Member

mikeharder commented Oct 16, 2024 •
edited
Loading

Overview

In the following PR, rule GetCollectionResponseSchema raised an error that appears to be a false positive, caused by two paths that share the same "/providers..." suffix.

Azure/azure-rest-api-specs#30656 (comment)

My first question is whether this scenario, where two paths in a spec only vary by a /subscriptions/{subscriptionId} prefix, but return different data types, should even be allowed?

@bdefoy, @rkmanda: What do you think?

If not, then the rule is technically correct to raise an error here, but the error message is misleading.

If so, then the rule will need to be updated to allow it. Inspecting the code, I believe the root cause is around here, since it only considers the path after the last index of "/providers". This code would need to be updated, to consider the prefix before this as well.

azure-openapi-validator/packages/rulesets/src/native/utilities/arm-helper.ts

Lines 508 to 516 in edb6559

const matchedPaths = allPathKeys.filter(
/**
* path may end with "/", so here we remove it
*/
(p) =>
p
.substr(p.lastIndexOf("/providers"))
.replace(/{[^/]+}/gi, "{}")
.replace(/\/$/gi, "") === possibleCollectionApiPath.replace(/{[^/]+}/gi, "{}"),

V1 APIs

/subscriptions/{subscriptionId}/providers/Microsoft.HybridCompute/locations/{location}/publishers/{publisher}/extensionTypes/{extensionType}/versions/{version}
operationId: ExtensionMetadata_Get
Response: #/definitions/ExtensionValue

/subscriptions/{subscriptionId}/providers/Microsoft.HybridCompute/locations/{location}/publishers/{publisher}/extensionTypes/{extensionType}/versions
operationId: ExtensionMetadata_List
Response: #/definitions/ExtensionValueListResult

    "ExtensionValueListResult": {
      "type": "object",
      "properties": {
        "value": {
          "type": "array",
          "readOnly": true,
          "items": {
            "$ref": "#/definitions/ExtensionValue"
          },

V2 APIs

/providers/Microsoft.HybridCompute/locations/{location}/publishers/{publisher}/extensionTypes/{extensionType}/versions/{version}
operationId: ExtensionMetadataV2_Get
Response: #/definitions/ExtensionValueV2

/providers/Microsoft.HybridCompute/locations/{location}/publishers/{publisher}/extensionTypes/{extensionType}/versions
operationId: ExtensionMetadataV2_List
Response: #/definitions/ExtensionValueListResultV2

    "ExtensionValueListResultV2": {
      "type": "object",
      "properties": {
        "value": {
          "type": "array",
          "readOnly": true,
          "items": {
            "$ref": "#/definitions/ExtensionValueV2"
          },
@mikeharder mikeharder self-assigned this Oct 16, 2024
@mikeharder mikeharder moved this from 🤔 Triage to 🐝 Dev in Azure SDK EngSys 🤖🧠 Oct 16, 2024
@github-project-automation github-project-automation bot moved this from 🐝 Dev to 🎊 Closed in Azure SDK EngSys 🤖🧠 Oct 17, 2024
@mikeharder mikeharder reopened this Oct 17, 2024
@github-project-automation github-project-automation bot moved this from 🎊 Closed to 🤔 Triage in Azure SDK EngSys 🤖🧠 Oct 17, 2024
@mikeharder mikeharder moved this from 🤔 Triage to 🐝 Dev in Azure SDK EngSys 🤖🧠 Oct 17, 2024
@mikeharder mikeharder changed the title [GetCollectionResponseSchema] Bugs detecting/reporting errors in APIs that vary only by subscription prefix [GetCollectionResponseSchema] Does not disambiguate paths that share the same "/providers" suffix Oct 17, 2024
@mikeharder mikeharder assigned bdefoy and unassigned bdefoy and mikeharder Oct 17, 2024
@mikeharder mikeharder mentioned this issue Oct 17, 2024
Closed
8 tasks
@bdefoy
Copy link
Member

bdefoy commented Jan 24, 2025 •
edited
Loading

From Azure/azure-rest-api-specs#30656 (comment):

Since the schema is different, the name of the Type must also be different within a namespace. The ARM Manifest would have a single entry for the type and doesnt document the scopes separately, so it makes even more sense to not allow the pattern where the definition varies at each scope.

So, the issue we want to fix is:

the rule is technically correct to raise an error here, but the error message is misleading.

@bdefoy
Copy link
Member

bdefoy commented Jan 25, 2025 •
edited
Loading

Since ARM considers the V1 and V2 endpoints the same resource type, the collection get and point get should have the same response schema across both V1 and V2. With that in mind, the rule is valid to flag here because the V2 point get response schema does not match the V1 collection get response (and vice versa with V1 point, V2 collection).

We can add another linter rule to specifically point out that endpoints which only differ by the scope (e.g. subscription vs. tenant scope) must have the same response schema. Explicitly showing an error for this might help clear up confusion in this scenario. This rule can also suggest using a scope parameter to consolidate the endpoints.

@bdefoy bdefoy linked a pull request Jan 25, 2025 that will close this issue
Add linter rule to suggest the scope parameter #765
Open
@rkmanda
Copy link
Member

rkmanda commented Jan 28, 2025

Agree with @bdefoy 's evaluation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bdefoy bdefoy

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mikeharder @rkmanda @bdefoy