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

The BodyTopLevelProperties rule is mistakenly flagging paged responses #722

Open
markcowl opened this issue Jul 24, 2024 · 11 comments
Open
Assignees

Comments

@markcowl
Copy link
Member

Describe the bug
The BodyTopLevelProperties check is incorrectly flagging the response in List operations, which rturn a page of data as if they were resource GET operations. There are several examples in this PR: https://github.com/Azure/azure-rest-api-specs-pr/pull/18696
To Reproduce
Steps to reproduce the behavior:
Create list operations like: https://github.com/Azure/azure-rest-api-specs-pr/blob/a6bade603572212620a6cbb906f400e19d899b1b/specification/edge/resource-manager/Microsoft.Edge/configurationmanager/preview/2024-06-01-preview/configurationmanager.json#L62-L99

Expected behavior
This check should not be applied to list operations at all - in ARM specs, having an odd number of PATH segments and a last segment that is not a path variable should prevent the rule from being applied

Screenshots
See the link above.

Desktop (please complete the following information):
n/a, this occurs in CI

Additional context

@dhruvesh
Copy link

I am facing same issue: Azure/azure-rest-api-specs#29961

@sdorbala
Copy link

I'm facing the same issue on my PR https://github.com/Azure/azure-rest-api-specs-pr/pull/19036

@arjun-d-patel
Copy link

@pjhanwar
Copy link

@JoseLimaIVD
Copy link

olivertowers added a commit to Azure/azure-rest-api-specs that referenced this issue Jul 31, 2024
olivertowers added a commit to Azure/azure-rest-api-specs that referenced this issue Aug 7, 2024
* Initial 2024-06-01-preview changes

* Match breaking change fixes

* remove redundant file

* Compile changes

* style/linter fixes

* Add replica promotion and geo-replica scenario

* Linter/style fixes

* Fixes

* Fix: Spellcheck, ModelValidation, Advocado

* Fix promote async action, fix promote example, add reset password example.

* Add examples

* Fix examples

* Fix more examples

* prettier fixes

* Fix LintDiff warnings

* Fixes

* Fix example

* Update npm and redo tsv

* Changes

* address comments

* update scenario file

* Finalize basic scenario

* Add suppression for Azure/azure-openapi-validator#722

* Fix supression

* Fix typespec validations

* Override go SDK for MongoClusterStatus

* Convert to new AutoRest suppressions format.

* Update readme.md

---------

Co-authored-by: Roopesh Manda <[email protected]>
@rosychen427
Copy link
Member

dhananjay824 pushed a commit to dhananjay824/azure-rest-api-specs that referenced this issue Aug 9, 2024
* Initial 2024-06-01-preview changes

* Match breaking change fixes

* remove redundant file

* Compile changes

* style/linter fixes

* Add replica promotion and geo-replica scenario

* Linter/style fixes

* Fixes

* Fix: Spellcheck, ModelValidation, Advocado

* Fix promote async action, fix promote example, add reset password example.

* Add examples

* Fix examples

* Fix more examples

* prettier fixes

* Fix LintDiff warnings

* Fixes

* Fix example

* Update npm and redo tsv

* Changes

* address comments

* update scenario file

* Finalize basic scenario

* Add suppression for Azure/azure-openapi-validator#722

* Fix supression

* Fix typespec validations

* Override go SDK for MongoClusterStatus

* Convert to new AutoRest suppressions format.

* Update readme.md

---------

Co-authored-by: Roopesh Manda <[email protected]>
@arjun-d-patel
Copy link

Same issue here: Azure/azure-rest-api-specs#30271

@priyankarking
Copy link

I am facing same issue Azure/azure-rest-api-specs#30293

@priyankarking
Copy link

how to suppress this issue or make it optional ?

@mikeharder
Copy link
Member

Fixed by #729

@mikeharder mikeharder reopened this Sep 24, 2024
@mikeharder
Copy link
Member

Fix #729 requires the path after the last . to contain an even number of path segments:

export function isListOperationPath(path: string) {
if (path.includes(".")) {
// Get the portion of the api path to the right of the provider namespace by splitting the path by '.' and taking the last element
const splitNamespace = path.split(".")
if (path.includes("/")) {
const segments = splitNamespace[splitNamespace.length - 1].split("/")
// If the last segment split by '/' has even segments, then the operation is a list operation
if (segments.length % 2 == 0) {
return true
}
}
}

Which fixed cases like this:

https://github.com/Azure/azure-rest-api-specs-pr/blob/b928ddf9ff0b8a721fe51b8694276e2a864e2982/specification/edge/resource-manager/Microsoft.Edge/configurationmanager/preview/2024-06-01-preview/configurationmanager.json#L322

However, we have an example of a spec with an odd number of path segments instead:

https://github.com/Azure/azure-rest-api-specs-pr/pull/19621/files#diff-25d510d5d4b2927e4a5e839a6258baf6c9f7be821f48e3b7602383fb67647acfR125

Should the rule be fixed to allow this path? Or is the rule correct, and the spec should be updated to use an even number of path segments?

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

No branches or pull requests