generate create session #4125

peterrsongg · 2025-11-13T22:55:24Z

Description

1 of 5 stacked PRs. Generates Create Session.
I will create one DevConfig with all these in merge PR.

HOW TO REVIEW / FIND differences

Pull up current custom code on left and newly generated code on right instead of relying on the github diff.

NOTE

CreateSessionRequestMarshaller was setting BucketName in the resource path which isn't right. I tested with python:

Python:  method=GET, url=https://<s3-bucket-name>.s3express-usw2-az1.us-west-2.amazonaws.com/?session)
.NET    :  {Method: GET,'https://<s3-bucket-name>.s3express-usw2-az1.us-west-2.amazonaws.com/<s3-bucket-name>?session', Version: 1.1

Then after generation we are doing it the same as python:

Request	{Method: GET, RequestUri: 'https://<s3-bucket-name>.s3express-usw2-az1.us-west-2.amazonaws.com/?session'

Motivation and Context

Testing

Base-Branch dry run 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

stack-info: PR: #4125, branch: peterrsongg/petesong/phase-3-pr-2-all/1

peterrsongg · 2025-11-14T18:03:16Z

.../Services/S3/Custom/Model/Internal/MarshallTransformations/CreateSessionRequestMarshaller.cs

+            request.ResourcePath = "/";

-            request.AddPathResource("{Bucket}", StringUtils.FromString(publicRequest.BucketName));
-            request.ResourcePath = "/{Bucket}";


this is the difference in resource path im talking about in the description

peterrsongg · 2025-11-14T18:32:14Z

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:

Custom Marshallers: Logic properly split between Generated and Custom files using partial methods:
- GetBucketAnalyticsConfigurationRequestMarshaller - PreMarshallCustomization preserved
- PutBucketAnalyticsConfigurationRequestMarshaller - AnalyticsFilterCustomMarshall and PostMarshallCustomization preserved
- AnalyticsS3BucketDestinationUnmarshaller - FormatCustomUnmarshall preserved
SessionCredentials.cs - No breaking changes (identical behavior)
CreateSessionRequest.cs - No breaking changes (field name changes only, getters/setters identical)
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:

String validation changes that no longer reject empty strings (AnalyticsConfiguration, GetBucketAnalyticsConfigurationRequest)
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.

dscpinheiro · 2025-11-14T20:37:34Z

...rvices/S3/Generated/Model/Internal/MarshallTransformations/CreateSessionRequestMarshaller.cs

            }
+            if (!publicRequest.IsSetBucketName())
+                throw new AmazonS3Exception("Request object does not have required field BucketName set");
+            request.ResourcePath = "/";


The rest of the PR looks OK but I do need to check this is intentional (there's a routing section in the S3 Express SEP, were we not following that before? @muhammad-othman)

@dscpinheiro this is just an S3 quirk, bucketname is modeled as a uri param, but in reality it is part of the host. The way they model it is incorrect. If i just let the genertator run, it would try to add the bucket name to the resource path, but in the generator I added logic to not do that for s3 (near the beginning of the epic)

generate create session

6a201fb

stack-info: PR: #4125, branch: peterrsongg/petesong/phase-3-pr-2-all/1

peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-2-all/1 branch from 839af32 to 6a201fb Compare November 13, 2025 22:56

peterrsongg marked this pull request as ready for review November 14, 2025 17:53

peterrsongg commented Nov 14, 2025

View reviewed changes

dscpinheiro requested review from dscpinheiro and muhammad-othman November 14, 2025 18:21

dscpinheiro reviewed Nov 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

generate create session #4125

generate create session #4125

Uh oh!

peterrsongg commented Nov 13, 2025 •

edited

Loading

Uh oh!

peterrsongg Nov 14, 2025

Uh oh!

peterrsongg commented Nov 14, 2025

Uh oh!

dscpinheiro Nov 14, 2025

Uh oh!

peterrsongg Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

generate create session #4125

Are you sure you want to change the base?

generate create session #4125

Uh oh!

Conversation

peterrsongg commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

HOW TO REVIEW / FIND differences

NOTE

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

peterrsongg Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

peterrsongg commented Nov 14, 2025

Breaking Changes Analysis Report

Summary

BREAKING CHANGES DETECTED

1. AnalyticsConfiguration.cs

2. AnalyticsExportDestination.cs

3. AnalyticsS3BucketDestination.cs

4. StorageClassAnalysisDataExport.cs

5. GetBucketAnalyticsConfigurationRequest.cs

VERIFIED AS CORRECT

Files Analyzed: 69/69 (100%)

Model Files Analyzed (16):

Marshaller/Unmarshaller Files Analyzed (23):

Configuration Files Analyzed (2):

CRITICAL IMPACT SUMMARY

Uh oh!

dscpinheiro Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

peterrsongg Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

peterrsongg commented Nov 13, 2025 •

edited

Loading