Allow SDK to build on JDK 17 #3037

millems · 2022-02-16T18:20:06Z

The following changes were required:

Update environment variable mocking approach to no longer rely on reflection. Instead, all environment variable reading in the SDK (via SystemSetttings) supports value overrides via a testing backdoor added to SystemSettingsUtils.
Update to Mockito 4.x from 1.x.
Do not rely on reflection for comparing Instants in protocol-test-core
Run tests with -XX:+AllowRedefinitionToAddDeleteMethods enabled, because blockhound requires it for Java 13+: OpenJDK 13 requires -XX:+AllowRedefinitionToAddDeleteMethods flag reactor/BlockHound#33

millems · 2022-02-16T18:21:05Z

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

+          <property name="message" value="NEVER use System.getenv or System.getProperty. Create and use a
+          SystemSetting, instead."/>
          <property name="ignoreComments" value="true"/>
        </module>

+        <module name="Regexp">
+            <property name="format" value="SystemSetting\s*(\.|::)\s*getStringValueFromEnvironmentVariable"/>
+            <property name="illegalPattern" value="true"/>
+            <property name="message" value="Are you being naughty and not creating a SystemSetting? Don't you know that
+            your customers want both system properties AND environment variables? Are you REALLY sure you want to use
+            this and not support a system property?"/>
+            <property name="ignoreComments" value="true"/>
+        </module>


Note for reviewers: This was modified to really push people to use SystemSetting, because that's the only mechanism that supports environment variable mocking, now. We expect everyone to create new SystemSettings to support both properties and environment variables. If they really want to break that, then they can use the static SystemSetting.getStringValueFromEnvironmentVariable (long to be annoying to write), and they still have to add a checkstyle suppression to do it.

millems · 2022-02-16T18:23:26Z

...ain/java/software/amazon/awssdk/auth/credentials/EnvironmentVariableCredentialsProvider.java

@@ -39,7 +39,7 @@ public static EnvironmentVariableCredentialsProvider create() {
    protected Optional<String> loadSetting(SystemSetting setting) {
        // CHECKSTYLE:OFF - Customers should be able to specify a credentials provider that only looks at the environment
        // variables, but not the system properties. For that reason, we're only checking the environment variable here.
-        return Optional.ofNullable(System.getenv(setting.environmentVariable()));
+        return SystemSetting.getStringValueFromEnvironmentVariable(setting.environmentVariable());


Note for reviewer: Non-Mockito change. This uses SystemSetting to support mocking.

millems · 2022-02-16T18:25:07Z

http-client-spi/src/main/java/software/amazon/awssdk/http/AbortableInputStream.java

+    /**
+     * Access the underlying delegate stream, for testing purposes.
+     */
+    @SdkTestInternalApi
+    public InputStream delegate() {
+        return in;
+    }


Note to reviewer: Our existing tests relied on reflection to access 'in', which does not work in JDK 17. Now, we expose it via a test API. This should be okay, since this is a protected (not public) API.

millems · 2022-02-16T18:26:42Z

...re/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java

+        connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) {
+            @Override
+            public int getResponseCode() {
+                throw new NullPointerException();
+            }


Note to reviewer: Mockito can no longer spy on classes loaded by the bootstrap class loader in JDK 17, so we had to do the method overriding manually.

millems · 2022-02-16T18:27:41Z

...on/awssdk/enhanced/dynamodb/converters/attribute/InstantAsStringAttributeConvertersTest.java

-    @Test
-    public void InstantAsStringAttributeConverterNotAcceptOffsetTimeTest() {
-        assertFails(() -> transformTo(CONVERTER, EnhancedAttributeValue.fromString("1988-05-21T00:12:00+01:00")
-                                                                       .toAttributeValue()));
-    }
-


Note to reviewer: JDK 17 now supports parsing offset times. There shouldn't be a problem supporting this now, so I removed the assertion that we do not support it.

millems · 2022-02-16T18:29:52Z

test/http-client-tests/pom.xml

+        <dependency>
+            <groupId>org.bouncycastle</groupId>
+            <artifactId>bcpkix-jdk15on</artifactId>
+            <version>1.70</version>
+            <scope>compile</scope>
+        </dependency>


Note to reviewer: Netty uses JDK internals to generate self-signed certs for our netty services we spin up for testing purposes. This no longer works on JDK 17, because those classes aren't exposed. We add this (test, since it's a module that's only used for testing) dependency on bouncycastle, which netty will automatically use instead of the JDK classes.

millems · 2022-02-16T18:31:35Z

.../java/software/amazon/awssdk/protocol/asserts/unmarshalling/UnmarshalledResultAssertion.java

@@ -62,7 +71,11 @@ private void assertFieldEquals(Field field, Object actual, Object expectedResult
            assertTrue(IOUtils.contentEquals((InputStream) field.get(expectedResult),
                                             (InputStream) field.get(actual)));
        } else {
-            assertReflectionEquals(field.get(expectedResult), field.get(actual));
+            Difference difference = CustomComparatorFactory.getComparator()


Note to reviewer: this fixes an issue where the comparator is using reflection to compare instants (which fails on JDK 17), because our comparator library is old enough that it doesn't support Instant types. (It hasn't been updated since 2017).

…loading via SystemSettings. This fixes an issue where the existing environment variable helpers did not work on Java 17, because it relied on reflectivly modifying the environment variable map directly.

sonarqubecloud · 2022-02-16T22:02:06Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

62.3% Coverage
0.0% Duplication

joviegas · 2022-02-17T07:37:16Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/Profile.java

@@ -108,7 +108,7 @@ private Boolean parseBooleanProperty(String propertyKey, String property) {
     * Retrieve an unmodifiable view of all of the properties currently in this profile.
     */
    public Map<String, String> properties() {
-        return properties;
+        return Collections.unmodifiableMap(properties);


Do we still require wrapping the input list as unmodifiableMap in builder line number 193 ?

Good catch! I didn't see that (and neither did spotbugs).

pom.xml

joviegas · 2022-02-17T08:06:13Z

.../java/software/amazon/awssdk/protocol/asserts/unmarshalling/UnmarshalledResultAssertion.java

+    private static class InstantComparator implements Comparator {
+        @Override
+        public boolean canCompare(Object left, Object right) {
+            return left instanceof Instant || right instanceof Instant;


Query :why isn't it ?

return left instanceof Instant && right instanceof Instant;

I had the same thought at first. I looked at other comparator implementations and they did ||. It actually makes sense after I thought about it. This comparator knows whether two instants are equal, but it also knows whether one instant is equal to a non-instant, so it makes sense to say it can compare an instant and a non-instant (to return false).

got it, thanks 👍

…4bb20707f Pull request: release <- staging/15799bce-3545-419d-8e68-bc74bb20707f

millems requested a review from a team as a code owner February 16, 2022 18:20

millems commented Feb 16, 2022

View reviewed changes

millems force-pushed the millem/jdk-17 branch from 0b63ca8 to 354c747 Compare February 16, 2022 18:38

millems added 2 commits February 16, 2022 13:03

Update environment variable helpers to mock out environment variable …

bfcf9ee

…loading via SystemSettings. This fixes an issue where the existing environment variable helpers did not work on Java 17, because it relied on reflectivly modifying the environment variable map directly.

Update Mockito to 4.x to allow building on JDK 17.

9c3c07c

millems force-pushed the millem/jdk-17 branch from db914da to e73d387 Compare February 16, 2022 21:03

Fixed some findbugs issues.

0227012

millems force-pushed the millem/jdk-17 branch from e73d387 to 0227012 Compare February 16, 2022 21:16

millems enabled auto-merge (squash) February 16, 2022 23:19

joviegas approved these changes Feb 17, 2022

View reviewed changes

millems merged commit 03f2db7 into master Feb 17, 2022

millems deleted the millem/jdk-17 branch October 19, 2022 19:35

aws-sdk-java-automation added a commit that referenced this pull request May 17, 2024

Merge pull request #3037 from aws/staging/15799bce-3545-419d-8e68-bc7…

5de3fdc

…4bb20707f Pull request: release <- staging/15799bce-3545-419d-8e68-bc74bb20707f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SDK to build on JDK 17 #3037

Allow SDK to build on JDK 17 #3037

millems commented Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

millems Feb 16, 2022

sonarqubecloud bot commented Feb 16, 2022

joviegas Feb 17, 2022

millems Feb 17, 2022

joviegas Feb 17, 2022

millems Feb 17, 2022

joviegas Feb 17, 2022

Allow SDK to build on JDK 17 #3037

Allow SDK to build on JDK 17 #3037

Conversation

millems commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment