You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by rc...@apache.org on 2020/09/04 03:02:54 UTC
[james-project] 04/04: JAMES-3363: Add integration test for
initialize Rule and validating parameters
This is an automated email from the ASF dual-hosted git repository.
rcordier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 9f214eabd995475a68a6b04aad2c3728cea8a879
Author: duc91 <vd...@linagora.com>
AuthorDate: Wed Aug 26 17:32:33 2020 +0700
JAMES-3363: Add integration test for initialize Rule and validating parameters
---
.../org/apache/james/jmap/api/filtering/Rule.java | 18 ++-
.../james/jmap/api/filtering/RuleFixture.java | 8 +-
.../apache/james/jmap/api/filtering/RuleTest.java | 20 ++--
.../jmap/draft/methods/integration/FilterTest.java | 132 ++++++++++++++++++++-
.../james/jmap/draft/methods/SetFilterMethod.java | 31 ++++-
.../apache/james/jmap/draft/model/JmapRuleDTO.java | 49 ++++----
6 files changed, 206 insertions(+), 52 deletions(-)
diff --git a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/Rule.java b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/Rule.java
index 5a34a09..3d9af27 100644
--- a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/Rule.java
+++ b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/Rule.java
@@ -35,7 +35,6 @@ public class Rule {
public static class Id {
public static Id of(String id) {
- Preconditions.checkArgument(StringUtils.isNotEmpty(id), "`id` is mandatory");
Preconditions.checkArgument(StringUtils.isNotBlank(id), "`id` is mandatory");
return new Id(id);
}
@@ -88,7 +87,7 @@ public class Rule {
}
public static Field of(String fieldName) {
- return find(fieldName).orElseThrow(() -> new IllegalStateException("'" + fieldName + "' is not a valid field name"));
+ return find(fieldName).orElseThrow(() -> new IllegalArgumentException("'" + fieldName + "' is not a valid field name"));
}
private final String fieldName;
@@ -115,7 +114,7 @@ public class Rule {
}
public static Comparator of(String comparatorName) {
- return find(comparatorName).orElseThrow(() -> new IllegalStateException("'" + comparatorName + "' is not a valid comparator name"));
+ return find(comparatorName).orElseThrow(() -> new IllegalArgumentException("'" + comparatorName + "' is not a valid comparator name"));
}
private final String comparatorName;
@@ -294,6 +293,11 @@ public class Rule {
}
public Rule build() {
+ Preconditions.checkState(id != null, "`id` is mandatory");
+ Preconditions.checkState(StringUtils.isNotBlank(name), "`name` is mandatory");
+ Preconditions.checkState(condition != null, "`condition` is mandatory");
+ Preconditions.checkState(action != null, "`action` is mandatory");
+
return new Rule(id, name, condition, action);
}
@@ -308,13 +312,7 @@ public class Rule {
private final Condition condition;
private final Action action;
- public Rule(Id id, String name, Condition condition, Action action) {
- Preconditions.checkArgument(id != null, "`id` is mandatory");
- Preconditions.checkArgument(StringUtils.isNotBlank(name), "`name` is mandatory");
- Preconditions.checkArgument(StringUtils.isNotEmpty(name), "`name` is mandatory");
- Preconditions.checkArgument(condition != null, "`condition` is mandatory");
- Preconditions.checkArgument(action != null, "`action` is mandatory");
-
+ private Rule(Id id, String name, Condition condition, Action action) {
this.id = id;
this.name = name;
this.condition = condition;
diff --git a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleFixture.java b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleFixture.java
index 96d31ea..b318f63 100644
--- a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleFixture.java
+++ b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleFixture.java
@@ -23,10 +23,10 @@ public interface RuleFixture {
String NAME = "a name";
Rule.Condition CONDITION = Rule.Condition.of(Rule.Condition.Field.CC, Rule.Condition.Comparator.CONTAINS, "something");
Rule.Action ACTION = Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds("id-01"));
- Rule.Builder RULE_BUILER = Rule.builder().name(NAME).condition(CONDITION).action(ACTION);
- Rule RULE_1 = RULE_BUILER.id(Rule.Id.of("1")).build();
- Rule RULE_2 = RULE_BUILER.id(Rule.Id.of("2")).build();
- Rule RULE_3 = RULE_BUILER.id(Rule.Id.of("3")).build();
+ Rule.Builder RULE_BUILDER = Rule.builder().name(NAME).condition(CONDITION).action(ACTION);
+ Rule RULE_1 = RULE_BUILDER.id(Rule.Id.of("1")).build();
+ Rule RULE_2 = RULE_BUILDER.id(Rule.Id.of("2")).build();
+ Rule RULE_3 = RULE_BUILDER.id(Rule.Id.of("3")).build();
Rule RULE_TO = Rule.builder()
.id(Rule.Id.of("id-to"))
diff --git a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleTest.java b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleTest.java
index 41a383d..830f975 100644
--- a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleTest.java
+++ b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/RuleTest.java
@@ -92,7 +92,7 @@ class RuleTest {
.condition(CONDITION)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
@@ -103,11 +103,11 @@ class RuleTest {
.condition(CONDITION)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
- void shouldWhenNameEmpty() {
+ void shouldFailWhenNameEmpty() {
assertThatThrownBy(() ->
Rule.builder()
.id(UNIQUE_ID)
@@ -115,11 +115,11 @@ class RuleTest {
.condition(CONDITION)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
- void shouldWhenNameBlank() {
+ void shouldFailWhenNameBlank() {
assertThatThrownBy(() ->
Rule.builder()
.id(UNIQUE_ID)
@@ -127,11 +127,11 @@ class RuleTest {
.condition(CONDITION)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
- void shouldWhenNameNull() {
+ void shouldFailWhenNameNull() {
assertThatThrownBy(() ->
Rule.builder()
.id(UNIQUE_ID)
@@ -139,7 +139,7 @@ class RuleTest {
.condition(CONDITION)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
@@ -150,7 +150,7 @@ class RuleTest {
.name(NAME)
.action(ACTION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
@@ -161,7 +161,7 @@ class RuleTest {
.name(NAME)
.condition(CONDITION)
.build())
- .isInstanceOf(IllegalArgumentException.class);
+ .isInstanceOf(IllegalStateException.class);
}
@Test
diff --git a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/FilterTest.java b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/FilterTest.java
index 1eef429..283c800 100644
--- a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/FilterTest.java
+++ b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/FilterTest.java
@@ -241,6 +241,136 @@ public abstract class FilterTest {
}
@Test
+ public void setFilterShouldRejectRuleWithInvalidRuleName() {
+ MailboxId mailbox = randomMailboxId();
+
+ given()
+ .header("Authorization", accessToken.asString())
+ .body("[[" +
+ " \"setFilter\", " +
+ " {" +
+ " \"singleton\": [" +
+ " {" +
+ " \"id\": \"3000-34e\"," +
+ " \"name\": null," +
+ " \"condition\": {" +
+ " \"field\": \"subject\"," +
+ " \"comparator\": \"contains\"," +
+ " \"value\": \"question\"" +
+ " }," +
+ " \"action\": {" +
+ " \"appendIn\": {" +
+ " \"mailboxIds\": [\"" + mailbox.serialize() + "\"]" +
+ " }" +
+ " }" +
+ " }" +
+ " ]}, " +
+ "\"#0\"" +
+ "]]")
+ .when()
+ .post("/jmap")
+ .then()
+ .body(ARGUMENTS + ".type", equalTo("invalidArguments"))
+ .body(ARGUMENTS + ".description", equalTo("`name` is mandatory"));
+ }
+
+ @Test
+ public void setFilterShouldRejectRuleWithInvalidRuleId() {
+ MailboxId mailbox = randomMailboxId();
+
+ given()
+ .header("Authorization", accessToken.asString())
+ .body("[[" +
+ " \"setFilter\", " +
+ " {" +
+ " \"singleton\": [" +
+ " {" +
+ " \"id\": null," +
+ " \"name\": \"some name\"," +
+ " \"condition\": {" +
+ " \"field\": \"subject\"," +
+ " \"comparator\": \"contains\"," +
+ " \"value\": \"question\"" +
+ " }," +
+ " \"action\": {" +
+ " \"appendIn\": {" +
+ " \"mailboxIds\": [\"" + mailbox.serialize() + "\"]" +
+ " }" +
+ " }" +
+ " }" +
+ " ]}, " +
+ "\"#0\"" +
+ "]]")
+ .when()
+ .post("/jmap")
+ .then()
+ .body(ARGUMENTS + ".type", equalTo("invalidArguments"))
+ .body(ARGUMENTS + ".description", equalTo("`id` is mandatory"));
+ }
+
+ @Test
+ public void setFilterShouldRejectRuleWithoutRuleCondition() {
+ MailboxId mailbox = randomMailboxId();
+
+ given()
+ .header("Authorization", accessToken.asString())
+ .body("[[" +
+ " \"setFilter\", " +
+ " {" +
+ " \"singleton\": [" +
+ " {" +
+ " \"id\": \"3000-34e\"," +
+ " \"name\": \"some name\"," +
+ " \"condition\": null," +
+ " \"action\": {" +
+ " \"appendIn\": {" +
+ " \"mailboxIds\": [\"" + mailbox.serialize() + "\"]" +
+ " }" +
+ " }" +
+ " }" +
+ " ]}, " +
+ "\"#0\"" +
+ "]]")
+ .when()
+ .post("/jmap")
+ .then()
+ .body(ARGUMENTS + ".type", equalTo("invalidArguments"))
+ .body(ARGUMENTS + ".description", equalTo("`condition` is mandatory"));
+ }
+
+ @Test
+ public void setFilterShouldRejectRuleWithInvalidRuleCondition() {
+ MailboxId mailbox = randomMailboxId();
+
+ given()
+ .header("Authorization", accessToken.asString())
+ .body("[[" +
+ " \"setFilter\", " +
+ " {" +
+ " \"singleton\": [" +
+ " {" +
+ " \"id\": \"3000-34e\"," +
+ " \"name\": \"some name\"," +
+ " \"condition\": {" +
+ " \"field\": \"subject\" " +
+ " }," +
+ " \"action\": {" +
+ " \"appendIn\": {" +
+ " \"mailboxIds\": [\"" + mailbox.serialize() + "\"]" +
+ " }" +
+ " }" +
+ " }" +
+ " ]}, " +
+ "\"#0\"" +
+ "]]")
+ .when()
+ .post("/jmap")
+ .then()
+ .body(ARGUMENTS + ".type", equalTo("invalidArguments"))
+ .body(ARGUMENTS + ".description", equalTo("'null' is not a valid comparator name"));
+ }
+
+ @Test
public void setFilterShouldReturnUpdatedSingleton() {
MailboxId mailbox = randomMailboxId();
@@ -1304,7 +1434,6 @@ public abstract class FilterTest {
() -> JmapCommonRequests.isAnyMessageFoundInRecipientsMailbox(accessToken, inbox));
}
-
@Test
public void messageShouldBeAppendedInInboxWhenSubjectRuleDoesNotMatchRuleBecauseOfCase() {
given()
@@ -1635,7 +1764,6 @@ public abstract class FilterTest {
() -> JmapCommonRequests.isAnyMessageFoundInRecipientsMailbox(accessToken, matchedMailbox));
}
-
@Test
public void messageShouldBeAppendedInSpecificMailboxWhenExactlyEqualsMatchesCaseInsensitivelyFullHeader() {
given()
diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetFilterMethod.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetFilterMethod.java
index 154aefa..2c45d51 100644
--- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetFilterMethod.java
+++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetFilterMethod.java
@@ -128,16 +128,22 @@ public class SetFilterMethod implements Method {
try {
return updateFilter(methodCallId, request, mailboxSession.getUser())
.doOnEach(ReactorUtils.logOnError(e -> LOGGER.warn("Failed setting Rules", e)))
- .onErrorResume(e -> Mono.just(unKnownError(methodCallId)));
+ .onErrorResume(e -> Mono.just(unknownError(methodCallId)));
} catch (MultipleMailboxIdException e) {
LOGGER.debug("Rule targeting several mailboxes", e);
return Mono.just(multipleMailboxesError(methodCallId, e));
} catch (DuplicatedRuleException e) {
LOGGER.debug("Duplicated rules", e);
return Mono.just(duplicatedIdsError(methodCallId, e));
- } catch (Exception e) {
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("IllegalArgumentException of setting Rules", e);
+ return Mono.just(invalidArgumentsError(methodCallId, e.getMessage()));
+ } catch (IllegalStateException e) {
+ LOGGER.warn("IllegalStateException of setting Rules", e);
+ return Mono.just(invalidArgumentsError(methodCallId, e.getMessage()));
+ } catch (Exception e) {
LOGGER.warn("Failed setting Rules", e);
- return Mono.just(unKnownError(methodCallId));
+ return Mono.just(unknownError(methodCallId));
}
}
@@ -185,13 +191,28 @@ public class SetFilterMethod implements Method {
}
}
- private JmapResponse unKnownError(MethodCallId methodCallId) {
+ private JmapResponse unknownError(MethodCallId methodCallId) {
+ return unknownError(methodCallId, "Failed to retrieve filter");
+ }
+
+ private JmapResponse invalidArgumentsError(MethodCallId methodCallId, String errorMessage) {
+ return JmapResponse.builder()
+ .methodCallId(methodCallId)
+ .responseName(RESPONSE_NAME)
+ .response(ErrorResponse.builder()
+ .type(SetError.Type.INVALID_ARGUMENTS.asString())
+ .description(errorMessage)
+ .build())
+ .build();
+ }
+
+ private JmapResponse unknownError(MethodCallId methodCallId, String errorMessage) {
return JmapResponse.builder()
.methodCallId(methodCallId)
.responseName(RESPONSE_NAME)
.response(ErrorResponse.builder()
.type(SetError.Type.ERROR.asString())
- .description("Failed to retrieve filter")
+ .description(errorMessage)
.build())
.build();
}
diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/JmapRuleDTO.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/JmapRuleDTO.java
index 0d71567..3ceda24 100644
--- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/JmapRuleDTO.java
+++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/JmapRuleDTO.java
@@ -20,6 +20,7 @@
package org.apache.james.jmap.draft.model;
import java.util.List;
+import java.util.Optional;
import org.apache.james.jmap.api.filtering.Rule;
@@ -132,51 +133,57 @@ public class JmapRuleDTO {
}
public static JmapRuleDTO from(Rule rule) {
- return new JmapRuleDTO(rule.getId().asString(),
- rule.getName(),
- ConditionDTO.from(rule.getCondition()),
- ActionDTO.from(rule.getAction()));
+ return new JmapRuleDTO(Optional.of(rule.getId().asString()),
+ Optional.of(rule.getName()),
+ Optional.of(ConditionDTO.from(rule.getCondition())),
+ Optional.of(ActionDTO.from(rule.getAction())));
}
- private final String id;
- private final String name;
- private final ConditionDTO conditionDTO;
- private final ActionDTO actionDTO;
+ private final Optional<String> id;
+ private final Optional<String> name;
+ private final Optional<ConditionDTO> conditionDTO;
+ private final Optional<ActionDTO> actionDTO;
@JsonCreator
- public JmapRuleDTO(@JsonProperty("id") String id,
- @JsonProperty("name") String name,
- @JsonProperty("condition") ConditionDTO conditionDTO,
- @JsonProperty("action") ActionDTO actionDTO) {
+ public JmapRuleDTO(@JsonProperty("id") Optional<String> id,
+ @JsonProperty("name") Optional<String> name,
+ @JsonProperty("condition") Optional<ConditionDTO> conditionDTO,
+ @JsonProperty("action") Optional<ActionDTO> actionDTO) {
this.name = name;
this.conditionDTO = conditionDTO;
this.actionDTO = actionDTO;
this.id = id;
}
- public String getId() {
+ public Optional<String> getId() {
return id;
}
- public String getName() {
+ public Optional<String> getName() {
return name;
}
- public ConditionDTO getCondition() {
+ public Optional<ConditionDTO> getCondition() {
return conditionDTO;
}
- public ActionDTO getAction() {
+ public Optional<ActionDTO> getAction() {
return actionDTO;
}
public Rule toRule() {
+ Preconditions.checkState(conditionDTO.isPresent(), "`condition` is mandatory");
+ Preconditions.checkState(actionDTO.isPresent(), "`action` is mandatory");
+ Preconditions.checkState(name.isPresent(), "`name` is mandatory");
+ Preconditions.checkState(!name.get().isEmpty(), "`name` is mandatory");
+ Preconditions.checkState(id.isPresent(), "`id` is mandatory");
+ Preconditions.checkState(!id.get().isEmpty(), "`id` is mandatory");
+
return Rule.builder()
- .id(Rule.Id.of(id))
- .name(name)
- .condition(conditionDTO.toCondition())
- .name(name)
- .action(actionDTO.toAction())
+ .id(Rule.Id.of(id.get()))
+ .name(name.get())
+ .condition(conditionDTO.get().toCondition())
+ .action(actionDTO.get().toAction())
.build();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org