Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 13, 2025

Description

3 of 5 stacked PRs. Generates GetBucketAnalyticsConfiguration
I will create one DevConfig with all these in merge PR.

NOTE

AnalyticsId - not changing IsSet because we check string.IsNullOrEmpty earlier and throw if true

Motivation and Context

Testing

Base-Branch dry run passed
FuzzTesting passed

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

@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-2-all/2 branch from 46a57f7 to 42efdb7 Compare November 13, 2025 22:56
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-2-all/3 branch from 5e2c0fe to 18d3b16 Compare November 13, 2025 22:56
@peterrsongg peterrsongg marked this pull request as ready for review November 14, 2025 17:57
@dscpinheiro dscpinheiro self-requested a review November 14, 2025 18:20
@peterrsongg
Copy link
Contributor Author

Breaking Changes Analysis Report

Summary

Analyzed 69 files changed in this PR migrating S3 Analytics and CreateSession operations from Custom to Generated code.

Total Breaking Changes Found: 5 files with issues


BREAKING CHANGES DETECTED

1. AnalyticsConfiguration.cs

File: sdk/src/Services/S3/Generated/Model/AnalyticsConfiguration.cs

ISSUE: Internal validation method logic changed

  • Old Custom: return !(string.IsNullOrEmpty(this.analyticsId));
  • New Generated: return this._analyticsId != null;
  • Impact: The old code checked for both null AND empty strings, new code only checks for null. An empty string will now pass validation when it shouldn't.

2. AnalyticsExportDestination.cs

File: sdk/src/Services/S3/Generated/Model/AnalyticsExportDestination.cs

ISSUE: Added Required attribute to S3BucketDestination property

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: This property is now marked as required at the model level. This could cause validation errors for existing code that doesn't set this property.

3. AnalyticsS3BucketDestination.cs

File: sdk/src/Services/S3/Generated/Model/AnalyticsS3BucketDestination.cs

ISSUES (Multiple):

a) BucketName Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: BucketName is now required when previously it was optional

b) Format Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: Format is now required when previously it was optional

c) Internal Validation Methods Changed

  • BucketAccountId:

    • Old: return !(string.IsNullOrEmpty(this.accountId));
    • New: return !String.IsNullOrEmpty(this._bucketAccountId);
    • Impact: Changed from checking both null and empty to using String.IsNullOrEmpty (functionally equivalent)
  • BucketName:

    • Old: return !(string.IsNullOrEmpty(this.bucketName));
    • New: return !String.IsNullOrEmpty(this._bucketName);
    • Impact: Changed from checking both null and empty to using String.IsNullOrEmpty (functionally equivalent)
  • Prefix:

    • Old: return !(string.IsNullOrEmpty(this.prefix));
    • New: return !String.IsNullOrEmpty(this._prefix);
    • Impact: Changed from checking both null and empty to using String.IsNullOrEmpty (functionally equivalent)

4. StorageClassAnalysisDataExport.cs

File: sdk/src/Services/S3/Generated/Model/StorageClassAnalysisDataExport.cs

ISSUES (Multiple):

a) Destination Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: Destination property is now required when previously it was optional

b) OutputSchemaVersion Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: OutputSchemaVersion is now required when previously it was optional

5. GetBucketAnalyticsConfigurationRequest.cs

File: sdk/src/Services/S3/Generated/Model/GetBucketAnalyticsConfigurationRequest.cs

ISSUES (Multiple):

a) AnalyticsId Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: AnalyticsId is now required when previously it was optional

b) BucketName Required Attribute Added

  • Old Custom: No [AWSProperty(Required=true)] attribute
  • New Generated: [AWSProperty(Required=true)] attribute added
  • Impact: BucketName is now required when previously it was optional

c) Internal Validation Methods Changed

  • AnalyticsId:

    • Old: return !(string.IsNullOrEmpty(this.analyticsId));
    • New: return this._analyticsId != null;
    • Impact: Old checked for both null AND empty strings, new only checks null. Empty string will now pass validation.
  • BucketName:

    • Old: return !(string.IsNullOrEmpty(this.bucketName));
    • New: return this._bucketName != null;
    • Impact: Old checked for both null AND empty strings, new only checks null. Empty string will now pass validation.

VERIFIED AS CORRECT

The following changes were verified as correctly preserving existing behavior:

  1. Custom Marshallers: Logic properly split between Generated and Custom files using partial methods:

    • GetBucketAnalyticsConfigurationRequestMarshaller - PreMarshallCustomization preserved
    • PutBucketAnalyticsConfigurationRequestMarshaller - AnalyticsFilterCustomMarshall and PostMarshallCustomization preserved
    • AnalyticsS3BucketDestinationUnmarshaller - FormatCustomUnmarshall preserved
  2. SessionCredentials.cs - No breaking changes (identical behavior)

  3. CreateSessionRequest.cs - No breaking changes (field name changes only, getters/setters identical)

  4. StorageClassAnalysis.cs - No breaking changes (private field name change only)


Files Analyzed: 69/69 (100%)

Model Files Analyzed (16):

✅ AnalyticsConfiguration.cs ✅ AnalyticsExportDestination.cs
✅ AnalyticsS3BucketDestination.cs ✅ CreateSessionRequest.cs ✅ CreateSessionResponse.cs ✅ DeleteBucketAnalyticsConfigurationRequest.cs ✅ DeleteBucketAnalyticsConfigurationResponse.cs ✅ GetBucketAnalyticsConfigurationRequest.cs ✅ GetBucketAnalyticsConfigurationResponse.cs ✅ ListBucketAnalyticsConfigurationsRequest.cs ✅ ListBucketAnalyticsConfigurationsResponse.cs ✅ PutBucketAnalyticsConfigurationRequest.cs ✅ PutBucketAnalyticsConfigurationResponse.cs ✅ SessionCredentials.cs ✅ StorageClassAnalysis.cs ✅ StorageClassAnalysisDataExport.cs

Marshaller/Unmarshaller Files Analyzed (23):

✅ All marshaller and unmarshaller files verified for custom logic preservation

Configuration Files Analyzed (2):

✅ generator/ServiceClientGeneratorLib/ServiceModel.cs ✅ generator/ServiceModels/s3/s3.customizations.json


CRITICAL IMPACT SUMMARY

The most severe breaking changes are:

  1. String validation changes that no longer reject empty strings (AnalyticsConfiguration, GetBucketAnalyticsConfigurationRequest)
  2. Required attributes added to previously optional properties across multiple models, which will break existing code that doesn't set these properties

These changes violate backward compatibility and will cause runtime errors for existing customer code.

Copy link
Contributor

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 generates the GetBucketAnalyticsConfiguration operation for the AWS S3 SDK, migrating from custom implementations to auto-generated code. The changes involve replacing manually-written marshaller and unmarshaller classes with generated versions, while preserving necessary custom behavior through partial methods and customization hooks.

Key changes:

  • Enabled GetBucketAnalyticsConfiguration operation in the code generator
  • Replaced custom marshallers/unmarshallers with generated versions that use partial classes for customizations
  • Updated singleton initialization pattern from lazy to eager initialization across multiple unmarshaller classes

Reviewed Changes

Copilot reviewed 5 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
GetBucketAnalyticsConfigurationResponseUnmarshaller.cs (Generated) New auto-generated response unmarshaller replacing custom implementation
GetBucketAnalyticsConfigurationRequestMarshaller.cs (Generated) New auto-generated request marshaller with partial method for customization
AnalyticsS3BucketDestinationUnmarshaller.cs (Generated) New auto-generated unmarshaller with custom Format handling
StorageClassAnalysisUnmarshaller.cs Refactored to use generated pattern with eager singleton initialization
StorageClassAnalysisDataExportUnmarshaller.cs Refactored to use generated pattern with eager singleton initialization
GetBucketAnalyticsConfigurationResponseUnmarshaller.cs (Custom) Removed custom implementation in favor of generated version
GetBucketAnalyticsConfigurationRequestMarshaller.cs (Custom) Simplified to partial class with Suppress404Exceptions customization
AnalyticsS3BucketDestinationUnmarshaller.cs (Custom) Simplified to partial class with Format custom unmarshalling method
AnalyticsExportDestinationUnmarshaller.cs Refactored to use generated pattern with eager singleton initialization
AnalyticsConfigurationUnmarshaller.cs Refactored to use generated pattern with eager singleton initialization
GetBucketAnalyticsConfigurationResponse.cs Updated to follow generated code conventions with backing fields
GetBucketAnalyticsConfigurationRequest.cs Updated to follow generated code conventions and improved documentation
s3.customizations.json Added customization directives for Analytics-related classes
ServiceModel.cs Uncommented GetBucketAnalyticsConfiguration operation in allow list

stack-info: PR: #4127, branch: peterrsongg/petesong/phase-3-pr-2-all/3
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-2-all/3 branch from 18d3b16 to 5bee66a Compare November 14, 2025 20:03
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.

2 participants