Return an empty array instead of null to indicate successful response but empty result#415
Return an empty array instead of null to indicate successful response but empty result#415BrianCollet wants to merge 3 commits into
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
|
I appreciate the time you put into this. Just a heads up that we have a PR close to being merged that will change the folder structure. I recommend waiting for that to merge, then rebase, and then update your PR. That would be easiest for you. #403 |
Happy to contribute! I'll take another crack at it when the changes are merged. 😃 |
|
@BrianCollet - Can you now have a look? |
|
@BrianCollet - Do you still want to contrib? |
|
@jongio yes, my latest push on July 7th was after rebasing |
|
Thanks, we'll get the test issue fixed. |
|
/azp run azure - mcp |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@BrianCollet Our big repo change just happened. You'll need to rebase your changes on top the new repo structure. I can review once it's ready :) |
|
Sorry, @BrianCollet we are moving fast with big changes. Can you open a new PR for this? |
There was a problem hiding this comment.
Pull Request Overview
This PR changes the behavior of list commands to return empty arrays instead of null when no results are found, ensuring a consistent API response structure for successful operations. This improves API predictability by guaranteeing that the results field is always present with a valid structure when the operation succeeds.
Key changes:
- Modified all list commands to always return a populated results object containing empty collections instead of null
- Updated corresponding unit tests to validate empty arrays rather than null values
- Renamed test methods to reflect the new behavior
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
Multiple *ListCommand.cs files |
Removed conditional logic that returned null for empty results, now always return structured response objects |
Multiple *ListCommandTests.cs files |
Updated test expectations from null to empty arrays and improved test structure with consistent field initialization |
GroupJsonContext.cs |
Updated JSON serialization context to use renamed result class |
| private sealed class SubscriptionListResult | ||
| { | ||
| [JsonPropertyName("subscriptions")] | ||
| public List<string>? Subscriptions { get; set; } |
There was a problem hiding this comment.
This change is missing an entry in CHANGELOG.md. According to the coding guidelines, all changes should be documented in the CHANGELOG.md file in the appropriate release and section.
| "--subscription", _knownSubscriptionId, | ||
| "--vault", _knownVaultName, |
There was a problem hiding this comment.
Duplicate arguments are being passed to the parser. Lines 56-59 contain duplicate '--vault' and '--subscription' parameters, which may cause parsing errors.
| "--subscription", _knownSubscriptionId, | |
| "--vault", _knownVaultName, |
| "--subscription", _knownSubscriptionId, | ||
| "--vault", _knownVaultName, |
There was a problem hiding this comment.
Duplicate arguments are being passed to the parser. Lines 83-86 contain duplicate '--vault' and '--subscription' parameters, which may cause parsing errors.
| "--subscription", _knownSubscriptionId, | |
| "--vault", _knownVaultName, |
| "--subscription", _knownSubscriptionId, | ||
| "--vault", _knownVaultName, |
There was a problem hiding this comment.
Duplicate arguments are being passed to the parser. Lines 114-117 contain duplicate '--vault' and '--subscription' parameters, which may cause parsing errors.
| "--subscription", _knownSubscriptionId, | |
| "--vault", _knownVaultName, |
|
@anuchandy - Can you review this? |
anuchandy
left a comment
There was a problem hiding this comment.
Hi @BrianCollet, Thanks a lot for the contribution! Is it possible to scope the PR changes to only about returning [] array (instead of null) and test for that specific case?
There appear to be several formatting changes across tests, as well as new variables, logic and deserialization of results. Can those be reverted so that only the tests previously checking for null result are updated to check for an empty array? To keep things straightforward, we can simply compare against "[]" rather than deserializing.
If reverting is too much work due to the number of files, as Jon suggested, you can considering opening a new PR based of latest main focused only on the empty array.
|
@anuchandy yeah I'll close this PR and open a new one focusing only on the empty array. Thanks! |
thanks @BrianCollet looking forward to it! |
What does this PR do?
Returns empty array instead of omitting results field for successful responses
Updated tests to check for empty array instead of null
GitHub issue number?
Fixes #382
Checklist before merging
/azp run azure - mcpto start the run