etag lock once again passed down to wire (Azure#24731)

* etag lock once again passed down to wire

* don't modify options in DownloadStreamingInternal

* deep copy constructors

* will no longer clone default

* added null coalesce now that clone can return null

Co-authored-by: jschrepp-MSFT <[email protected]>

Author: jaschrep-msft

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

@@ -1260,7 +1260,7 @@ private async Task<Response<BlobDownloadStreamingResult>> DownloadStreamingInter
 
                     if (UsingClientSideEncryption)
                     {
-                        options ??= new BlobDownloadOptions();
+                        options = BlobDownloadOptions.CloneOrDefault(options) ?? new BlobDownloadOptions();
                         options.Range = BlobClientSideDecryptor.GetEncryptedBlobRange(options.Range);
                     }
 
@@ -1279,6 +1279,8 @@ private async Task<Response<BlobDownloadStreamingResult>> DownloadStreamingInter
 
                     ETag etag = response.Value.Details.ETag;
                     BlobRequestConditions conditionsWithEtag = options?.Conditions?.WithIfMatch(etag) ?? new BlobRequestConditions { IfMatch = etag };
+                    options = BlobDownloadOptions.CloneOrDefault(options) ?? new BlobDownloadOptions();
+                    options.Conditions = conditionsWithEtag;
 
                     // Wrap the response Content in a RetriableStream so we
                     // can return it before it's finished downloading, but still
@@ -1394,14 +1396,18 @@ private async Task<Response<BlobDownloadStreamingResult>> StartDownloadAsync(
             CancellationToken cancellationToken = default)
         {
             HttpRange? pageRange = null;
-            if ((options?.Range ?? default) != default || startOffset != 0)
+            if ((options?.Range ?? default) != default)
             {
                 pageRange = new HttpRange(
                     options.Range.Offset + startOffset,
                     options.Range.Length.HasValue ?
                         options.Range.Length.Value - startOffset :
                         (long?)null);
             }
+            else if (startOffset != 0)
+            {
+                pageRange = new HttpRange(startOffset);
+            }
 
             ClientConfiguration.Pipeline.LogTrace($"Download {Uri} with range: {pageRange}");
 

sdk/storage/Azure.Storage.Blobs/src/Models/BlobDownloadOptions.cs

@@ -1,6 +1,9 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System;
+using Azure.Core;
+
 namespace Azure.Storage.Blobs.Models
 {
     /// <summary>
@@ -28,5 +31,46 @@ public class BlobDownloadOptions
         /// REST documentation</a> for range limitation details.
         /// </summary>
         public DownloadTransactionalHashingOptions TransactionalHashingOptions { get; set; }
+
+        /// <summary>
+        /// Default constructor.
+        /// </summary>
+        public BlobDownloadOptions()
+        {
+        }
+
+        /// <summary>
+        /// Deep copy constructor.
+        /// </summary>
+        /// <param name="deepCopySource"></param>
+        private BlobDownloadOptions(BlobDownloadOptions deepCopySource)
+        {
+            Argument.AssertNotNull(deepCopySource, nameof(deepCopySource));
+
+            Range = new HttpRange(offset: deepCopySource.Range.Offset, length: deepCopySource.Range.Length);
+            Conditions = BlobRequestConditions.CloneOrDefault(deepCopySource.Conditions);
+            // can't access an internal deep copy in Storage.Common
+            TransactionalHashingOptions = deepCopySource.TransactionalHashingOptions == default
+                ? default
+                : new DownloadTransactionalHashingOptions()
+                {
+                    Algorithm = deepCopySource.TransactionalHashingOptions.Algorithm,
+                    Validate = deepCopySource.TransactionalHashingOptions.Validate
+                };
+        }
+
+        /// <summary>
+        /// Creates a deep copy of the given instance, if any.
+        /// </summary>
+        /// <param name="deepCopySource">Instance to deep copy.</param>
+        /// <returns>The deep copy, or null.</returns>
+        internal static BlobDownloadOptions CloneOrDefault(BlobDownloadOptions deepCopySource)
+        {
+            if (deepCopySource == default)
+            {
+                return default;
+            }
+            return new BlobDownloadOptions(deepCopySource);
+        }
     }
 }

sdk/storage/Azure.Storage.Blobs/src/Models/BlobLeaseRequestConditions.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using Azure.Core;
+
 namespace Azure.Storage.Blobs.Models
 {
     /// <summary>
@@ -12,5 +14,43 @@ public class BlobLeaseRequestConditions : RequestConditions
         /// Optional SQL statement to apply to the Tags of the Blob.
         /// </summary>
         public string TagConditions { get; set; }
+
+        /// <summary>
+        /// Default constructor.
+        /// </summary>
+        public BlobLeaseRequestConditions()
+        {
+        }
+
+        internal BlobLeaseRequestConditions(BlobLeaseRequestConditions deepCopySource)
+        {
+            Argument.AssertNotNull(deepCopySource, nameof(deepCopySource));
+
+            TagConditions = deepCopySource.TagConditions;
+
+            // can't get to Azure.Core internals from here; copy it's values here
+
+            // Azure.MatchConditions
+            IfMatch = deepCopySource.IfMatch;
+            IfNoneMatch = deepCopySource.IfNoneMatch;
+
+            // Azure.RequestConditions
+            IfModifiedSince = deepCopySource.IfModifiedSince;
+            IfUnmodifiedSince = deepCopySource.IfUnmodifiedSince;
+        }
+
+        /// <summary>
+        /// Creates a deep copy of the given instance, if any.
+        /// </summary>
+        /// <param name="deepCopySource">Instance to deep copy.</param>
+        /// <returns>The deep copy, or null.</returns>
+        internal static BlobLeaseRequestConditions CloneOrDefault(BlobLeaseRequestConditions deepCopySource)
+        {
+            if (deepCopySource == default)
+            {
+                return default;
+            }
+            return new BlobLeaseRequestConditions(deepCopySource);
+        }
     }
 }

sdk/storage/Azure.Storage.Blobs/src/Models/BlobRequestConditions.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Text;
+using Azure.Core;
 using Azure.Storage.Blobs.Models;
 
 namespace Azure.Storage.Blobs.Models
@@ -18,6 +19,33 @@ public class BlobRequestConditions : BlobLeaseRequestConditions
         /// </summary>
         public string LeaseId { get; set; }
 
+        /// <summary>
+        /// Default constructor.
+        /// </summary>
+        public BlobRequestConditions()
+        {
+        }
+
+        private BlobRequestConditions(BlobRequestConditions deepCopySource) : base(deepCopySource)
+        {
+            Argument.AssertNotNull(deepCopySource, nameof(deepCopySource));
+            LeaseId = deepCopySource.LeaseId;
+        }
+
+        /// <summary>
+        /// Creates a deep copy of the given instance, if any.
+        /// </summary>
+        /// <param name="deepCopySource">Instance to deep copy.</param>
+        /// <returns>The deep copy, or null.</returns>
+        internal static BlobRequestConditions CloneOrDefault(BlobRequestConditions deepCopySource)
+        {
+            if (deepCopySource == default)
+            {
+                return default;
+            }
+            return new BlobRequestConditions(deepCopySource);
+        }
+
         /// <summary>
         /// Converts the value of the current RequestConditions object to
         /// its equivalent string representation.
@@ -42,14 +70,9 @@ public override string ToString()
         }
 
         internal BlobRequestConditions WithIfMatch(ETag etag) =>
-            new BlobRequestConditions
+            new BlobRequestConditions(this)
             {
-                LeaseId = LeaseId,
                 IfMatch = etag,
-                IfNoneMatch = IfNoneMatch,
-                IfModifiedSince = IfModifiedSince,
-                IfUnmodifiedSince = IfUnmodifiedSince,
-                TagConditions = TagConditions,
             };
 
         /// <summary>

sdk/storage/Azure.Storage.Blobs/tests/ModelsTests.cs

@@ -0,0 +1,111 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using System.Text;
+using System.Threading.Tasks;
+using Azure.Storage.Blobs.Models;
+using NUnit.Framework;
+
+namespace Azure.Storage.Blobs.Tests
+{
+    public class ModelsTests
+    {
+        private bool ReflectiveValueEqual(object left, object right, int recusrsiveLimit = 10)
+        {
+            if (recusrsiveLimit <= 0)
+            {
+                throw new ArgumentException("Hit recursive limit for equality test");
+            }
+
+            if (left == default && right == default)
+            {
+                return true;
+            }
+            else if (left == default || right == default)
+            {
+                return false;
+            }
+
+            if (left.GetType() != right.GetType())
+            {
+                return false;
+            }
+            if (left.GetType().IsValueType)
+            {
+                return left == right;
+            }
+
+            bool equal = true;
+            PropertyInfo[] properties = left.GetType().GetProperties();
+            foreach (PropertyInfo property in properties)
+            {
+                if (property.PropertyType.IsValueType)
+                {
+                    var leftval = property.GetValue(left);
+                    var rightval = property.GetValue(right);
+                    equal &= leftval.Equals(rightval);
+                }
+                else
+                {
+                    equal &= ReflectiveValueEqual(property.GetValue(left), property.GetValue(right), recusrsiveLimit - 1);
+                }
+
+                if (!equal)
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        [Test]
+        public void BlobDownloadOptionsDeepCopy()
+        {
+            // note these are all non-default values
+            BlobDownloadOptions MakeOriginal()
+            {
+                return new BlobDownloadOptions
+                {
+                    Conditions = new BlobRequestConditions
+                    {
+                        IfModifiedSince = DateTime.Now,
+                        IfUnmodifiedSince = DateTime.Now,
+                        IfMatch = new ETag("foo"),
+                        IfNoneMatch = new ETag("bar")
+                    },
+                    Range = new HttpRange(offset: 1, length: 1),
+                    TransactionalHashingOptions = new DownloadTransactionalHashingOptions
+                    {
+                        Algorithm = TransactionalHashAlgorithm.MD5,
+                        Validate = true
+                    }
+                };
+            }
+
+            var original = MakeOriginal();
+            var deepCopy = BlobDownloadOptions.CloneOrDefault(original);
+
+            // test deep copy successful equality
+            Assert.IsTrue(ReflectiveValueEqual(original, deepCopy), "deep copy did not properly clone");
+
+            // test deep copy no longer equal when one value changed
+            deepCopy.Range = new HttpRange(offset: 1, length: 2);
+            Assert.IsFalse(ReflectiveValueEqual(original, deepCopy), "change to deep copy affected original");
+
+            // TODO perhaps come up with exhaustive test to ensure this is true for all possible changes
+        }
+
+        [Test]
+        public void BlobDownloadOptionsDeepCopyDefault()
+        {
+            BlobDownloadOptions original = default;
+            BlobDownloadOptions deepCopy = BlobDownloadOptions.CloneOrDefault(original);
+
+            Assert.IsNull(deepCopy);
+        }
+    }
+}