Skip to content

Commit 33fd41e

Browse files
MadrigalMadrigal
authored
Expand checks for S3 200 errors to more operations (wave 1) (#3493)
* Expand S3 error with 200 status codegen to cover all S3 operations that should support it * Update 200 error middleware handler to not treat empty response as error * Regen service/s3 for 200 error handling * add changelog * Update snapshot testing * Update generation to release this in waves * Regenerate s3 * Update snapshot testing
1 parent e022bf5 commit 33fd41e

140 files changed

Lines changed: 481 additions & 14 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.changelog/5b72ece8d1944b2ba99ad90ad3690b46.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"id": "5b72ece8-d194-4b2b-a99a-d90ad3690b46",
3+
"type": "bugfix",
4+
"description": "Expand S3 operations that check for an error inside an HTTP 200 response",
5+
"modules": [
6+
"service/s3"
7+
]
8+
}

‎codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/service/s3/S3ErrorWith200Status.java‎

Lines changed: 198 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,23 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
116
package software.amazon.smithy.aws.go.codegen.customization.service.s3;
217

18+
import java.util.HashSet;
319
import java.util.List;
20+
import java.util.Optional;
421
import java.util.Set;
522

623
import software.amazon.smithy.aws.go.codegen.customization.AwsCustomGoDependency;
@@ -11,18 +28,166 @@
1128
import software.amazon.smithy.model.Model;
1229
import software.amazon.smithy.model.shapes.OperationShape;
1330
import software.amazon.smithy.model.shapes.ServiceShape;
14-
import software.amazon.smithy.utils.ListUtils;
15-
import software.amazon.smithy.utils.SetUtils;
31+
import software.amazon.smithy.model.shapes.ShapeId;
32+
import software.amazon.smithy.model.shapes.StructureShape;
33+
import software.amazon.smithy.model.traits.HttpPayloadTrait;
34+
import software.amazon.smithy.model.traits.StreamingTrait;
1635

1736
/**
1837
* Adds middleware to handle S3 response errors with 200 ok status code.
38+
*
39+
* Per internal specification, this customization MUST be applied to all S3
40+
* operations with a structured XML response. The response MUST NOT have a
41+
* streaming binary payload or event-stream. In Smithy terms, this means all
42+
* S3 operations whose output does not contain a member with @httpPayload
43+
* targeting a @streaming blob, @streaming union, or string.
44+
*
45+
* Coverage is being rolled out in waves gated by call volume. Once all waves
46+
* have soaked, remove the wave sets and the isInEnabledWave check — the
47+
* model-driven logic in supports200Error is the correct final behavior.
1948
*/
2049
public class S3ErrorWith200Status implements GoIntegration {
21-
private static String ADD_ERROR_HANDLER_INTERNAL = "HandleResponseErrorWith200Status";
50+
private static final String ADD_ERROR_HANDLER_INTERNAL = "HandleResponseErrorWith200Status";
51+
52+
// Already covered on main — always enabled.
53+
private static final Set<String> ORIGINAL_OPERATIONS = Set.of(
54+
"CopyObject", "UploadPartCopy", "CompleteMultipartUpload"
55+
);
56+
57+
// Wave 1: < 1B req/week. Very low risk.
58+
private static final Set<String> WAVE_1_OPERATIONS = Set.of(
59+
"AbortMultipartUpload",
60+
"CreateBucketMetadataConfiguration",
61+
"CreateBucketMetadataTableConfiguration",
62+
"CreateSession",
63+
"DeleteBucket",
64+
"DeleteBucketAnalyticsConfiguration",
65+
"DeleteBucketCors",
66+
"DeleteBucketEncryption",
67+
"DeleteBucketIntelligentTieringConfiguration",
68+
"DeleteBucketInventoryConfiguration",
69+
"DeleteBucketLifecycle",
70+
"DeleteBucketMetadataConfiguration",
71+
"DeleteBucketMetadataTableConfiguration",
72+
"DeleteBucketMetricsConfiguration",
73+
"DeleteBucketOwnershipControls",
74+
"DeleteBucketPolicy",
75+
"DeleteBucketReplication",
76+
"DeleteBucketTagging",
77+
"DeleteBucketWebsite",
78+
"DeleteObjectAnnotation",
79+
"DeleteObjectTagging",
80+
"DeletePublicAccessBlock",
81+
"GetBucketAbac",
82+
"GetBucketAnalyticsConfiguration",
83+
"GetBucketIntelligentTieringConfiguration",
84+
"GetBucketInventoryConfiguration",
85+
"GetBucketMetadataConfiguration",
86+
"GetBucketMetadataTableConfiguration",
87+
"GetBucketMetricsConfiguration",
88+
"GetBucketTagging",
89+
"GetObjectAcl",
90+
"GetObjectAttributes",
91+
"GetObjectLegalHold",
92+
"GetObjectRetention",
93+
"GetPublicAccessBlock",
94+
"ListBucketAnalyticsConfigurations",
95+
"ListBucketIntelligentTieringConfigurations",
96+
"ListBucketInventoryConfigurations",
97+
"ListBucketMetricsConfigurations",
98+
"ListMultipartUploads",
99+
"ListObjectAnnotations",
100+
"ListParts",
101+
"PutBucketAbac",
102+
"PutBucketAccelerateConfiguration",
103+
"PutBucketCors",
104+
"PutBucketIntelligentTieringConfiguration",
105+
"PutBucketInventoryConfiguration",
106+
"PutBucketLifecycleConfiguration",
107+
"PutBucketLogging",
108+
"PutBucketMetricsConfiguration",
109+
"PutBucketNotificationConfiguration",
110+
"PutBucketOwnershipControls",
111+
"PutBucketReplication",
112+
"PutBucketRequestPayment",
113+
"PutBucketTagging",
114+
"PutBucketVersioning",
115+
"PutBucketWebsite",
116+
"PutObjectAnnotation",
117+
"PutObjectLegalHold",
118+
"PutObjectLockConfiguration",
119+
"PutPublicAccessBlock",
120+
"RenameObject",
121+
"RestoreObject",
122+
"UpdateBucketMetadataAnnotationTableConfiguration",
123+
"UpdateBucketMetadataInventoryTableConfiguration",
124+
"UpdateBucketMetadataJournalTableConfiguration",
125+
"UpdateObjectEncryption",
126+
"WriteGetObjectResponse"
127+
);
128+
129+
// Wave 2: 600M–5B req/week. Low-medium risk.
130+
private static final Set<String> WAVE_2_OPERATIONS = Set.of(
131+
"GetBucketAccelerateConfiguration",
132+
"GetBucketCors",
133+
"GetBucketLifecycleConfiguration",
134+
"GetBucketLogging",
135+
"GetBucketNotificationConfiguration",
136+
"GetBucketOwnershipControls",
137+
"GetBucketPolicyStatus",
138+
"GetBucketReplication",
139+
"GetBucketRequestPayment",
140+
"GetBucketVersioning",
141+
"GetBucketWebsite",
142+
"GetObjectLockConfiguration",
143+
"GetPublicAccessBlock",
144+
"ListBuckets",
145+
"ListDirectoryBuckets",
146+
"PutBucketAcl",
147+
"PutBucketEncryption",
148+
"PutBucketPolicy"
149+
);
150+
151+
// Wave 3: 5B–20B req/week. Medium risk.
152+
private static final Set<String> WAVE_3_OPERATIONS = Set.of(
153+
"CreateMultipartUpload",
154+
"DeleteObjects",
155+
"GetBucketAcl",
156+
"GetBucketEncryption",
157+
"GetBucketLocation",
158+
"GetObjectTagging",
159+
"HeadBucket",
160+
"ListObjectVersions",
161+
"PutObjectRetention",
162+
"PutObjectTagging"
163+
);
22164

23-
// list of operations for which this customization is valid.
24-
private static Set<String> customizedOperations = SetUtils.of(
25-
"CopyObject", "UploadPartCopy", "CompleteMultipartUpload");
165+
// Wave 4: 50B+ req/week. Medium-high risk (volume).
166+
private static final Set<String> WAVE_4_OPERATIONS = Set.of(
167+
"CreateBucket",
168+
"DeleteObject",
169+
"HeadObject",
170+
"ListObjects",
171+
"ListObjectsV2",
172+
"PutObject",
173+
"UploadPart"
174+
);
175+
176+
/**
177+
* Combined set of all currently enabled operations. To enable a wave,
178+
* add it to this set. To finish rollout, delete all wave sets and the
179+
* isInEnabledWave check entirely.
180+
*/
181+
private static final Set<String> ENABLED_OPERATIONS = buildEnabledOperations();
182+
183+
private static Set<String> buildEnabledOperations() {
184+
Set<String> enabled = new HashSet<>(ORIGINAL_OPERATIONS);
185+
enabled.addAll(WAVE_1_OPERATIONS);
186+
// enabled.addAll(WAVE_2_OPERATIONS);
187+
// enabled.addAll(WAVE_3_OPERATIONS);
188+
// enabled.addAll(WAVE_4_OPERATIONS);
189+
return Set.copyOf(enabled);
190+
}
26191

27192
@Override
28193
public byte getOrder() {
@@ -33,7 +198,7 @@ public byte getOrder() {
33198

34199
@Override
35200
public List<RuntimeClientPlugin> getClientPlugins() {
36-
return ListUtils.of(
201+
return List.of(
37202
RuntimeClientPlugin.builder()
38203
.operationPredicate(S3ErrorWith200Status::supports200Error)
39204
.registerMiddleware(MiddlewareRegistrar.builder()
@@ -44,13 +209,36 @@ public List<RuntimeClientPlugin> getClientPlugins() {
44209
);
45210
}
46211

47-
// returns true if the operation supports error response with 200 ok status code
48-
private static boolean supports200Error(Model model, ServiceShape service, OperationShape operation){
212+
/**
213+
* Returns true if the operation supports error response with 200 ok status code.
214+
*
215+
* Uses a two-layer check:
216+
* 1. Model-driven: excludes operations with @httpPayload targeting @streaming or string.
217+
* 2. Wave gate: only enables operations that are in a currently-active wave.
218+
*
219+
* Once all waves are shipped and baked, remove the wave gate (step 2) and
220+
* the model-driven logic alone becomes the final behavior.
221+
*/
222+
private static boolean supports200Error(Model model, ServiceShape service, OperationShape operation) {
49223
if (!isS3Service(model, service)) {
50224
return false;
51225
}
52226

53-
return customizedOperations.contains(operation.getId().getName(service));
227+
// Model-driven exclusion: this is the correct final logic.
228+
Optional<ShapeId> output = operation.getOutput();
229+
if (output.isPresent()) {
230+
StructureShape outputShape = model.expectShape(output.get(), StructureShape.class);
231+
boolean excluded = outputShape.getAllMembers().values().stream()
232+
.filter(member -> member.hasTrait(HttpPayloadTrait.class))
233+
.map(member -> model.expectShape(member.getTarget()))
234+
.anyMatch(target -> target.hasTrait(StreamingTrait.class) || target.isStringShape());
235+
if (excluded) {
236+
return false;
237+
}
238+
}
239+
240+
// Wave gate: remove this check once all waves are shipped and baked.
241+
return ENABLED_OPERATIONS.contains(operation.getId().getName(service));
54242
}
55243

56244
// returns true if service is s3

‎service/s3/api_op_AbortMultipartUpload.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_CreateBucketMetadataConfiguration.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_CreateBucketMetadataTableConfiguration.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_CreateSession.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_DeleteBucket.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_DeleteBucketAnalyticsConfiguration.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_DeleteBucketCors.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎service/s3/api_op_DeleteBucketEncryption.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)