Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
173 stars 122 forks source link

Should ctors of credentials throw exceptions during input validation or defer to first GetToken() call? #5750

Open ahsonkhan opened 2 months ago

ahsonkhan commented 2 months ago

Context: A, potentially implicit, design goal of our DefaultAzureCredential (and by extension ChainedTokenCredential) is to allow the end user to construct one without hitting exceptions, as part of a “good” getting started experience.

For example, we don’t expect this line to fail/throw an exception to disrupt the getting started flow/samples for end users. In C++:

auto credential = std::make_shared<Azure::Identity::DefaultAzureCredential>();

In .NET:

var credential = new DefaultAzureCredential();

The way we’ve solved this problem in C++ is defer throwing an AuthenticationException on the first GetToken() call. Within the various Credential constructors, we log error messages when doing input/environment validation, but don’t explicitly throw. We follow this pattern even for credentials that are explicitly not part of the DAC (as of today) such as AzurePipelinesCredential, to be consistent, and since they can still be included as part of the ChainTokenCredential. I want to make sure the design choice of how credentials validate and throw is consistent across languages, both as individual types, and as part of a DAC.

Open questions: 1) To make sure my assumption is sound: Is it a desired design goal to not throw an exception on the construction of DefaultAzureCredential as a good getting started experience? 2) How do other languages solve this design problem, if not deferring throwing outside the credential ctor? For example, rather than deferring throwing, do we throw an exception in each Credential ctor for input/environment validation, but catch those exceptions in the chain or DAC ctors? I imagine we’d want to be consistent here in behavior across languages.

LarryOsterman commented 2 months ago

My $.02: I'm a HUGE fan of validating inputs as early as possible, especially for simple situations.

That is especially true if the credential can used in a chained credential, where the failure may mask the customers preferred credential (the customer has credential A, B, and C to be used in that order, but because credential A was misconfigured, the client did not use their preferred credential but instead used a backup credential).

ahsonkhan commented 2 months ago

I tend to agree, and that's generally what we'd want in most of our types.

I just filed the issue to discuss with the v-team, and will add more context in the description, for what's motivating the question. Maybe that background would require more nuance.

Thanks for sharing your perspective Larry! You are too quick :)

LarryOsterman commented 2 months ago

For the specific case of credentials, IMHO the only reason they should throw during the GetToken() call is if there was a failure to retrieve the token.

The GetToken method should NEVER throw for input validation errors for the reason I mentioned above: Deferring the validation to GetToken will break customers.