github-advanced-security[bot] commented on code in PR #19892:
URL: https://github.com/apache/druid/pull/19892#discussion_r3724532246
##########
cloud/aws-common/src/test/java/org/apache/druid/common/aws/AWSClientConfigTest.java:
##########
@@ -39,84 +66,144 @@
}
@Test
- public void testDefaultCrossRegionAccessEnabled() throws Exception
+ public void testDefaultRetryModeIsStandard()
{
- AWSClientConfig config = MAPPER.readValue("{}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertEquals(AWSClientConfig.RetryMode.STANDARD,
config.getRetryMode());
+ Assertions.assertInstanceOf(StandardRetryStrategy.class,
config.getRetryStrategy());
}
- @Test
- public void testCrossRegionAccessEnabledExplicitlySet() throws Exception
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("retryModeStrategies")
+ public void testEachRetryModeBuildsItsStrategy(
+ AWSClientConfig.RetryMode mode,
+ Class<? extends RetryStrategy> expected
+ )
{
- AWSClientConfig config = MAPPER.readValue("{\"crossRegionAccessEnabled\":
true}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+ Assertions.assertInstanceOf(expected, mode.createStrategy());
}
+ /**
+ * Guards {@link #retryModeStrategies} against a mode being added without a
strategy expectation.
+ */
@Test
- public void testNewConfigTakesPrecedenceOverDeprecatedWhenBothSet() throws
Exception
+ public void testEveryRetryModeHasAStrategyExpectation()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true,
\"crossRegionAccessEnabled\": false}",
- AWSClientConfig.class
+ Assertions.assertEquals(AWSClientConfig.RetryMode.values().length,
retryModeStrategies().count());
+ }
+
+ private static Stream<Arguments> retryModeStrategies()
+ {
+ return Stream.of(
+ Arguments.of(AWSClientConfig.RetryMode.STANDARD,
StandardRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.ADAPTIVE,
AdaptiveRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.LEGACY,
LegacyRetryStrategy.class)
);
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"adaptive", "ADAPTIVE", "Adaptive"})
+ public void testRetryModeParsingIsCaseInsensitive(String value)
+ {
+ Assertions.assertEquals(AWSClientConfig.RetryMode.ADAPTIVE,
AWSClientConfig.RetryMode.fromString(value));
}
@Test
- public void testNewConfigTrueWinsOverDeprecatedFalse() throws Exception
+ public void testRetryModeBindsFromItsProperty()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": false,
\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.ADAPTIVE,
+ bind(Map.of("retryMode", "adaptive")).getRetryMode()
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDeprecatedForceGlobalBucketAccessAloneTrue() throws Exception
+ public void testRetryModeSerializesToItsPropertyValue()
+ {
+ Assertions.assertEquals("adaptive",
MAPPER.convertValue(AWSClientConfig.RetryMode.ADAPTIVE, String.class));
+ }
+
+ /**
+ * Binding the config is the last point at which a bad mode can be reported
against the property that set it, so it
+ * has to fail here rather than when some client is first built.
+ */
+ @Test
+ public void testUnrecognizedRetryModeIsRejectedWhenConfigIsBound()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true}",
- AWSClientConfig.class
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> bind(Map.of("retryMode", "aggressive"))
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+
+ final Throwable rootCause = Throwables.getRootCause(e);
+ Assertions.assertInstanceOf(IAE.class, rootCause);
+ Assertions.assertTrue(rootCause.getMessage().contains("aggressive"));
}
@Test
- public void testDeprecatedNotSetFallsThroughToCrossRegion() throws Exception
+ public void testUnsetAttemptCountLeavesTheCountTheModeDefines()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertNull(config.getMaxRetryAttempts());
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.STANDARD.createStrategy().maxAttempts(),
+ config.getRetryStrategy().maxAttempts()
);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDefaultMaxConnectionsKeepsAwsSdkFloorOnSmallHost() throws
Exception
+ public void testConfiguredAttemptCountIsApplied()
{
- AWSClientConfig config = mapperWithRuntimeInfo(new
FixedProcessorsRuntimeInfo(8))
- .readValue("{}", AWSClientConfig.class);
- Assertions.assertEquals(50, config.getMaxConnections());
+ Assertions.assertEquals(8, bind(Map.of("maxRetryAttempts",
8)).getRetryStrategy().maxAttempts());
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("crossRegionAccessBindings")
+ public void testCrossRegionAccessResolution(Map<String, Object> properties,
boolean expected)
+ {
+ Assertions.assertEquals(expected,
bind(properties).isCrossRegionAccessEnabled());
+ }
+
+ private static Stream<Arguments> crossRegionAccessBindings()
+ {
+ return Stream.of(
+ Arguments.of(Map.of(), false),
+ Arguments.of(Map.of("crossRegionAccessEnabled", true), true),
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", true), true),
+ // the new property wins whichever way the two disagree
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", true,
"crossRegionAccessEnabled", false), false),
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", false,
"crossRegionAccessEnabled", true), true)
+ );
+ }
+
+ /**
+ * The deprecated property is only ever populated by its own key, so code
still reading it cannot be misled by the
+ * replacement being set.
+ */
@Test
- public void testDefaultMaxConnectionsScalesWithCoresOnLargeHost() throws
Exception
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedPropertyStaysUnsetWhenOnlyItsReplacementIsBound()
{
- AWSClientConfig config = mapperWithRuntimeInfo(new
FixedProcessorsRuntimeInfo(32))
- .readValue("{}", AWSClientConfig.class);
- Assertions.assertEquals(128, config.getMaxConnections());
+ Assertions.assertNull(bind(Map.of()).isForceGlobalBucketAccessEnabled());
Review Comment:
## CodeQL / Deprecated method or constructor invocation
Invoking [AWSClientConfig.isForceGlobalBucketAccessEnabled](1) should be
avoided because it has been deprecated.
[Show more
details](https://github.com/apache/druid/security/code-scanning/11635)
##########
cloud/aws-common/src/test/java/org/apache/druid/common/aws/AWSClientConfigTest.java:
##########
@@ -39,84 +66,144 @@
}
@Test
- public void testDefaultCrossRegionAccessEnabled() throws Exception
+ public void testDefaultRetryModeIsStandard()
{
- AWSClientConfig config = MAPPER.readValue("{}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertEquals(AWSClientConfig.RetryMode.STANDARD,
config.getRetryMode());
+ Assertions.assertInstanceOf(StandardRetryStrategy.class,
config.getRetryStrategy());
}
- @Test
- public void testCrossRegionAccessEnabledExplicitlySet() throws Exception
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("retryModeStrategies")
+ public void testEachRetryModeBuildsItsStrategy(
+ AWSClientConfig.RetryMode mode,
+ Class<? extends RetryStrategy> expected
+ )
{
- AWSClientConfig config = MAPPER.readValue("{\"crossRegionAccessEnabled\":
true}", AWSClientConfig.class);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+ Assertions.assertInstanceOf(expected, mode.createStrategy());
}
+ /**
+ * Guards {@link #retryModeStrategies} against a mode being added without a
strategy expectation.
+ */
@Test
- public void testNewConfigTakesPrecedenceOverDeprecatedWhenBothSet() throws
Exception
+ public void testEveryRetryModeHasAStrategyExpectation()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true,
\"crossRegionAccessEnabled\": false}",
- AWSClientConfig.class
+ Assertions.assertEquals(AWSClientConfig.RetryMode.values().length,
retryModeStrategies().count());
+ }
+
+ private static Stream<Arguments> retryModeStrategies()
+ {
+ return Stream.of(
+ Arguments.of(AWSClientConfig.RetryMode.STANDARD,
StandardRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.ADAPTIVE,
AdaptiveRetryStrategy.class),
+ Arguments.of(AWSClientConfig.RetryMode.LEGACY,
LegacyRetryStrategy.class)
);
- Assertions.assertFalse(config.isCrossRegionAccessEnabled());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"adaptive", "ADAPTIVE", "Adaptive"})
+ public void testRetryModeParsingIsCaseInsensitive(String value)
+ {
+ Assertions.assertEquals(AWSClientConfig.RetryMode.ADAPTIVE,
AWSClientConfig.RetryMode.fromString(value));
}
@Test
- public void testNewConfigTrueWinsOverDeprecatedFalse() throws Exception
+ public void testRetryModeBindsFromItsProperty()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": false,
\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.ADAPTIVE,
+ bind(Map.of("retryMode", "adaptive")).getRetryMode()
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDeprecatedForceGlobalBucketAccessAloneTrue() throws Exception
+ public void testRetryModeSerializesToItsPropertyValue()
+ {
+ Assertions.assertEquals("adaptive",
MAPPER.convertValue(AWSClientConfig.RetryMode.ADAPTIVE, String.class));
+ }
+
+ /**
+ * Binding the config is the last point at which a bad mode can be reported
against the property that set it, so it
+ * has to fail here rather than when some client is first built.
+ */
+ @Test
+ public void testUnrecognizedRetryModeIsRejectedWhenConfigIsBound()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"forceGlobalBucketAccessEnabled\": true}",
- AWSClientConfig.class
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> bind(Map.of("retryMode", "aggressive"))
);
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
+
+ final Throwable rootCause = Throwables.getRootCause(e);
+ Assertions.assertInstanceOf(IAE.class, rootCause);
+ Assertions.assertTrue(rootCause.getMessage().contains("aggressive"));
}
@Test
- public void testDeprecatedNotSetFallsThroughToCrossRegion() throws Exception
+ public void testUnsetAttemptCountLeavesTheCountTheModeDefines()
{
- AWSClientConfig config = MAPPER.readValue(
- "{\"crossRegionAccessEnabled\": true}",
- AWSClientConfig.class
+ final AWSClientConfig config = new AWSClientConfig();
+
+ Assertions.assertNull(config.getMaxRetryAttempts());
+ Assertions.assertEquals(
+ AWSClientConfig.RetryMode.STANDARD.createStrategy().maxAttempts(),
+ config.getRetryStrategy().maxAttempts()
);
- Assertions.assertNull(config.isForceGlobalBucketAccessEnabled());
- Assertions.assertTrue(config.isCrossRegionAccessEnabled());
}
@Test
- public void testDefaultMaxConnectionsKeepsAwsSdkFloorOnSmallHost() throws
Exception
+ public void testConfiguredAttemptCountIsApplied()
{
- AWSClientConfig config = mapperWithRuntimeInfo(new
FixedProcessorsRuntimeInfo(8))
- .readValue("{}", AWSClientConfig.class);
- Assertions.assertEquals(50, config.getMaxConnections());
+ Assertions.assertEquals(8, bind(Map.of("maxRetryAttempts",
8)).getRetryStrategy().maxAttempts());
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("crossRegionAccessBindings")
+ public void testCrossRegionAccessResolution(Map<String, Object> properties,
boolean expected)
+ {
+ Assertions.assertEquals(expected,
bind(properties).isCrossRegionAccessEnabled());
+ }
+
+ private static Stream<Arguments> crossRegionAccessBindings()
+ {
+ return Stream.of(
+ Arguments.of(Map.of(), false),
+ Arguments.of(Map.of("crossRegionAccessEnabled", true), true),
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", true), true),
+ // the new property wins whichever way the two disagree
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", true,
"crossRegionAccessEnabled", false), false),
+ Arguments.of(Map.of("forceGlobalBucketAccessEnabled", false,
"crossRegionAccessEnabled", true), true)
+ );
+ }
+
+ /**
+ * The deprecated property is only ever populated by its own key, so code
still reading it cannot be misled by the
+ * replacement being set.
+ */
@Test
- public void testDefaultMaxConnectionsScalesWithCoresOnLargeHost() throws
Exception
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedPropertyStaysUnsetWhenOnlyItsReplacementIsBound()
{
- AWSClientConfig config = mapperWithRuntimeInfo(new
FixedProcessorsRuntimeInfo(32))
- .readValue("{}", AWSClientConfig.class);
- Assertions.assertEquals(128, config.getMaxConnections());
+ Assertions.assertNull(bind(Map.of()).isForceGlobalBucketAccessEnabled());
+ Assertions.assertNull(bind(Map.of("crossRegionAccessEnabled",
true)).isForceGlobalBucketAccessEnabled());
Review Comment:
## CodeQL / Deprecated method or constructor invocation
Invoking [AWSClientConfig.isForceGlobalBucketAccessEnabled](1) should be
avoided because it has been deprecated.
[Show more
details](https://github.com/apache/druid/security/code-scanning/11636)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
