Add SecretClient.GetSecret with RequestContext

[heaths]

Fixes #25125

[heaths]

@KrzysztofCwalina @tg-msft @pallavit I'm reluctant to smash this one into our upcoming release in a couple weeks. Initially when I spoke with @annelo-msft it seemed pretty straightforward. It was, actually, but as you can see required a lot of small changes. I tried to minimize the impact to code, but honestly a good refactoring of the KeyVaultPipeline (created when the Azure.Core pipelining was still in development) may be best. That said, I don't see any unnecessary allocations and should have minimized / eliminated any unnecessary stack pushes e.g., returning a value and not using it.

I'm open to pushing this into our upcoming GA, but leaning toward punting it till after to get more usage on a beta release. The issue has been open for 3 years, so another few months (or so) probably won't hurt.

/cc @vcolin7

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Security.KeyVault.Secrets

[pallavit]

I'm open to pushing this into our upcoming GA, but leaning toward punting it till after to get more usage on a beta release. The issue has been open for 3 years, so another few months (or so) probably won't hurt.

Agree with the approach.

[heaths]

I'm open to pushing this into our upcoming GA, but leaning toward punting it till after to get more usage on a beta release. The issue has been open for 3 years, so another few months (or so) probably won't hurt.

Agree with the approach.

@pallavit could you clarify: you agree with the idea to punt this till after the 7.4-related GA, or you agree with the concept in this PR (as @annelo-msft said in her PR review)?

[pallavit]

I agree with moving this out of the GA scope :)

[heaths]

I'll leave it as a draft for now then and can rebase after we GA.

[heaths]

@KrzysztofCwalina @tg-msft now that Storage has shipped with a RequestContext overload like I wanted here, can we revisit shipping with an overload and I can do a sample how to do this no-throw technique using the RC?

[heaths]

Per offline discussion with @KrzysztofCwalina, @tg-msft, and @m-nash I am working on changing this to an overload of GetSecret(..., RequestContext) but the desirable change may not be possible. Previously, @KrzysztofCwalina or @tg-msft indicated I should make the existing signature e.g., Response<KeyVaultSecret> GetSecret(string name, string version = null, CancellationToken cancellationToken = default) the all-required signature, similar to what we do for overloads any other time. This means making version required as well, which isn't a problem. However, this does mean that the typical caller e.g., GetSecret("secret-name") is now broken because the response type now changes to a NullableResponse<KeyVaultSecret> instread of the derived Response<KeyVaultSecret> i.e., Response<T> : NullableResponse<T>. This means that the new method can't return a NoValueResponse<KeyVaultSecret>.

I see three possible outcomes and am torn on the first two:

1. Leave this PR as-is at the time this comment was posted i.e., define a singularly purposed GetSecretIfExists. It's obvious but it serves one purpose.
2. Keep the existing signature as is and make the new convenience method taking a RequestContext use all-required parameters i.e., NullableResponse<KeyVaultSecret> GetSecret(string name, string version, RequestContect context). This provides more robust support for user scenarios without breaking existing code. Anyone who wants to use it has to pass in a RequestContext anyway. The downside is that this does not seem to fit the model @KrzysztofCwalina and @tg-msft want, but I wonder if it was considered that existing HLCs or hand-coded clients are likely returning a Response<T> so may have this same problem. NullableResponse<T> is newer.
3. Define a new method name - a "pseudo-overload" like GetSecretIfExists - that takes a RequestContext but has a different name e.g., GetSecretWithContext. This feels far afoul from what we want to accomplish long-term, though.

[github-actions]

Hi @heaths. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[heaths]

@KrzysztofCwalina @tg-msft @m-nash where do we stand on this?

I still feel like making customers use System.Text.Json or DynamicData just to use the power of a RequestContext is overkill. If we make Response<T> a NullableResponse<T>, we can still provide convenience methods that take/return explicit types as C# developers would be more comfortable with. If customers need the raw response, they can still get it with NullableResponse<T>.GetRawResponse().Content, yes?

EDIT: We got another request today: #36942

[m-nash]

@KrzysztofCwalina @tg-msft @m-nash where do we stand on this?

I still feel like making customers use System.Text.Json or DynamicData just to use the power of a RequestContext is overkill. If we make Response<T> a NullableResponse<T>, we can still provide convenience methods that take/return explicit types as C# developers would be more comfortable with. If customers need the raw response, they can still get it with NullableResponse<T>.GetRawResponse().Content, yes?

EDIT: We got another request today: #36942

I believe the explicit casts will be the answer here, but the exact public api for those isn't fully decided but hopefully by Azure.Core 34 we will be ready at which point we can follow that pattern here.

[github-actions]

Hi @heaths. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[vcolin7]

In progress.

[pallavit]

@heaths Has the story streamlined here? Can we tackle this as part of this or upcoming milestone?

[heaths]

Asked our architects and DPG driver for clarification. It seemed they were open to some variation of what I have here currently.

[github-actions]

Hi @heaths. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[github-actions]

Hi @heaths. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.