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

Identify client APIs that return null for the Value of an operations returning Response<T> #37905

Open
christothes opened this issue Jul 28, 2023 · 0 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. MQ This issue is part of a "milestone of quality" initiative.
Milestone

Comments

@christothes
Copy link
Member

christothes commented Jul 28, 2023

related customer issue: #35475

We should either make the return value of HasValue accurate or fix APIs that return null for Value against guidance.

Notes taken from #35560:

  • I would feel better about this change if we knew the scope of its effects across our libraries. Is Azure.Storage.Queues the only library that ever returns null for T in Response<T>, or are there others?
    • With an inventory of libraries we would know (a) who could be affected by the behavioral change to Response<T>.HasValue, and (b) where we could go in and add "correct" APIs and hide the ones that violate intended usage of Reponse<T>.
    • This could also inform our understanding if we wanted to change the guidance we're thinking we'll add for Improve the Azure.Core docs around Response<T> and its various forms #37171
  • Since we don't have anything other than human PR review preventing people from implementing methods incorrectly, I'd love to see us have a clear understanding of what we can do to prevent this in the future. For example, we're generating the convenience layer more and more in our libraries - how can we know the service won't return null in a way that's unintended in these cases?
  • I think there is a risk of breaking customers by changing the implementation of Response<T>.HasValue to sometimes return false when it never did before. I agree it is unlikely that there is a customer dependency on this, but if someone had a loop that was checking HasValue and making decisions based on the presence of null, we would filter out items they were expecting and could change downstream behavior as a result. If the team decides the risk of this is sufficiently small, I will downgrade my concern to non-blocking, but wanted to make sure it was considered.
    • One option here would be to have libraries that needed to set Response<T>.Value to null use a different static method on Reponse, e.g. Response.FromNull() instead of Response.FromValue(), and have that new static method return a different subtype of Response<T> that changed the behavior of HasValue. I realize this implementation would have its own set of tradeoffs that may not be preferable to the current proposal.
@christothes christothes added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Jul 28, 2023
@christothes christothes self-assigned this Jul 28, 2023
@christothes christothes added this to the Backlog milestone Jul 28, 2023
@annelo-msft annelo-msft added the MQ This issue is part of a "milestone of quality" initiative. label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. MQ This issue is part of a "milestone of quality" initiative.
Projects
None yet
Development

No branches or pull requests

2 participants