Repository navigation
Inconsistent exception handling when AddAzureAppConfiguration is invoked with optional=true #318
Description
My preference would be to fail no matter what for errors that come from invalid inputs, but that seems to contradict the documentation for the enabled flag.
My thoughts too. I think our "optional" behavior comes from the "optional" behavior of FileConfigurationProviders. For configuration files, this flag suppresses the exception when a file is not found. Anything else, like invalid syntax in file or empty file path still throws an exception when optional is true.
Along similar lines, if optional is true for AppConfig provider, any errors that occur during Build() should not be suppressed (these are always due to invalid inputs). However, any errors that occur during Load() should be suppressed.
Originally posted by Avani Gupta (@avanigupta) in #316 (comment)
Activity
zhenlan commentedon Apr 26, 2022
Just want to make sure this continues to work
https://docs.microsoft.com/en-us/azure/azure-app-configuration/concept-disaster-recovery
avanigupta commentedon Apr 26, 2022
The idea is to throw on user input errors. Since the disaster recovery setup kicks in when there's a runtime error, the above doc instructions should still work.
Eskibear commentedon Apr 26, 2023
It looks trivial by removing the check of optional here within
Build().As far I can tell,
FormatExceptionoccurs when we fail to parse the connection string for Endpoint part, andInvalidOperationExceptionoccurs when SDK fails to parse the connection string when constructing a client.I would create a PR to make the change, and probably add more test cases related to guard the behavior. Let me know if you have more context info or any concern.
Eskibear commentedon Apr 26, 2023
It makes total sense.
BTW, below is an idiot question and maybe a little off-topic.
While I was browsing the impl, I find that Build() and Load() are always called together by
builder.Build()given how we usually use the snippet below.AzureAppConfigurationSource.Build()is called here andAzureAppConfigurationProvider.Load()is called here when constructing ConfigurationRoot.Given the idea to fail early, for obvious error like wrong format in user input, my gut feeling is to parse and check as early as in
AddAzureAppConfiguration(..). Why do we allow user to add an obvious wrong config source? The parsing should not be expensive I guess. Or perhaps to stay consistent with other config providers. Anyway it should not be a big problem and I just curious about it given my limited knowledge about .NET.avanigupta commentedon Apr 26, 2023
Yan Zhang (@Eskibear) that's a valid point. We could fail earlier during
AddAzureAppConfigurationfor invalid connection strings. However, connection string parsing is not the only user input error we need to handle. There could be other user errors in setting upAzureAppConfigurationOptions, and we lazy initialize these options only if they are needed, ie., whenBuild()is invoked. So it makes more sense to validate the connection string duringBuildtoo.[-]Inconsistent exception handling when `AddAzueAppConfiguration` is invoked with `optional=true`[/-][+]Inconsistent exception handling when `AddAzureAppConfiguration` is invoked with `optional=true`[/+]