Skip to content

Commit 922ccc3

Browse files
millemsmillems
committed
Add spotbugs rule validating that all CopyableBuilder fields are copied when ToCopyableBuilder.toBuilder is invoked.
Also fixed any violations that rule found.
1 parent be5f2c5 commit 922ccc3

File tree

26 files changed

+610
-36
lines changed

26 files changed

+610
-36
lines changed

‎build-tools/src/main/java/software/amazon/awssdk/buildtools/findbugs/ToBuilderIsCorrect.java

Lines changed: 456 additions & 0 deletions
Large diffs are not rendered by default.

‎build-tools/src/main/resources/findbugs.xml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,8 @@
1515

1616
<FindbugsPlugin>
1717
<Detector class="software.amazon.awssdk.buildtools.findbugs.DisallowMethodCall" speed="fast" />
18+
<Detector class="software.amazon.awssdk.buildtools.findbugs.ToBuilderIsCorrect" speed="fast" />
19+
1820
<BugPattern abbrev="BM" type="SDK_BAD_METHOD_CALL" category="PERFORMANCE" />
19-
</FindbugsPlugin>
21+
<BugPattern abbrev="BTB" type="BAD_TO_BUILDER" category="BUG" />
22+
</FindbugsPlugin>

‎build-tools/src/main/resources/messages.xml

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@
1414
-->
1515

1616
<MessageCollection>
17-
1817
<Detector class="software.amazon.awssdk.buildtools.findbugs.DisallowMethodCall" >
1918
<Details>This detector checks for method calls that are not allowed for use.</Details>
2019
</Detector>
20+
<Detector class="software.amazon.awssdk.buildtools.findbugs.ToBuilderIsCorrect" >
21+
<Details>This detector checks for correct CopyableBuilder definition.</Details>
22+
</Detector>
2123

2224
<BugPattern type="SDK_BAD_METHOD_CALL">
2325
<ShortDescription>Bad method call</ShortDescription>
@@ -41,6 +43,11 @@
4143
]]>
4244
</Details>
4345
</BugPattern>
46+
<BugPattern type="BAD_TO_BUILDER">
47+
<ShortDescription>Bad toBuilder implementation</ShortDescription>
48+
<LongDescription>Bad toBuilder implementation. See the SpotBugs logs for problem details.</LongDescription>
49+
<Details>Bad toBuilder implementation. See the SpotBugs logs for problem details.</Details>
50+
</BugPattern>
4451

45-
<BugCode abbrev="BM">Bad method</BugCode>
52+
<BugCode abbrev="BTB">Bad toBuilder implementation</BugCode>
4653
</MessageCollection>

‎core/annotations/src/main/java/software/amazon/awssdk/annotations/ToBuilderIgnoreField.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
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+
16+
package software.amazon.awssdk.annotations;
17+
18+
import java.lang.annotation.ElementType;
19+
import java.lang.annotation.Retention;
20+
import java.lang.annotation.RetentionPolicy;
21+
import java.lang.annotation.Target;
22+
23+
/**
24+
* Used to suppress certain fields from being considered in the spot-bugs rule for toBuilder(). This annotation must be
25+
* attached to the toBuilder() method to function.
26+
*/
27+
@Target(ElementType.METHOD)
28+
@Retention(RetentionPolicy.CLASS)
29+
@SdkProtectedApi
30+
public @interface ToBuilderIgnoreField {
31+
/**
32+
* Specify which fields to ignore in the to-builder spotbugs rule.
33+
*/
34+
String[] value();
35+
}

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,7 @@ public static FileTransformerConfiguration defaultCreateOrAppend() {
101101

102102
@Override
103103
public Builder toBuilder() {
104-
return new DefaultBuilder().fileWriteOption(fileWriteOption)
105-
.failureBehavior(failureBehavior);
104+
return new DefaultBuilder(this);
106105
}
107106

108107
@Override
@@ -188,6 +187,14 @@ private static class DefaultBuilder implements Builder {
188187
private FileWriteOption fileWriteOption;
189188
private FailureBehavior failureBehavior;
190189

190+
private DefaultBuilder() {
191+
}
192+
193+
private DefaultBuilder(FileTransformerConfiguration fileTransformerConfiguration) {
194+
this.fileWriteOption = fileTransformerConfiguration.fileWriteOption;
195+
this.failureBehavior = fileTransformerConfiguration.failureBehavior;
196+
}
197+
191198
@Override
192199
public Builder fileWriteOption(FileWriteOption fileWriteOption) {
193200
this.fileWriteOption = fileWriteOption;

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/client/config/ClientOverrideConfiguration.java

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.TreeMap;
2626
import java.util.function.Consumer;
2727
import software.amazon.awssdk.annotations.SdkPublicApi;
28+
import software.amazon.awssdk.annotations.ToBuilderIgnoreField;
2829
import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
2930
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
3031
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
@@ -78,16 +79,19 @@ private ClientOverrideConfiguration(Builder builder) {
7879
}
7980

8081
@Override
82+
@ToBuilderIgnoreField("advancedOptions")
8183
public Builder toBuilder() {
82-
return new DefaultClientOverrideConfigurationBuilder().advancedOptions(advancedOptions.toBuilder())
83-
.headers(headers)
84-
.retryPolicy(retryPolicy)
85-
.apiCallTimeout(apiCallTimeout)
86-
.apiCallAttemptTimeout(apiCallAttemptTimeout)
87-
.executionInterceptors(executionInterceptors)
88-
.defaultProfileFile(defaultProfileFile)
89-
.defaultProfileName(defaultProfileName)
90-
.executionAttributes(executionAttributes);
84+
return new DefaultClientOverrideConfigurationBuilder()
85+
.advancedOptions(advancedOptions.toBuilder())
86+
.headers(headers)
87+
.retryPolicy(retryPolicy)
88+
.apiCallTimeout(apiCallTimeout)
89+
.apiCallAttemptTimeout(apiCallAttemptTimeout)
90+
.executionInterceptors(executionInterceptors)
91+
.defaultProfileFile(defaultProfileFile)
92+
.defaultProfileName(defaultProfileName)
93+
.executionAttributes(executionAttributes)
94+
.metricPublishers(metricPublishers);
9195
}
9296

9397
/**
@@ -484,7 +488,7 @@ private static final class DefaultClientOverrideConfigurationBuilder implements
484488
private ProfileFile defaultProfileFile;
485489
private String defaultProfileName;
486490
private List<MetricPublisher> metricPublishers = new ArrayList<>();
487-
private ExecutionAttributes.Builder executionAttributesBuilder = ExecutionAttributes.builder();
491+
private ExecutionAttributes.Builder executionAttributes = ExecutionAttributes.builder();
488492

489493
@Override
490494
public Builder headers(Map<String, List<String>> headers) {
@@ -652,19 +656,19 @@ public List<MetricPublisher> metricPublishers() {
652656
@Override
653657
public Builder executionAttributes(ExecutionAttributes executionAttributes) {
654658
Validate.paramNotNull(executionAttributes, "executionAttributes");
655-
this.executionAttributesBuilder = executionAttributes.toBuilder();
659+
this.executionAttributes = executionAttributes.toBuilder();
656660
return this;
657661
}
658662

659663
@Override
660664
public <T> Builder putExecutionAttribute(ExecutionAttribute<T> executionAttribute, T value) {
661-
this.executionAttributesBuilder.put(executionAttribute, value);
665+
this.executionAttributes.put(executionAttribute, value);
662666
return this;
663667
}
664668

665669
@Override
666670
public ExecutionAttributes executionAttributes() {
667-
return executionAttributesBuilder.build();
671+
return executionAttributes.build();
668672
}
669673

670674
@Override

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/endpointdiscovery/EndpointDiscoveryRequest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ private BuilderImpl(EndpointDiscoveryRequest request) {
143143
this.identifiers = request.identifiers;
144144
this.cacheKey = request.cacheKey;
145145
this.required = request.required;
146+
this.defaultEndpoint = request.defaultEndpoint;
146147
}
147148

148149
@Override

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/http/ExecutionContext.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,12 @@ public static class Builder implements CopyableBuilder<Builder, ExecutionContext
9191
private Builder() {
9292
}
9393

94-
public Builder(ExecutionContext executionContext) {
94+
private Builder(ExecutionContext executionContext) {
9595
this.signer = executionContext.signer;
9696
this.interceptorContext = executionContext.interceptorContext;
9797
this.interceptorChain = executionContext.interceptorChain;
9898
this.executionAttributes = executionContext.executionAttributes;
99+
this.metricCollector = executionContext.metricCollector;
99100
}
100101

101102
public Builder interceptorContext(InterceptorContext interceptorContext) {

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionAttributes.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public static Builder builder() {
109109

110110
@Override
111111
public Builder toBuilder() {
112-
return builder().putAll(attributes);
112+
return new ExecutionAttributes.Builder(this);
113113
}
114114

115115
public ExecutionAttributes copy() {
@@ -170,6 +170,10 @@ public static final class Builder implements CopyableBuilder<ExecutionAttributes
170170
private Builder() {
171171
}
172172

173+
private Builder(ExecutionAttributes attributes) {
174+
this.executionAttributes.putAll(attributes.attributes);
175+
}
176+
173177
/**
174178
* Add a mapping between the provided key and value.
175179
*/

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/interceptor/DefaultFailedExecutionContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ public static final class Builder implements CopyableBuilder<Builder, DefaultFai
9191
private Builder() {
9292
}
9393

94-
public Builder(DefaultFailedExecutionContext defaultFailedExecutionContext) {
94+
private Builder(DefaultFailedExecutionContext defaultFailedExecutionContext) {
9595
this.exception = defaultFailedExecutionContext.exception;
9696
this.interceptorContext = defaultFailedExecutionContext.interceptorContext;
9797
}

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicy.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import java.util.Objects;
1919
import software.amazon.awssdk.annotations.Immutable;
2020
import software.amazon.awssdk.annotations.SdkPublicApi;
21+
import software.amazon.awssdk.annotations.ToBuilderIgnoreField;
2122
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
2223
import software.amazon.awssdk.core.internal.retry.SdkDefaultRetrySetting;
2324
import software.amazon.awssdk.core.retry.backoff.BackoffStrategy;
@@ -182,6 +183,7 @@ private RetryCondition generateAggregateRetryCondition() {
182183
}
183184

184185
@Override
186+
@ToBuilderIgnoreField("retryMode")
185187
public Builder toBuilder() {
186188
return builder(retryMode).additionalRetryConditionsAllowed(additionalRetryConditionsAllowed)
187189
.numRetries(numRetries)

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/FullJitterBackoffStrategy.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public Duration computeDelayBeforeNextRetry(RetryPolicyContext context) {
6767

6868
@Override
6969
public Builder toBuilder() {
70-
return builder().baseDelay(baseDelay).maxBackoffTime(maxBackoffTime);
70+
return new BuilderImpl(this);
7171
}
7272

7373
public static Builder builder() {
@@ -95,6 +95,11 @@ private static final class BuilderImpl implements Builder {
9595
private BuilderImpl() {
9696
}
9797

98+
private BuilderImpl(FullJitterBackoffStrategy strategy) {
99+
this.baseDelay = strategy.baseDelay;
100+
this.maxBackoffTime = strategy.maxBackoffTime;
101+
}
102+
98103
@Override
99104
public Builder baseDelay(Duration baseDelay) {
100105
this.baseDelay = baseDelay;

‎http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ProxyConfiguration.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ public Builder toBuilder() {
159159
.ntlmDomain(ntlmDomain)
160160
.ntlmWorkstation(ntlmWorkstation)
161161
.nonProxyHosts(nonProxyHosts)
162-
.preemptiveBasicAuthenticationEnabled(preemptiveBasicAuthenticationEnabled);
162+
.preemptiveBasicAuthenticationEnabled(preemptiveBasicAuthenticationEnabled)
163+
.useSystemPropertyValues(useSystemPropertyValues);
163164
}
164165

165166
/**

‎http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/ProxyConfiguration.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ public Builder toBuilder() {
125125
.endpoint(endpoint)
126126
.username(username)
127127
.password(password)
128-
.nonProxyHosts(nonProxyHosts);
128+
.nonProxyHosts(nonProxyHosts)
129+
.useSystemPropertyValues(useSystemPropertyValues);
129130
}
130131

131132
/**

‎pom.xml

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -575,10 +575,12 @@
575575
<exclude>*.internal.*</exclude>
576576
<exclude>software.amazon.awssdk.thirdparty.*</exclude>
577577

578-
<!-- Modified when making HttpCredentialsProvider a public API -->
579-
<exclude>software.amazon.awssdk.auth.credentials.ContainerCredentialsProvider</exclude>
580-
<exclude>software.amazon.awssdk.auth.credentials.InstanceProfileCredentialsProvider</exclude>
581-
<exclude>software.amazon.awssdk.auth.credentials.HttpCredentialsProvider</exclude>
578+
<!-- Fixed constructor visibility -->
579+
<exclude>software.amazon.awssdk.core.http.ExecutionContext$Builder</exclude>
580+
<exclude>software.amazon.awssdk.enhanced.dynamodb.update.AddAction$Builder</exclude>
581+
<exclude>software.amazon.awssdk.enhanced.dynamodb.update.DeleteAction$Builder</exclude>
582+
<exclude>software.amazon.awssdk.enhanced.dynamodb.update.RemoveAction$Builder</exclude>
583+
<exclude>software.amazon.awssdk.enhanced.dynamodb.update.SetAction$Builder</exclude>
582584
</excludes>
583585

584586
<ignoreMissingOldVersion>true</ignoreMissingOldVersion>

‎services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/EnhancedTypeDocumentConfiguration.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public int hashCode() {
7878

7979
@Override
8080
public Builder toBuilder() {
81-
return builder().preserveEmptyObject(preserveEmptyObject);
81+
return builder().preserveEmptyObject(preserveEmptyObject).ignoreNulls(ignoreNulls);
8282
}
8383

8484
public static Builder builder() {

‎services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/update/AddAction.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ public static final class Builder implements CopyableBuilder<Builder, AddAction>
143143
private Map<String, String> expressionNames;
144144
private Map<String, AttributeValue> expressionValues;
145145

146+
private Builder() {
147+
}
148+
146149
/**
147150
* A string expression representing the attribute to be acted upon
148151
*/

‎services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/update/DeleteAction.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ public static final class Builder implements CopyableBuilder<Builder, DeleteActi
143143
private Map<String, String> expressionNames;
144144
private Map<String, AttributeValue> expressionValues;
145145

146+
private Builder() {
147+
}
148+
146149
/**
147150
* A string expression representing the attribute to be acted upon
148151
*/

‎services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/update/RemoveAction.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ public static final class Builder implements CopyableBuilder<Builder, RemoveActi
113113
private String path;
114114
private Map<String, String> expressionNames;
115115

116+
private Builder() {
117+
}
118+
116119
/**
117120
* A string expression representing the attribute to remove
118121
*/

‎services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/update/SetAction.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,9 @@ public static final class Builder implements CopyableBuilder<Builder, SetAction>
156156
private Map<String, String> expressionNames;
157157
private Map<String, AttributeValue> expressionValues;
158158

159+
private Builder() {
160+
}
161+
159162
/**
160163
* A string expression representing the attribute to be acted upon
161164
*/

‎services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/DownloadDirectoryRequest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,7 @@ private DefaultBuilder(DownloadDirectoryRequest request) {
400400
this.filter = request.filter;
401401
this.downloadFileRequestTransformer = request.downloadFileRequestTransformer;
402402
this.listObjectsRequestTransformer = request.listObjectsRequestTransformer;
403+
this.delimiter = request.delimiter;
403404
}
404405

405406
@Override

‎services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/DownloadRequest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public final class DownloadRequest<ReturnT>
5151
private DownloadRequest(DefaultTypedBuilder<ReturnT> builder) {
5252
this.responseTransformer = Validate.paramNotNull(builder.responseTransformer, "responseTransformer");
5353
this.getObjectRequest = Validate.paramNotNull(builder.getObjectRequest, "getObjectRequest");
54-
this.overrideConfiguration = builder.configuration;
54+
this.overrideConfiguration = builder.overrideConfiguration;
5555
}
5656

5757
/**
@@ -321,7 +321,7 @@ default TypedBuilder<T> overrideConfiguration(
321321

322322
private static class DefaultTypedBuilder<T> implements TypedBuilder<T> {
323323
private GetObjectRequest getObjectRequest;
324-
private TransferRequestOverrideConfiguration configuration;
324+
private TransferRequestOverrideConfiguration overrideConfiguration;
325325
private AsyncResponseTransformer<GetObjectResponse, T> responseTransformer;
326326

327327
private DefaultTypedBuilder() {
@@ -330,6 +330,7 @@ private DefaultTypedBuilder() {
330330
private DefaultTypedBuilder(DownloadRequest<T> request) {
331331
this.getObjectRequest = request.getObjectRequest;
332332
this.responseTransformer = request.responseTransformer;
333+
this.overrideConfiguration = request.overrideConfiguration;
333334
}
334335

335336
@Override
@@ -340,7 +341,7 @@ public TypedBuilder<T> getObjectRequest(GetObjectRequest getObjectRequest) {
340341

341342
@Override
342343
public DefaultTypedBuilder<T> overrideConfiguration(TransferRequestOverrideConfiguration configuration) {
343-
this.configuration = configuration;
344+
this.overrideConfiguration = configuration;
344345
return this;
345346
}
346347

‎services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/progress/TransferListenerFailedContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ public static final class Builder implements CopyableBuilder<Builder, TransferLi
9191
private Builder() {
9292
}
9393

94-
public Builder(TransferListenerFailedContext failedContext) {
94+
private Builder(TransferListenerFailedContext failedContext) {
9595
this.exception = failedContext.exception;
9696
this.transferContext = failedContext.transferContext;
9797
}

0 commit comments

Comments
 (0)