Values are not reloaded when a key is deleted

[andrewvk]

If the configuration key is deleted in Consul or even set to an empty string, the configuration is not reloaded, it still has the old value. This behavior is explicitly specified in the code, and I can't figure out the reason for it.

[Choc13]

Hi @andrewvk could you point me to the bit in the code where it's explicitly specified? I've not looked at this library for a while so I'm a bit rusty, but if you point me to it I should be able to remember why that was done that way. It doesn't sound a like a design decision that was consciously made, but maybe it's just my foggy memory.

[andrewvk]

https://github.com/wintoncode/Winton.Extensions.Configuration.Consul/blob/d9705de56f83572fc96d5e880ae24301da92cb18/src/Winton.Extensions.Configuration.Consul/ConsulConfigurationProvider.cs#L137 result.HasValue returns false if the key is deleted or contains an empty string.

[Choc13]

Ah OK, I remember now. This wasn't exactly deliberate as just being defensive about avoiding a NullReferenceException. Clearly HasValue() should be telling us to remove the key from the local configuration if it is false. Do you want to have a go at a PR for this?

Just off the top of my head I imagine we'll want something along these lines:

if (result.LastIndex > _lastIndex)
{
    if (SetData(result))
    {
        OnReload();
    }
}

and then SetData can probably become something like

private bool SetData(QueryResult<KVPair[]> result)
{
    if (result.HasValue())
    {
        Data = result.ToConfigDictionary(_source.ConvertConsulKVPairToConfig);
        return true;
    }

    if (!_source.Optional)
    {
        return false;
    }

    Data = null;
    return true;
}

So that in DoLoad we can write:

if (!SetData(result))
{
    throw new Exception($"The configuration for key {_source.Key} was not found and is not optional.");
}

It's a bit tricky because we need to decide what to do when result.HasValue() is false and _source.Optional == false too (i.e. when SetData returns false) in the case when we're in the polling loop. Raising an exception doesn't feel correct in all circumstances because some applications might want to continue with the old value, but I can also see why some other applications would want to know about this and potentially take some different action.

One option is to extend the _source.OnWatchException mechanism to handle this kind of failure by adding some data to ConsulWatchExceptionContext that allows clients to determine this case has occurred.

What would you want to do in your use case?

[andrewvk]

I think the best approach is to do it completely similar to standard providers, such as FileConfigurationProvider. The code is like this:

private void Load(bool reload)
{
    IFileInfo file = Source.FileProvider?.GetFileInfo(Source.Path);
    if (file == null || !file.Exists)
    {
        if (Source.Optional || reload) // Always optional on reload
        {
            Data = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
        }
        else
        {
            var error = new StringBuilder(SR.Format(SR.Error_FileNotFound, Source.Path));
            if (!string.IsNullOrEmpty(file?.PhysicalPath))
            {
                error.Append(SR.Format(SR.Error_ExpectedPhysicalPath, file.PhysicalPath));
            }
            HandleException(ExceptionDispatchInfo.Capture(new FileNotFoundException(error.ToString())));
        }
    }

That is, when reload, the behavior is always the same as the optional source, no matter what the original settings are.

[Choc13]

Ah yes I remember that behaviour now from when I originally wrote this one. Good idea. Let's go for that. Should be pretty straightforward to fix now then. 👍

[andrewvk]

Made PR - https://github.com/wintoncode/Winton.Extensions.Configuration.Consul/pull/129