Update transfer utility class hierarchy

[GarrettBeatty]

Description

TransferDownloadDirectoryRequest currently has fields which are already located in BaseDownloadRequest. This change makes TransferDownloadDirectoryRequest extend BaseDownloadRequest so that we dont have to duplicate fields.

In the next PR, we will be adding more common fields to BaseDownloadRequest (in order to comply with having all of the required s3 fields in our high level Transfer Utility Request). To make it easier to review I am separating the changes into multiple PRs (this PR and future PRs)

Please note there is one small difference which is we are changing ModifiedSinceDate and UnmodifiedSinceDate default behavior.

TransferUtilityDownloadDirectoryRequest

 public DateTime ModifiedSinceDate
        {
            get { return this.modifiedSinceDate.GetValueOrDefault(); }
            set { this.modifiedSinceDate = value; }
        }

to be

BaseDownloadRequest

   public DateTime ModifiedSinceDate
        {
            get { return this.modifiedSinceDate ?? DateTime.SpecifyKind(default, DateTimeKind.Utc); }
            set
            {
                this.modifiedSinceDate = value;
            }
        }

From my testing (see Peter's comment in PR) - the only difference between the two is that the DateTimeKind was not set in the first approach. And in the new code it will be set to UTC.

Motivation and Context

DOTNET-8277

Testing

dry run - 52990dbb-1153-40cc-aad0-04c8b6c39b70 pass

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed

License

• I confirm that this pull request can be released under the Apache 2 license

[Copilot]

Pull Request Overview

This PR refactors the TransferUtilityDownloadDirectoryRequest class to inherit from BaseDownloadRequest, eliminating code duplication by removing redundant fields that already exist in the base class.

• Converts TransferUtilityDownloadDirectoryRequest from a standalone class to inherit from BaseDownloadRequest
• Removes duplicate field definitions for properties like BucketName, ModifiedSinceDate, UnmodifiedSinceDate, and server-side encryption settings
• Maintains existing functionality while leveraging inheritance for common download request properties

[peterrsongg]

Can you check that the behavior isn't different? In the BaseDownloadRequest I see
https://github.com/aws/aws-sdk-net/blob/main/sdk/src/Services/S3/Custom/Transfer/BaseDownloadRequest.cs#L145-L152

there is some extra logic. Same goes for ModifiedSinceDate

[GarrettBeatty]

good catch ill have to double check this

[GarrettBeatty]

so i made a test program and validated that the two approaches are basically identical. The only thing that is different is the Kind - Unspecified vs Utc.

In both cases the date will be 1/1/0001 12:00:00 AM when its not set.

using System;

namespace DateTimeComparisonApp
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("=== ModifiedSinceDate Implementation Comparison ===");
            Console.WriteLine();

            // Test both implementations when values are not set (null)
            TestUnsetValues();
            Console.WriteLine();

            // Test both implementations when values are explicitly set
            TestSetValues();
            Console.WriteLine();

            // Test edge cases
            TestEdgeCases();
            Console.WriteLine();

            Console.WriteLine("Press any key to exit...");
            Console.ReadKey();
        }

        static void TestUnsetValues()
        {
            Console.WriteLine("📋 TESTING UNSET VALUES (when DateTime? is null):");
            Console.WriteLine("".PadRight(50, '='));

            // Simulate the two approaches
            DateTime? modifiedSinceDate = null;  // Simulating unset property
            DateTime? unmodifiedSinceDate = null; // Simulating unset property

            // Implementation 1: GetValueOrDefault() approach (current TransferUtilityDownloadDirectoryRequest?)
            var result1 = modifiedSinceDate.GetValueOrDefault();

            // Implementation 2: Null coalescing with SpecifyKind approach (BaseDownloadRequest)
            var result2 = unmodifiedSinceDate ?? DateTime.SpecifyKind(default, DateTimeKind.Utc);

            Console.WriteLine($"Method 1 (GetValueOrDefault): {result1} (Kind: {result1.Kind})");
            Console.WriteLine($"Method 2 (Null coalescing + UTC): {result2} (Kind: {result2.Kind})");
            Console.WriteLine();

            // Compare the results
            Console.WriteLine("🔍 COMPARISON RESULTS:");
            Console.WriteLine($"✓ Same Ticks: {result1.Ticks == result2.Ticks}");
            Console.WriteLine($"✓ Same Date: {result1.Date == result2.Date}");
            Console.WriteLine($"✓ Same TimeOfDay: {result1.TimeOfDay == result2.TimeOfDay}");
            Console.WriteLine($"✓ Same DateTime Value: {result1 == result2}");
            Console.WriteLine($"❗ Different Kind: {result1.Kind} vs {result2.Kind}");

            // Check if they're equivalent when converted to UTC
            var result1Utc = result1.ToUniversalTime();
            var result2Utc = result2.ToUniversalTime();
            Console.WriteLine($"✓ UTC Conversion Identical: {result1Utc == result2Utc}");

            Console.WriteLine();
            Console.WriteLine("🎯 CONCLUSION: Implementations are functionally equivalent!");
            Console.WriteLine("   Only DateTimeKind differs (Unspecified vs UTC)");
            Console.WriteLine("   UTC is more appropriate for S3 operations.");
        }

        static void TestSetValues()
        {
            Console.WriteLine("📋 TESTING SET VALUES (when DateTime? has a value):");
            Console.WriteLine("".PadRight(50, '='));

            var testDateTime = new DateTime(2023, 12, 25, 14, 30, 0, DateTimeKind.Local);
            
            // Simulate both approaches with set values
            DateTime? modifiedSinceDate = testDateTime;
            DateTime? unmodifiedSinceDate = testDateTime;

            var result1 = modifiedSinceDate.GetValueOrDefault();
            var result2 = unmodifiedSinceDate ?? DateTime.SpecifyKind(default, DateTimeKind.Utc);

            Console.WriteLine($"Test Value: {testDateTime} (Kind: {testDateTime.Kind})");
            Console.WriteLine($"Method 1 Result: {result1} (Kind: {result1.Kind})");
            Console.WriteLine($"Method 2 Result: {result2} (Kind: {result2.Kind})");
            Console.WriteLine();

            Console.WriteLine("🔍 COMPARISON RESULTS:");
            Console.WriteLine($"✓ Same Ticks: {result1.Ticks == result2.Ticks}");
            Console.WriteLine($"✓ Same DateTime Value: {result1 == result2}");
            Console.WriteLine($"✓ Same Kind: {result1.Kind == result2.Kind}");

            Console.WriteLine();
            Console.WriteLine("🎯 CONCLUSION: When values are set, both implementations behave identically!");
        }

        static void TestEdgeCases()
        {
            Console.WriteLine("📋 TESTING EDGE CASES:");
            Console.WriteLine("".PadRight(50, '='));

            // Test with different DateTimeKind values
            var utcDateTime = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            var localDateTime = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Local);
            var unspecifiedDateTime = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Unspecified);

            Console.WriteLine("Testing with different DateTimeKind values:");
            
            TestEdgeCase("UTC DateTime", utcDateTime);
            TestEdgeCase("Local DateTime", localDateTime);
            TestEdgeCase("Unspecified DateTime", unspecifiedDateTime);
        }

        static void TestEdgeCase(string description, DateTime testValue)
        {
            DateTime? modifiedSinceDate = testValue;
            DateTime? unmodifiedSinceDate = testValue;

            var result1 = modifiedSinceDate.GetValueOrDefault();
            var result2 = unmodifiedSinceDate ?? DateTime.SpecifyKind(default, DateTimeKind.Utc);

            Console.WriteLine($"  {description}:");
            Console.WriteLine($"    Input: {testValue} (Kind: {testValue.Kind})");
            Console.WriteLine($"    Method 1: {result1} (Kind: {result1.Kind})");
            Console.WriteLine($"    Method 2: {result2} (Kind: {result2.Kind})");
            Console.WriteLine($"    Identical: {result1 == result2 && result1.Kind == result2.Kind}");
            Console.WriteLine();
        }
    }
}

 TESTING UNSET VALUES (when DateTime? is null):
==================================================
Method 1 (GetValueOrDefault): 1/1/0001 12:00:00 AM (Kind: Unspecified)
Method 2 (Null coalescing + UTC): 1/1/0001 12:00:00 AM (Kind: Utc)

🔍 COMPARISON RESULTS:
✓ Same Ticks: True
✓ Same Date: True
✓ Same TimeOfDay: True
✓ Same DateTime Value: True
❗ Different Kind: Unspecified vs Utc
✓ UTC Conversion Identical: False

🎯 CONCLUSION: Implementations are functionally equivalent!
   Only DateTimeKind differs (Unspecified vs UTC)
   UTC is more appropriate for S3 operations.

📋 TESTING SET VALUES (when DateTime? has a value):
==================================================
Test Value: 12/25/2023 2:30:00 PM (Kind: Local)
Method 1 Result: 12/25/2023 2:30:00 PM (Kind: Local)
Method 2 Result: 12/25/2023 2:30:00 PM (Kind: Local)

🔍 COMPARISON RESULTS:
✓ Same Ticks: True
✓ Same DateTime Value: True
✓ Same Kind: True

🎯 CONCLUSION: When values are set, both implementations behave identically!

📋 TESTING EDGE CASES:
==================================================
Testing with different DateTimeKind values:
  UTC DateTime:
    Input: 1/1/2023 12:00:00 AM (Kind: Utc)
    Method 1: 1/1/2023 12:00:00 AM (Kind: Utc)
    Method 2: 1/1/2023 12:00:00 AM (Kind: Utc)
    Identical: True

  Local DateTime:
    Input: 1/1/2023 12:00:00 AM (Kind: Local)
    Method 1: 1/1/2023 12:00:00 AM (Kind: Local)
    Method 2: 1/1/2023 12:00:00 AM (Kind: Local)
    Identical: True

  Unspecified DateTime:
    Input: 1/1/2023 12:00:00 AM (Kind: Unspecified)
    Method 1: 1/1/2023 12:00:00 AM (Kind: Unspecified)
    Method 2: 1/1/2023 12:00:00 AM (Kind: Unspecified)
    Identical: True

[GarrettBeatty]

my initial thoughts are that the Kind should not matter in this case in reality since the default value would be 1/1/0001 and there would be no real functional difference between UTC and unspecified kind in this case because the date is so old

[Copilot]

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[normj]

The inherited Key and VersionId from the base class would make no sense for the download directory request and could cause confusion.

If this was needed for some consolidation of logic reading the properties I suggest adding an internal interface that has just the properties you need but don't make this inheritance change.

[GarrettBeatty]

The inherited Key and VersionId from the base class would make no sense for the download directory request and could cause confusion.

If this was needed for some consolidation of logic reading the properties I suggest adding an internal interface that has just the properties you need but don't make this inheritance change.

good point i didnt notice those extra properties

[GarrettBeatty]

@normj im wondering if we can do something like this (i updated this pr with a sample implementation of below)

Current

BaseDownloadRequest (abstract)
├── BucketName
├── Key
├── VersionId
├── ModifiedSinceDate, UnmodifiedSinceDate
├── ServerSideEncryptionCustomerMethod
├── ServerSideEncryptionCustomerProvidedKey
├── ServerSideEncryptionCustomerProvidedKeyMD5
├── ChecksumMode
├── RequestPayer
└── (All shared S3 single-object download properties)
    │
    ├── TransferUtilityDownloadRequest : BaseDownloadRequest
    │   ├── FilePath
    │   └── WriteObjectProgressEvent
    │
    └── TransferUtilityOpenStreamRequest : BaseDownloadRequest
        └── (No new fields; used for streaming)
        
TransferUtilityDownloadDirectoryRequest (standalone class)
├── BucketName
├── S3Directory
├── LocalDirectory
├── ModifiedSinceDate, UnmodifiedSinceDate
├── ServerSideEncryptionCustomerMethod
├── ServerSideEncryptionCustomerProvidedKey
├── ServerSideEncryptionCustomerProvidedKeyMD5
├── RequestPayer
├── etc

Proposed

BaseTransferRequest (NEW)
├── BucketName
├── ModifiedSinceDate, UnmodifiedSinceDate
├── Server-side encryption properties
├── ChecksumMode, RequestPayer
└── (All shared S3 transfer properties)
    │
    ├── BaseDownloadRequest : BaseTransferRequest (UPDATED)
    │   ├── Key (object-specific)
    │   └── VersionId (object-specific)
    │       │
    │       └── TransferUtilityDownloadRequest : BaseDownloadRequest
    │           └── FilePath, additional download-specific properties
    │
    └── TransferUtilityDownloadDirectoryRequest : BaseTransferRequest (UPDATED)
        ├── S3Directory, LocalDirectory  
        ├── SearchPattern, SearchOption
        └── (Directory-specific properties - NO Key/VersionId)

we would need to change some of the hierarchy in the public classes. This would reduce the code duplication but then still let each class have its specific properties when needed

[normj]

@normj im wondering if we can do something like this (i updated this pr with a sample implementation of below)

Current

BaseDownloadRequest (abstract)
├── BucketName
├── Key
├── VersionId
├── ModifiedSinceDate, UnmodifiedSinceDate
├── ServerSideEncryptionCustomerMethod
├── ServerSideEncryptionCustomerProvidedKey
├── ServerSideEncryptionCustomerProvidedKeyMD5
├── ChecksumMode
├── RequestPayer
└── (All shared S3 single-object download properties)
    │
    ├── TransferUtilityDownloadRequest : BaseDownloadRequest
    │   ├── FilePath
    │   └── WriteObjectProgressEvent
    │
    └── TransferUtilityOpenStreamRequest : BaseDownloadRequest
        └── (No new fields; used for streaming)
        
TransferUtilityDownloadDirectoryRequest (standalone class)
├── BucketName
├── S3Directory
├── LocalDirectory
├── ModifiedSinceDate, UnmodifiedSinceDate
├── ServerSideEncryptionCustomerMethod
├── ServerSideEncryptionCustomerProvidedKey
├── ServerSideEncryptionCustomerProvidedKeyMD5
├── RequestPayer
├── etc

Proposed

BaseTransferRequest (NEW)
├── BucketName
├── ModifiedSinceDate, UnmodifiedSinceDate
├── Server-side encryption properties
├── ChecksumMode, RequestPayer
└── (All shared S3 transfer properties)
    │
    ├── BaseDownloadRequest : BaseTransferRequest (UPDATED)
    │   ├── Key (object-specific)
    │   └── VersionId (object-specific)
    │       │
    │       └── TransferUtilityDownloadRequest : BaseDownloadRequest
    │           └── FilePath, additional download-specific properties
    │
    └── TransferUtilityDownloadDirectoryRequest : BaseTransferRequest (UPDATED)
        ├── S3Directory, LocalDirectory  
        ├── SearchPattern, SearchOption
        └── (Directory-specific properties - NO Key/VersionId)

we would need to change some of the hierarchy in the public classes. This would reduce the code duplication but then still let each class have its specific properties when needed

I'm 99% sure this causes a binary breaking change and users would have to recompile which might not be possible depending on if the SDK is being brought in as a transitive dependency.