Unexpected behavior when a null label occurs in the middle of a set of selects

[jwyza-pi]

Problem

When using multiple Selects chained together, putting a null labeled select in the middle causes that to take precedence even if a future label should have overridden it.

Example

Given the following Keys in AAC:

And the following code:

void Main()
{
	IConfigurationRoot root = new ConfigurationBuilder()
              .AddAzureAppConfiguration(opts =>
              {
	              opts.ConfigureClientOptions(o => o.Diagnostics.IsLoggingEnabled = true);
	              var credential = new DefaultAzureCredential(new DefaultAzureCredentialOptions());
              
	              var aac = opts.Connect(new Uri("urlgoeshere"), credential);
	              aac.TrimKeyPrefix("Test:");
	              aac
		              .Select(KeyFilter.Any, "Development") // Gets all Keys with Development label
		              .Select(KeyFilter.Any, "WyzaTest") // Overrides any keys with WyzaTest label
		              .Select("Test:*") // Selects any keys starting with "Test:*" with no label
		              .Select("Test:*", "Development") // Selects any keys starting with "Test:*" with Development label
		              .Select("Test:*", "WyzaTest"); // Selects any keys starting with "Test:*" with WyzaTest label
	              aac.ConfigureKeyVault(kv => kv.SetCredential(credential));
              }).Build();
										
	System.Console.WriteLine(root.GetValue<string>("WyzaTest"));
}

The expected outcome should be that the word "boo" is written to the console because the very last selector of Test:* with label WyzaTest should have matched. Instead the actual result is that it returns true which is what the (no label) value is for that key.

If I change the selection to this, then it will properly resolve to boo as expected. Of course the layers of overrides won't quite behave the same way in this scenario since the goal was to override non-prefixed keys with their prefixed versions if they existed even if they didn't have a label.

		              .Select("Test:*") // Selects any keys starting with "Test:*" with no label
		              .Select(KeyFilter.Any, "Development") // Gets all Keys with Development label
		              .Select(KeyFilter.Any, "WyzaTest") // Overrides any keys with WyzaTest label
		              .Select("Test:*", "Development") // Selects any keys starting with "Test:*" with Development label
		              .Select("Test:*", "WyzaTest"); // Selects any keys starting with "Test:*" with WyzaTest label

If this really is the intended behavior, then I suggest something be added in to prevent (or warn) you from selecting a \0 label after any non-null label selection as it won't behave in the way that one might expect.

[avanigupta]

Hello Jonathon Wyza (@jwyza-pi), thank you for reporting this issue. We try to optimize the number of requests an application makes to AppConfig service. One of those optimizations is that we skip some Select statements if their selector would end up loading key-values that have already been loaded by a previous Select statement (code here).

But this introduces the issue you are noticing since we dont override the values from the skipped Select statements. Your use-case sounds fairly reasonable to me, and we need to rethink this optimization to support your scenario.

[jwyza-pi]

Unless I am not understanding the code, I don't think that the statement of "have already been loaded by a previous select" is accurate. The code checks to make sure that there isn't any select that would do it. It's not doing any sort of restriction to only look into the past, it looks at both past and future and only excludes the current one from it's condition.

If the priority is on keeping the number of requests to AAC down, then one choice could be to temporarily cache the results of the requests that are KeyFilter.Any with a label, then apply them at the right point in the chain without calling the client again. I think this would add significant complexity to what is (right now) a relatively simple method, but it would favor keeping the number of calls small.

Another thought would be to improve how to detect whether or not it actually would result in pointless calls. In this scenario we'd want to keep track of the state of the stack of overrides and look back to see if there was another call after a KeyFilter.Any with LabelFilter.Null with this selector.

So in this example

.Select(KeyFilter.Any, "Development") 
.Select(KeyFilter.Any, "WyzaTest") 
.Select("Test:*") // <-- Don't look further back than this entry when checking to see if another selection would have included "Test:*" 
.Select("Test:*", "Development") 
.Select("Test:*", "WyzaTest"); 
.Select("Test:SubSection:*", "WyzaTest"); // <-- This would be skipped since it's a child of Test:* and we've called "Test:*" with the Label "WyzaTest" previously.
.Select("SomeOtherThing:*", "Development") // <-- This would still be skipped because there has not been anything since the original "KeyFilter.Any" + "Development" selection that would have reset it.

So basically we'd be storing a Dictionary<string, int> where the keys would be KeyFilters where label is LabelFilter.Null and the value would be the index of the selector in the list. Then we'd skip the first X selectors up to that index before checking to see if the current selector was pointless because a previous selector would have overridden it rather than looking at all past selectors.

[jwyza-pi]

Follow up. This isn't super clean code, something I was toying around with that still needs alot of refinement, but something like this for that foreach loop in the current code:

// New Code
var temp = new Dictionary<string, short>();
short index = -1;
// New Code -end

foreach (var loadOption in _options.KeyValueSelectors)
{
// New Code
    index++;
    short skip = temp.Where(x => loadOption.KeyFilter.StartsWith(x.Key))
        .Select(x => x.Value)
        .DefaultIfEmpty((short)0)
        .Max(x => x);
//New code -end (other than the added .Skip(skip) in this linq query:
    if ((useDefaultQuery && LabelFilter.Null.Equals(loadOption.LabelFilter)) ||
        _options.KeyValueSelectors.Skip(skip).Any(s => s != loadOption &&
                                                       string.Equals(s.KeyFilter, KeyFilter.Any) &&
                                                       string.Equals(s.LabelFilter, loadOption.LabelFilter)))
    {
        // This selection was already encapsulated by a wildcard query
        // Or would select kvs obtained by a different selector
        // We skip it to prevent unnecessary requests
        continue;
    }

    // New Code
    if (loadOption.LabelFilter == LabelFilter.Null && loadOption.KeyFilter.EndsWith(KeyFilter.Any))
    {
        string filterWithoutWildcard =
            loadOption.KeyFilter.Substring(0, loadOption.KeyFilter.Length - 1);
        if (temp.ContainsKey(filterWithoutWildcard))
        {
            temp[filterWithoutWildcard] = index;
        }
        else
        {
            temp.Add(filterWithoutWildcard, index);
        }

    }
   // New Code -End

    var selector = new SettingSelector
    {
        KeyFilter = loadOption.KeyFilter,
        LabelFilter = loadOption.LabelFilter
    };

    await CallWithRequestTracing(async () =>
    {
        await foreach (ConfigurationSetting setting in _client.GetConfigurationSettingsAsync(selector, cancellationToken).ConfigureAwait(false))
        {
            serverData[setting.Key] = setting;
        }
    }).ConfigureAwait(false);
}

[jwyza-pi]

Avani Gupta (@avanigupta) do you think my proposed concept code is worth pursuing? If so I'd be happy to spend time refining it and putting in a PR, but I don't want to commit time to it if the owning team wants to take a different approach to solving this issue.

[avanigupta]

Hi Jonathon Wyza (@jwyza-pi), we have decided to remove this optimization in favor of keeping the code simple. Thank you for your inputs as they helped in making the decision!

[jwyza-pi]

Just a quick question, how often do releases get pushed out? If there is a document somewhere that describes this, I'd be happy to look at it (teach a man to fish vs give a man a fish :) ).

[avanigupta]

We don't have a document but usually we push out new releases every 2-3 months, or whenever we have major updates to release. We are working on a few more items for the next release and I'll update this thread when it's ready.

[avanigupta]

Hi Jonathon Wyza (@jwyza-pi), this should be fixed in the latest stable release - v5.2.0