Add reload on empty or missing key

[andrewvk]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[Choc13]

I think we need to include a check here and only set Data to null when _source.IsOptional. Otherwise we'll be unsetting required data which I think is different to the design of the other configuration providers.

It's a bit of a pain because we'll end up duplicating some of the checking that happens in the initial DoLoad. This isn't a big problem, but one potential solution would be to make this method return bool to indicate whether it was able to update the Data or not and then in DoLoad throw if setting fails (because the data was missing and required).

[andrewvk]

If you look at the code, unlike FileConfigurationProvider, DoLoad is called only in case of initial loading. And there is a check for HasValue. And at reload another code is called. So the check inside SetData is redundant.

[andrewvk]

I will try to do some refactoring to simplify the code.

[Choc13]

Sorry, I'm not sure if my first comment was clear as I think we're both in agreement that DoLoad is only called once when it first loads and then PollingLoop is responsible for monitoring for changes and reloading.

What I was referring to is the fact that in PollingLoop we need to make sure that don't set Data to null if result.HasValue() is false and _source.IsOptional is false. So we need something similar to the code that's in DoLoad but it shouldn't throw in the case when the result doesn't have a value and it is required, it should just silently ignore this and leave the Data as it was.

[andrewvk]

OK. I think the best approach (in terms of code size and complexity) would be to just inline the SetData method and add null logic only to the PollingLoop version. This will eliminate unnecessary returns and checks.

[andrewvk]

Made the commit

[Choc13]

So yes I think inlining this is better, nice idea. I think we just need to make one more change here now to make this correct. I think it should be:

Data = result.HasValue()
    ? result.ToConfigDictionary(_source.ConvertConsulKVPairToConfig)
    : _source.Optional
        ? null
        : Data;

[andrewvk]

This is not the behavior that FileConfigurationProvider has. There, even with optional=false, deleting a file will remove the values from it in IConfiguration. And in this code, when the key is deleted, the values will remains.

[Choc13]

Ah right, that is the source of the confusion then. I was under the impression that old data should remain if it is not optional and is missing on a reload.

[Choc13]

One final observation from me. In FileConfigurationProvider it sets Data to an empty dictionary when the value is missing. We should probably do the same here.

[andrewvk]

OK

[andrewvk]

Another difference is that FIleConfigurationProvider uses the OrdinalIgnoreCase comparer:
new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
Should we do the same?

[Choc13]

Yes. We do actually create the dictionary with StringComparer.OrdinalIgnoreCase in the result.ToConfigDictionary extension method. So I don't think it matters too much what we do in the empty case as I don't the comparer will be used, but good to be consistent.

[Choc13]

In fact, if you call result.ToConfigDictionary with a result that doesn't have any data then it should already do the right thing and create an empty dictionary.

[Choc13]

Which means we can just write

Data = result.ToConfigDictionary(_source.ConvertConsulKVPairToConfig);

[Choc13]

LGTM. I'm not longer an admin on this repo, but I'll ask someone who is to kick off the build and then merge it if that passes. Thanks for taking the time to fix this.