Skip to content

Commit 7fcc7ae

Browse files
authored
Remove unnecessary dependencies on JDK modules not in java.base. (#3133)
This didn't remove all of the dependencies on such modules, just the unnecessary ones. Also added a checkstyle rule to prevent such dependencies in the future (except where they are intentional).
1 parent e389126 commit 7fcc7ae

32 files changed

+230
-247
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"category": "AWS SDK for Java v2",
3+
"contributor": "",
4+
"type": "feature",
5+
"description": "Remove unnecessary dependencies on JDK modules not in java.base. This didn't remove all of the dependencies on such modules, just the unnecessary ones."
6+
}
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
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.buildtools.checkstyle;
17+
18+
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
19+
import com.puppycrawl.tools.checkstyle.api.DetailAST;
20+
import com.puppycrawl.tools.checkstyle.api.FullIdent;
21+
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
22+
import java.util.Arrays;
23+
import java.util.HashMap;
24+
import java.util.HashSet;
25+
import java.util.List;
26+
import java.util.Set;
27+
import java.util.regex.Pattern;
28+
29+
/**
30+
* Verify that we're not using classes in rt.jar that aren't exported via the java.base module.
31+
*
32+
* If anything fails this check, it increases our module dependencies. If you absolutely must use one of these
33+
* (e.g. java.beans) because it's fundamental to your functionality, you can suppress this checkstyle rule via
34+
* {@link #setLegalPackages(String...)}, but know that it is not free - you're essentially adding a dependency
35+
* for customers that use the module path.
36+
*/
37+
public class NonJavaBaseModuleCheck extends AbstractCheck {
38+
private static final List<String> ILLEGAL_PACKAGES = Arrays.asList(
39+
"java",
40+
"javax",
41+
"sun",
42+
"apple",
43+
"com.apple",
44+
"com.oracle");
45+
46+
private static final Set<String> LEGAL_PACKAGES = new HashSet<>(Arrays.asList(
47+
"java.io",
48+
"java.lang",
49+
"java.lang.annotation",
50+
"java.lang.invoke",
51+
"java.lang.module",
52+
"java.lang.ref",
53+
"java.lang.reflect",
54+
"java.math",
55+
"java.net",
56+
"java.net.spi",
57+
"java.nio",
58+
"java.nio.channels",
59+
"java.nio.channels.spi",
60+
"java.nio.charset",
61+
"java.nio.charset.spi",
62+
"java.nio.file",
63+
"java.nio.file.attribute",
64+
"java.nio.file.spi",
65+
"java.security",
66+
"java.security.acl",
67+
"java.security.cert",
68+
"java.security.interfaces",
69+
"java.security.spec",
70+
"java.text",
71+
"java.text.spi",
72+
"java.time",
73+
"java.time.chrono",
74+
"java.time.format",
75+
"java.time.temporal",
76+
"java.time.zone",
77+
"java.util",
78+
"java.util.concurrent",
79+
"java.util.concurrent.atomic",
80+
"java.util.concurrent.locks",
81+
"java.util.function",
82+
"java.util.jar",
83+
"java.util.regex",
84+
"java.util.spi",
85+
"java.util.stream",
86+
"java.util.jar",
87+
"java.util.zip",
88+
"javax.crypto",
89+
"javax.crypto.interfaces",
90+
"javax.crypto.spec",
91+
"javax.net",
92+
"javax.net.ssl",
93+
"javax.security.auth",
94+
"javax.security.auth.callback",
95+
"javax.security.auth.login",
96+
"javax.security.auth.spi",
97+
"javax.security.auth.x500",
98+
"javax.security.cert"));
99+
100+
private static final Pattern CLASSNAME_START_PATTERN = Pattern.compile("[A-Z]");
101+
102+
private String currentSdkPackage;
103+
104+
private HashMap<String, Set<String>> additionalLegalPackagesBySdkPackage = new HashMap<>();
105+
106+
/**
107+
* Additional legal packages are formatted as "sdk.package.name:jdk.package.name,sdk.package.name2:jdk.package.name2".
108+
* Multiple SDK packages can be repeated.
109+
*/
110+
public void setLegalPackages(String... legalPackages) {
111+
for (String additionalLegalPackage : legalPackages) {
112+
String[] splitPackage = additionalLegalPackage.split(":", 2);
113+
if (splitPackage.length != 2) {
114+
throw new IllegalArgumentException("Invalid legal package definition '" + additionalLegalPackage + "'. Expected"
115+
+ " format is sdk.package.name:jdk.package.name");
116+
}
117+
118+
this.additionalLegalPackagesBySdkPackage.computeIfAbsent(splitPackage[0], k -> new HashSet<>())
119+
.add(splitPackage[1]);
120+
}
121+
}
122+
123+
@Override
124+
public int[] getDefaultTokens() {
125+
return getRequiredTokens();
126+
}
127+
128+
@Override
129+
public int[] getAcceptableTokens() {
130+
return getRequiredTokens();
131+
}
132+
133+
@Override
134+
public int[] getRequiredTokens() {
135+
return new int[] { TokenTypes.IMPORT, TokenTypes.STATIC_IMPORT, TokenTypes.PACKAGE_DEF };
136+
}
137+
138+
@Override
139+
public void visitToken(DetailAST ast) {
140+
if (ast.getType() == TokenTypes.PACKAGE_DEF) {
141+
handlePackageDefToken(ast);
142+
return;
143+
}
144+
145+
handleImportToken(ast);
146+
}
147+
148+
private void handlePackageDefToken(DetailAST ast) {
149+
this.currentSdkPackage = FullIdent.createFullIdent(ast.getLastChild().getPreviousSibling()).getText();
150+
}
151+
152+
private void handleImportToken(DetailAST ast) {
153+
FullIdent importIdentifier;
154+
if (ast.getType() == TokenTypes.IMPORT) {
155+
importIdentifier = FullIdent.createFullIdentBelow(ast);
156+
} else {
157+
importIdentifier = FullIdent.createFullIdent(ast.getFirstChild().getNextSibling());
158+
}
159+
160+
String importText = importIdentifier.getText();
161+
if (isIllegalImport(importText) && !isLegalImport(importText)) {
162+
log(ast, "Import '" + importText + "' uses a JDK class that is not in java.base. This essentially adds an "
163+
+ "additional module dependency. Don't suppress this rule unless it's absolutely required, and only "
164+
+ "suppress the specific package you need via checkstyle.xml instead of suppressing the entire rule.");
165+
}
166+
}
167+
168+
private boolean isIllegalImport(String importText) {
169+
for (String illegalPackage : ILLEGAL_PACKAGES) {
170+
if (importText.startsWith(illegalPackage + ".")) {
171+
return true;
172+
}
173+
}
174+
175+
return false;
176+
}
177+
178+
private boolean isLegalImport(String importText) {
179+
String importPackageWithTrailingDot = CLASSNAME_START_PATTERN.split(importText, 2)[0];
180+
String importPackage = importText.substring(0, importPackageWithTrailingDot.length() - 1);
181+
182+
if (LEGAL_PACKAGES.contains(importPackage)) {
183+
return true;
184+
}
185+
186+
if (additionalLegalPackagesBySdkPackage.entrySet()
187+
.stream()
188+
.anyMatch(e -> currentSdkPackage.startsWith(e.getKey()) &&
189+
e.getValue().contains(importPackage))) {
190+
return true;
191+
}
192+
193+
return false;
194+
}
195+
}

build-tools/src/main/resources/software/amazon/awssdk/checkstyle-suppressions.xml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,7 @@
5050
<!-- Ignore usage of sslContext.newHandler for NettyUtils.!-->
5151
<suppress checks="Regexp"
5252
files=".*NettyUtils\.java$"/>
53+
54+
<!-- Allow non-java.base usage in tests -->
55+
<suppress checks="software.amazon.awssdk.buildtools.checkstyle.NonJavaBaseModuleCheck" files=".*testutils.*"/>
5356
</suppressions>

build-tools/src/main/resources/software/amazon/awssdk/checkstyle.xml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,18 @@
431431
<property name="packageToCheck" value="software.amazon.awssdk.http.nio.netty" />
432432
<property name="illegalPkgs" value="software.amazon.awssdk.utils.Logger,org.slf4j.Logger"/>
433433
</module>
434+
435+
<module name="software.amazon.awssdk.buildtools.checkstyle.NonJavaBaseModuleCheck">
436+
<!--
437+
codegen: Allowed to use classes from java.compiler, because poet requires them.
438+
aws-query-protocol: Allowed to use classes from java.xml for XML parsing.
439+
protocol-tests-core: Allows to use classes from java.xml for XML assertions.
440+
dynamodb-enhanced: Allowed to use classes from java.beans for bean processing.
441+
release-scripts: Allowed to use classes from java.xml for XML writing.
442+
sdk-benchmarks: Allowed to use classes from javax.servlet.http for benchmark servlets.
443+
-->
444+
<property name="legalPackages" value="software.amazon.awssdk.codegen:javax.lang.model, software.amazon.awssdk.codegen:javax.lang.model.element, software.amazon.awssdk.codegen:javax.lang.model.type, software.amazon.awssdk.protocols.query:javax.xml.stream, software.amazon.awssdk.protocols.query:javax.xml.stream.events, software.amazon.awssdk.protocol.asserts.marshalling:javax.xml, software.amazon.awssdk.protocol.asserts.marshalling:javax.xml.parsers, software.amazon.awssdk.protocol.asserts.marshalling:javax.xml.transform, software.amazon.awssdk.protocol.asserts.marshalling:javax.xml.transform.dom, software.amazon.awssdk.protocol.asserts.marshalling:javax.xml.transform.stream, software.amazon.awssdk.enhanced.dynamodb.mapper:java.beans, software.amazon.awssdk.release:javax.xml, software.amazon.awssdk.release:javax.xml.parsers, software.amazon.awssdk.release:javax.xml.transform, software.amazon.awssdk.release:javax.xml.xpath, software.amazon.awssdk.release:javax.xml.transform.dom, software.amazon.awssdk.release:javax.xml.transform.stream, software.amazon.awssdk.benchmark:javax.servlet.http"/>
445+
</module>
434446
</module>
435447

436448
<!-- Enforce maximum line lengths. -->

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/AbstractMemberSetters.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,19 @@
1515

1616
package software.amazon.awssdk.codegen.poet.model;
1717

18+
import static java.util.Collections.emptyList;
19+
import static java.util.Collections.singleton;
1820
import static software.amazon.awssdk.codegen.poet.model.TypeProvider.ShapeTransformation.USE_BUILDER_IMPL;
1921

22+
import com.squareup.javapoet.AnnotationSpec;
2023
import com.squareup.javapoet.ClassName;
2124
import com.squareup.javapoet.CodeBlock;
2225
import com.squareup.javapoet.MethodSpec;
2326
import com.squareup.javapoet.ParameterSpec;
2427
import com.squareup.javapoet.TypeName;
28+
// CHECKSTYLE:OFF java.beans is required for services that use names starting with 'set' to fix bean-based marshalling.
2529
import java.beans.Transient;
30+
// CHECKSTYLE:ON
2631
import java.util.Optional;
2732
import java.util.stream.Collectors;
2833
import javax.lang.model.element.Modifier;
@@ -85,11 +90,19 @@ protected MethodSpec.Builder fluentSetterBuilder(String methodName, ParameterSpe
8590
return MethodSpec.methodBuilder(methodName)
8691
.addParameter(setterParam)
8792
.addAnnotation(Override.class)
88-
.addAnnotation(Transient.class)
93+
.addAnnotations(maybeTransient(methodName))
8994
.returns(returnType)
9095
.addModifiers(Modifier.PUBLIC, Modifier.FINAL);
9196
}
9297

98+
private Iterable<AnnotationSpec> maybeTransient(String methodName) {
99+
if (methodName.startsWith("set")) {
100+
return singleton(AnnotationSpec.builder(Transient.class).build());
101+
}
102+
103+
return emptyList();
104+
}
105+
93106
protected MethodSpec.Builder beanStyleSetterBuilder() {
94107
return beanStyleSetterBuilder(memberAsBeanStyleParameter(), memberModel().getBeanStyleSetterMethodName());
95108
}

0 commit comments

Comments
 (0)