This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit de092eab6774da5bae1e731875a91bd0866f16dc
Author: Benoit Tellier <btell...@linagora.com>
AuthorDate: Mon Sep 9 09:30:19 2019 +0700
JAMES-2813 Enhance Duration parser negative number parsing
This enables a nicer error message upon negative numbers, avoiding hacks
on the caller side.
---
.../QuotaMailingListenerConfigurationTest.java | 3 ++-
.../java/org/apache/james/util/DurationParser.java | 19 ++++++++++++++-----
.../org/apache/james/util/DurationParserTest.java | 13 +++++++------
.../transport/mailets/remote/delivery/DelayTest.java | 8 +++++---
.../org/apache/james/webadmin/routes/TasksRoutes.java | 19 +++++++++----------
.../apache/james/webadmin/routes/TasksRoutesTest.java | 7 ++++---
6 files changed, 41 insertions(+), 28 deletions(-)
diff --git
a/mailbox/plugin/quota-mailing/src/test/java/org/apache/james/mailbox/quota/mailing/QuotaMailingListenerConfigurationTest.java
b/mailbox/plugin/quota-mailing/src/test/java/org/apache/james/mailbox/quota/mailing/QuotaMailingListenerConfigurationTest.java
index cff7d75..919c2c7 100644
---
a/mailbox/plugin/quota-mailing/src/test/java/org/apache/james/mailbox/quota/mailing/QuotaMailingListenerConfigurationTest.java
+++
b/mailbox/plugin/quota-mailing/src/test/java/org/apache/james/mailbox/quota/mailing/QuotaMailingListenerConfigurationTest.java
@@ -174,7 +174,8 @@ public class QuotaMailingListenerConfigurationTest {
"<configuration><gracePeriod>-12
ms</gracePeriod></configuration>"));
assertThatThrownBy(() ->
QuotaMailingListenerConfiguration.from(xmlConfiguration))
- .isInstanceOf(NumberFormatException.class);
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Duration amount should be positive");
}
@Test
diff --git
a/server/container/util/src/main/java/org/apache/james/util/DurationParser.java
b/server/container/util/src/main/java/org/apache/james/util/DurationParser.java
index b78fd6e..a0d61a6 100644
---
a/server/container/util/src/main/java/org/apache/james/util/DurationParser.java
+++
b/server/container/util/src/main/java/org/apache/james/util/DurationParser.java
@@ -27,12 +27,13 @@ import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
public class DurationParser {
- private static final String PATTERN_STRING =
"\\s*([0-9]+)\\s*([a-z,A-Z]*)\\s*";
+ private static final String PATTERN_STRING =
"\\s*(-?[0-9]+)\\s*([a-z,A-Z]*)\\s*";
private static final int AMOUNT = 1;
private static final int UNIT = 2;
@@ -87,16 +88,24 @@ public class DurationParser {
public static Duration parse(String rawString, ChronoUnit defaultUnit)
throws NumberFormatException {
Matcher res = PATTERN.matcher(rawString);
if (res.matches()) {
- if (res.group(AMOUNT) != null && res.group(UNIT) != null) {
+ String unitAsString = res.group(UNIT);
+ String amountAsString = res.group(AMOUNT);
+ if (amountAsString != null && unitAsString != null) {
long time = Integer.parseInt(res.group(AMOUNT).trim());
- return parseUnitAsDuration(res.group(UNIT))
- .orElse(defaultUnit.getDuration())
- .multipliedBy(time);
+ Duration unitAsDuration =
parseUnitAsDuration(unitAsString).orElse(defaultUnit.getDuration());
+
+ return computeDuration(unitAsDuration, time);
}
}
throw new NumberFormatException("The supplied String is not a
supported format " + rawString);
}
+ private static Duration computeDuration(Duration unitAsDuration, long
time) {
+ Preconditions.checkArgument(time >= 0, "Duration amount should be
positive");
+
+ return unitAsDuration.multipliedBy(time);
+ }
+
private static Optional<Duration> parseUnitAsDuration(String unit) {
if (Strings.isNullOrEmpty(unit)) {
return Optional.empty();
diff --git
a/server/container/util/src/test/java/org/apache/james/util/DurationParserTest.java
b/server/container/util/src/test/java/org/apache/james/util/DurationParserTest.java
index e0b573c..37a5550 100644
---
a/server/container/util/src/test/java/org/apache/james/util/DurationParserTest.java
+++
b/server/container/util/src/test/java/org/apache/james/util/DurationParserTest.java
@@ -59,6 +59,13 @@ class DurationParserTest {
.isEqualTo(Duration.ofMinutes(2));
}
+ @Test
+ void parseShouldThrowOnNegativeValue() {
+ assertThatThrownBy(() -> DurationParser.parse("-2 minutes"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Duration amount should be positive");
+ }
+
@ParameterizedTest
@ValueSource(strings = {"2ms", "2", "2 ms", "2 msec", "2 msecs", "2 Ms"})
void parseShouldHandleMilliseconds(String input) {
@@ -140,12 +147,6 @@ class DurationParserTest {
}
@Test
- void parseShouldThrowWhenNegativeAmount() {
- assertThatThrownBy(() -> DurationParser.parse("-1 s"))
- .isInstanceOf(NumberFormatException.class);
- }
-
- @Test
void parseShouldThrowWhenZero() {
assertThat(DurationParser.parse("0 s"))
.isEqualTo(Duration.ofSeconds(0));
diff --git
a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/DelayTest.java
b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/DelayTest.java
index f619d28..dafaf83 100644
---
a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/DelayTest.java
+++
b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/DelayTest.java
@@ -20,6 +20,7 @@
package org.apache.james.transport.mailets.remote.delivery;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.time.Duration;
@@ -51,9 +52,10 @@ public class DelayTest {
}
@Test
- public void stringConstructorShouldThrowOnNegativeNumbers() throws
Exception {
- expectedException.expect(NumberFormatException.class);
- assertThat(Delay.from("-1s")).isEqualTo(new
Delay(Delay.DEFAULT_ATTEMPTS, Duration.ofMillis(0)));
+ public void stringConstructorShouldThrowOnNegativeNumbers() {
+ assertThatThrownBy(() -> Delay.from("-1s"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Duration amount should be positive");
}
@Test
diff --git
a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java
b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java
index 35db8b8..a01fc99 100644
---
a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java
+++
b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java
@@ -41,10 +41,10 @@ import org.apache.james.webadmin.dto.ExecutionDetailsDto;
import org.apache.james.webadmin.utils.ErrorResponder;
import org.apache.james.webadmin.utils.JsonTransformer;
import org.apache.james.webadmin.utils.Responses;
-
import org.eclipse.jetty.http.HttpStatus;
import com.google.common.base.Preconditions;
+
import io.swagger.annotations.Api;
import io.swagger.annotations.ApiImplicitParam;
import io.swagger.annotations.ApiImplicitParams;
@@ -192,28 +192,27 @@ public class TasksRoutes implements Routes {
private Duration getTimeout(Request req) {
try {
- Optional<String> requestTimeout =
Optional.ofNullable(req.queryParams("timeout"))
- .filter(parameter -> !parameter.isEmpty());
-
- requestTimeout.ifPresent(timeout ->
- Preconditions.checkState(!timeout.replaceAll(" ",
"").startsWith("-"), "Timeout should be positive"));
-
- Duration timeout = requestTimeout
+ Duration timeout = Optional.ofNullable(req.queryParams("timeout"))
+ .filter(parameter -> !parameter.isEmpty())
.map(rawString -> DurationParser.parse(rawString,
ChronoUnit.SECONDS))
.orElse(MAXIMUM_AWAIT_TIMEOUT);
- Preconditions.checkState(timeout.compareTo(MAXIMUM_AWAIT_TIMEOUT)
<= 0, "Timeout should not exceed one year");
+ assertDoesNotExceedMaximumTimeout(timeout);
return timeout;
} catch (Exception e) {
throw ErrorResponder.builder()
.statusCode(HttpStatus.BAD_REQUEST_400)
.cause(e)
.type(ErrorResponder.ErrorType.INVALID_ARGUMENT)
- .message("Invalid timeout " + e.getMessage())
+ .message("Invalid timeout")
.haltError();
}
}
+ private void assertDoesNotExceedMaximumTimeout(Duration timeout) {
+ Preconditions.checkState(timeout.compareTo(MAXIMUM_AWAIT_TIMEOUT) <=
0, "Timeout should not exceed 365 days");
+ }
+
private TaskExecutionDetails awaitTask(TaskId taskId, Duration timeout) {
try {
return taskManager.await(taskId, timeout);
diff --git
a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java
b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java
index f31ec43..ad77c96 100644
---
a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java
+++
b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java
@@ -22,7 +22,6 @@ package org.apache.james.webadmin.routes;
import static io.restassured.RestAssured.given;
import static io.restassured.RestAssured.when;
import static io.restassured.RestAssured.with;
-import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasSize;
@@ -230,7 +229,8 @@ class TasksRoutesTest {
.get("/" + taskId.getValue() + "/await")
.then()
.statusCode(HttpStatus.BAD_REQUEST_400)
- .body("message", allOf(containsString("Invalid timeout"),
containsString("Timeout should be positive")));
+ .body("message", containsString("Invalid timeout"))
+ .body("details", containsString("Duration amount should be
positive"));
}
@Test
@@ -243,7 +243,8 @@ class TasksRoutesTest {
.get("/" + taskId.getValue() + "/await")
.then()
.statusCode(HttpStatus.BAD_REQUEST_400)
- .body("message", allOf(containsString("Invalid timeout"),
containsString("Timeout should not exceed one year")));
+ .body("message", containsString("Invalid timeout"))
+ .body("details", containsString("Timeout should not exceed 365
days"));
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
