Bugfix for collection monitoring etag logic (#623)

* check response code before comparing etags when they may be null

* fix ordering of etag check and response check

* fix mock response etag header

* fix select in tests

* add comment to explain loadselected etag is never null

Author: amerjusupovic

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

@@ -812,8 +812,6 @@ await CallWithRequestTracing(async () =>
                         {
                             using Response response = page.GetRawResponse();
 
-                            ETag serverEtag = (ETag)response.Headers.ETag;
-
                             foreach (ConfigurationSetting setting in page.Values)
                             {
                                 data[setting.Key] = setting;
@@ -824,7 +822,9 @@ await CallWithRequestTracing(async () =>
                                 }
                             }
 
-                            matchConditions.Add(new MatchConditions { IfNoneMatch = serverEtag });
+                            // The ETag will never be null here because it's not a conditional request
+                            // Each successful response should have 200 status code and an ETag
+                            matchConditions.Add(new MatchConditions { IfNoneMatch = response.Headers.ETag });
                         }
                     }).ConfigureAwait(false);
 

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/ConfigurationClientExtensions.cs

@@ -95,11 +95,9 @@ public static async Task<bool> HaveCollectionsChanged(this ConfigurationClient c
             {
                 using Response response = page.GetRawResponse();
 
-                ETag serverEtag = (ETag)response.Headers.ETag;
-
                 // Return true if the lists of etags are different
                 if ((!existingMatchConditionsEnumerator.MoveNext() ||
-                    !existingMatchConditionsEnumerator.Current.IfNoneMatch.Equals(serverEtag)) &&
+                    !existingMatchConditionsEnumerator.Current.IfNoneMatch.Equals(response.Headers.ETag)) &&
                     response.Status == (int)HttpStatusCode.OK)
                 {
                     return true;

tests/Tests.AzureAppConfiguration/Azure.Core.Testing/MockResponse.cs

@@ -18,7 +18,10 @@ public MockResponse(int status, string reasonPhrase = null)
             Status = status;
             ReasonPhrase = reasonPhrase;
 
-            AddHeader(new HttpHeader(HttpHeader.Names.ETag, "\"" + Guid.NewGuid().ToString() + "\""));
+            if (status == 200)
+            {
+                AddHeader(new HttpHeader(HttpHeader.Names.ETag, "\"" + Guid.NewGuid().ToString() + "\""));
+            }
         }
 
         public override int Status { get; }

tests/Tests.AzureAppConfiguration/RefreshTests.cs

@@ -1069,7 +1069,7 @@ public async Task RefreshTests_SelectedKeysRefreshWithRegisterAll()
                 .AddAzureAppConfiguration(options =>
                 {
                     options.ClientManager = TestHelpers.CreateMockedConfigurationClientManager(mockClient.Object);
-                    options.Select("TestKey*");
+                    options.Select("TestKey*", "label");
                     options.ConfigurationSettingPageIterator = new MockConfigurationSettingPageIterator();
                     options.ConfigureRefresh(refreshOptions =>
                     {
@@ -1159,7 +1159,7 @@ MockAsyncPageable GetTestKeys(SettingSelector selector, CancellationToken ct)
                 .AddAzureAppConfiguration(options =>
                 {
                     options.ClientManager = TestHelpers.CreateMockedConfigurationClientManager(mockClient.Object);
-                    options.Select("TestKey*");
+                    options.Select("TestKey*", "label");
                     options.ConfigurationSettingPageIterator = new MockConfigurationSettingPageIterator();
                     options.ConfigureRefresh(refreshOptions =>
                     {