[Bug] [DownstreamApiOptions should be readable entierely from the configuration
jmprieur opened this issue · 1 comments
Which version of Microsoft Identity Abstractions for dotnet are you using?
4.1.0
Repro
DownstreamApiOptions downstreamApiOptions = new DownstreamApiOptions();
builder.Configuration.GetSection("DownstreamApi").Bind(downstreamApiOptions);
Expected behavior
no exception thrown, and the properties can be read from the configuration.
Actual behavior
- Exceptions are thrown if the "ProtocolScheme" is not set in the configuration
- The
HttpMethod
property cannot be read from the configuration (there is no converter from string to HttpMethod)
Possible solution
-
Change the
HttpMethod
property to be a string, instead of anHttpMethod
(same for the backing field_httpMethod
). This will be a breaking change (people will have to change their code to usehttpMethod.ToString()
, for instanceHttpMethod.Post.ToString()
. This impacts the public API:AuthorizationHeaderProviderOptions.HttpMethod
,AuthorizationHeaderProviderOptions._httpMethod
which need to move to be a stringDownstreamApiOptionsReadOnlyHttpMethod(DownstreamApiOptions options, HttpMethod httpMethod)
(the last parameter needs to be a string)DownstreamApiOptionsReadOnlyHttpMethod.HttpMethod
that needs to be a string too.- the tests (lines such as
options.HttpMethod = HttpMethod.Patch;
becomesoptions.HttpMethod = HttpMethod.Patch.ToString();
)
-
Remove the
throw new ArgumentNullException
in the properties that have a default value (ProtocolScheme
that should return "Get",HttpMethod
, that should return "Get"). Setting them to null (that is not setting them in the configuration), sets them to their default value.DefaultValue("Bearer")] public string ProtocolScheme { get { return _protocolScheme; } set { - _protocolScheme = string.IsNullOrEmpty(value) ? throw new ArgumentNullException(_protocolScheme) : value; + _protocolScheme = string.IsNullOrEmpty(value) ? "Bearer" : value; } }
This impacts tests as assigning null to these properties won't throw any longer (they will get a default value)
We'd need to move to Abstractions 5.0.0