gnodet-bot commented on code in PR #2223:
URL: https://github.com/apache/maven-resolver/pull/2223#discussion_r4348369159
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtilsTest.java:
##########
@@ -119,4 +124,38 @@ void getBaseUriWithNonAscii() throws URISyntaxException {
assertEquals("/räpo/", uri.getPath());
assertEquals("https://host.de/r%C3%A4po/";, uri.toASCIIString());
}
+
+ @Test
+ void getHttpServiceUnavailableCodesUsesDefaultCodes() {
+ DefaultRepositorySystemSession session = new
DefaultRepositorySystemSession();
+ RemoteRepository repository = new RemoteRepository.Builder("test",
"default", "https://example.com/";).build();
+
+ Set<Integer> codes =
HttpTransporterUtils.getHttpServiceUnavailableCodes(session, repository);
+
+ assertEquals(new HashSet<>(Arrays.asList(429, 502, 503, 504)), codes);
+ }
+
+ @Test
+ void getHttpServiceUnavailableCodesSupportsCustomConfiguration() {
+ DefaultRepositorySystemSession session = new
DefaultRepositorySystemSession();
+ RemoteRepository repository = new RemoteRepository.Builder("test",
"default", "https://example.com/";).build();
+
+
session.setConfigProperty(ConfigurationProperties.HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE,
"429,502");
+
+ Set<Integer> codes =
HttpTransporterUtils.getHttpServiceUnavailableCodes(session, repository);
+
+ assertEquals(new HashSet<>(Arrays.asList(429, 502)), codes);
+ }
+
+ @Test
+ void
getHttpServiceUnavailableCodesSupportsRepositorySpecificConfiguration() {
Review Comment:
ℹ️ **Pre-fix validation note** (low severity)
`getHttpServiceUnavailableCodesSupportsCustomConfiguration` and
`getHttpServiceUnavailableCodesSupportsRepositorySpecificConfiguration` both
set an explicit config value that bypasses
`DEFAULT_HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE` entirely. They would pass
unchanged with the old default `"429,503"` — they test the config-override
mechanism, not the fix itself.
`getHttpServiceUnavailableCodesUsesDefaultCodes` correctly validates the
fix. The other two are positive additions for coverage but aren't regression
tests for this change. No code change needed — just worth noting so maintainers
know which test actually guards the new default.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -359,7 +359,7 @@ public final class ConfigurationProperties {
*
* @since 1.9.16
*/
- public static final String DEFAULT_HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE
= "429,503";
+ public static final String DEFAULT_HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE
= "429,502,503,504";
Review Comment:
⚠️ **Stale Javadoc — update both constants**
The PR changes the default but leaves two Javadoc blocks stale:
1. `HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE` (a few lines above): still says
_"too many requests (examples: HTTP codes 429 and 503)"_ — 502 and 504 are now
included and have different semantics (Bad Gateway / Gateway Timeout, not rate
limiting).
2. `DEFAULT_HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE` (this constant): still
says _"Default value: "429,503"."_
Suggested update for this constant's Javadoc:
```suggestion
/**
* The default HTTP codes of remote server responses that should be
handled as "service unavailable" or
* "too many requests". Default value: "429,502,503,504".
*
* @since 1.9.16
*/
public static final String
DEFAULT_HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE = "429,502,503,504";
```
And update `HTTP_RETRY_HANDLER_SERVICE_UNAVAILABLE`'s Javadoc description to
replace _"too many requests (examples: HTTP codes 429 and 503)"_ with _"service
unavailable or too many requests (examples: HTTP codes 429, 502, 503, 504)"_.
--
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]
