Skip to content

Conversation

GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Oct 12, 2025

This change creates 3 event progress trackers for SimpleUploadCommand. UploadInitiatedEvent, UploadedCompletedEvent, and UploadFailedEvent. This allows the user to subscribe to events like doing below

        private void uploadStarted(object sender, UploadInitiatedEventArgs args)
         {
             Console.WriteLine($"Upload started: {args.FilePath}");
             Console.WriteLine($"Total size: {args.TotalBytes} bytes");
             Console.WriteLine($"Bucket: {args.Request.BucketName}");
             Console.WriteLine($"Key: {args.Request.Key}");
         }


         TransferUtilityUploadRequest request = new TransferUtilityUploadRequest();
         request.UploadInitiatedEvent += uploadStarted;

These new events should only be fired 1 time during the entire lifecycle of the upload. The details of what each event should contain is in the SEP.

Description

  1. DOTNET-8275

Motivation and Context

This is being done as part of the SEP compliance.

Testing

  1. Integration tests/unit tests which demonstrate the event handler behavior
  2. dry run d0657a6a-92ce-4d24-99d8-15a473e66554

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

@GarrettBeatty GarrettBeatty requested a review from Copilot October 12, 2025 21:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements simple upload progress tracking for the AWS .NET SDK S3 Transfer Utility by adding lifecycle events (initiated, completed, failed) and creating a unified upload response class. It introduces a comprehensive testing framework with JSON-based field mapping validation to ensure response completeness.

Key Changes:

  • Added three new lifecycle events (UploadInitiatedEvent, UploadCompletedEvent, UploadFailedEvent) to TransferUtilityUploadRequest
  • Created TransferUtilityUploadResponse class to unify response data from different upload operations
  • Implemented ResponseMapper utility for mapping S3 response objects to the unified response format

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
TransferUtilityUploadRequest.cs Added lifecycle event handlers and event argument classes
TransferUtilityUploadResponse.cs New unified response class with properties mapped from S3 operations
SimpleUploadCommand.cs Added event firing logic and progress tracking
SimpleUploadCommand.async.cs Added async event firing and response mapping
ResponseMapper.cs New utility class for mapping S3 responses to unified format
ResponseMapperTests.cs Comprehensive test suite with JSON-driven validation
TransferUtilityTests.cs Integration tests for lifecycle events
AssemblyInfo.cs Added InternalsVisibleTo for unit testing
mapping.json JSON configuration defining field mappings between response types
property-aliases.json Property name aliases for backward compatibility
mapping-schema.json JSON schema validation for mapping configuration

@GarrettBeatty GarrettBeatty changed the title Gcbeatty/simpleuploadprogresstracking Add UploadIniaited, Completed, and Failed event progress trackers to SimpleUploadCommand Oct 12, 2025
@GarrettBeatty GarrettBeatty requested a review from Copilot October 13, 2025 14:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment on lines +107 to +108
// Keep track of the total transferred bytes so that we can also return this value in case of failure
long transferredBytes = Interlocked.Add(ref _totalTransferredBytes, e.IncrementTransferred - e.CompensationForRetry);
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of Interlocked.Add for thread-safe tracking of transferred bytes is correct, but consider potential race conditions where the final transferred bytes count might not accurately reflect the state at the exact moment of failure in multi-threaded scenarios.

Copilot uses AI. Check for mistakes.

Comment on lines +46 to +47
// Navigate up from bin/Debug/net472 to the test project root
while (testProjectDirectory != null && !File.Exists(Path.Combine(testProjectDirectory, "Custom", "TestData", "mapping.json")))
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded navigation logic to find test data files is fragile and could break with different build configurations or directory structures. Consider using a more robust approach like [DeploymentItem] attribute or embedded resources.

Copilot uses AI. Check for mistakes.

@GarrettBeatty GarrettBeatty force-pushed the gcbeatty/simpleuploadprogresstracking branch from b511b03 to 0f0e9f4 Compare October 14, 2025 15:53
… upload

update docs and function namse

update comments

add tests/update comments

remove response from progressargs

add missing properties

update test

Refactor test

rearrange test

update tests

add commented out tests

comments

comments

update comments

fix spelling

use interlocked read and fix build

dev config

update comment

fix internals
@GarrettBeatty GarrettBeatty force-pushed the gcbeatty/simpleuploadprogresstracking branch from 0f0e9f4 to 352afda Compare October 14, 2025 16:32
@GarrettBeatty GarrettBeatty changed the base branch from getobject to gcbeatty/tuuploadresponse October 14, 2025 16:43
@GarrettBeatty GarrettBeatty force-pushed the gcbeatty/tuuploadresponse branch from 1924fee to db89c38 Compare October 14, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant