Arsnael commented on code in PR #1643:
URL: https://github.com/apache/james-project/pull/1643#discussion_r1260543247
##########
server/data/data-jmap-cassandra/src/main/java/org/apache/james/jmap/cassandra/filtering/RuleDTO.java:
##########
@@ -254,13 +279,21 @@ public ActionDTO getAction() {
}
public Rule toRule() {
- return Rule.builder()
+ Rule.Builder ruleBuilder = Rule.builder()
.id(Rule.Id.of(id))
.name(name)
- .condition(conditionDTO.toCondition())
- .name(name)
- .action(actionDTO.toAction())
- .build();
+ .action(actionDTO.toAction());
+
+ if (conditionDTO != null) {
+ ruleBuilder.conditions(Arrays.asList(conditionDTO.toCondition()));
+ } else {
+
ruleBuilder.conditions(conditionDTOs.stream().map(ConditionDTO::toCondition).collect(Collectors.toList()));
+ }
+
+ if (conditionCombiner != null)
ruleBuilder.conditionCombiner(conditionCombiner);
+ else ruleBuilder.conditionCombiner(Rule.ConditionCombiner.AND);
Review Comment:
No if else blocks like this, should be:
```
if (condition) {
code
} else {
code
}
```
better readability
##########
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/mailet/filter/MailMatcher.java:
##########
@@ -51,27 +87,28 @@ private HeaderMatcher(ContentMatcher contentMatcher, String
ruleValue,
this.headerExtractor = headerExtractor;
}
- @Override
- public boolean match(Mail mail) {
- try {
- Stream<String> headerLines = headerExtractor.apply(mail);
- return contentMatcher.match(headerLines, ruleValue);
- } catch (Exception e) {
- LOGGER.error("error while extracting mail header", e);
- return false;
- }
+ public ContentMatcher getContentMatcher() {
+ return contentMatcher;
+ }
+
+ public String getRuleValue() {
+ return ruleValue;
+ }
+
+ public HeaderExtractor getHeaderExtractor() {
+ return headerExtractor;
}
}
static MailMatcher from(Rule rule) {
- Condition ruleCondition = rule.getCondition();
- Optional<ContentMatcher> maybeContentMatcher =
ContentMatcher.asContentMatcher(ruleCondition.getField(),
ruleCondition.getComparator());
- Optional<HeaderExtractor> maybeHeaderExtractor =
HeaderExtractor.asHeaderExtractor(ruleCondition.getField());
-
- return new HeaderMatcher(
- maybeContentMatcher.orElseThrow(() -> new RuntimeException("No
content matcher associated with field " + ruleCondition.getField())),
- rule.getCondition().getValue(),
- maybeHeaderExtractor.orElseThrow(() -> new RuntimeException("No
content matcher associated with comparator " + ruleCondition.getComparator())));
+ return new HeaderMatcher(rule.getConditions().stream()
+ .map(ruleCondition -> new MailMatchingCondition(
+ ContentMatcher.asContentMatcher(ruleCondition.getField(),
ruleCondition.getComparator()).
Review Comment:
the `.` at the end should be on the below line with `orEleseThrow`
##########
server/data/data-jmap-cassandra/src/main/java/org/apache/james/jmap/cassandra/filtering/RuleDTO.java:
##########
@@ -254,13 +279,21 @@ public ActionDTO getAction() {
}
public Rule toRule() {
- return Rule.builder()
+ Rule.Builder ruleBuilder = Rule.builder()
.id(Rule.Id.of(id))
.name(name)
- .condition(conditionDTO.toCondition())
- .name(name)
- .action(actionDTO.toAction())
- .build();
+ .action(actionDTO.toAction());
+
+ if (conditionDTO != null) {
+ ruleBuilder.conditions(Arrays.asList(conditionDTO.toCondition()));
+ } else {
+
ruleBuilder.conditions(conditionDTOs.stream().map(ConditionDTO::toCondition).collect(Collectors.toList()));
+ }
+
+ if (conditionCombiner != null)
ruleBuilder.conditionCombiner(conditionCombiner);
+ else ruleBuilder.conditionCombiner(Rule.ConditionCombiner.AND);
Review Comment:
I think we could have kept the initial layout? Could extract the if else
condition to functions
##########
server/data/data-jmap-cassandra/src/main/java/org/apache/james/jmap/cassandra/filtering/RuleDTO.java:
##########
@@ -215,22 +217,45 @@ public static ImmutableList<RuleDTO> from(List<Rule>
rules) {
public static RuleDTO from(Rule rule) {
return new RuleDTO(rule.getId().asString(),
rule.getName(),
- ConditionDTO.from(rule.getCondition()),
+
rule.getConditions().stream().map(ConditionDTO::from).collect(Collectors.toList()),
+ rule.getConditionCombiner(),
ActionDTO.from(rule.getAction()));
}
private final String id;
private final String name;
private final ConditionDTO conditionDTO;
+ private final List<ConditionDTO> conditionDTOs;
+ private final Rule.ConditionCombiner conditionCombiner;
private final ActionDTO actionDTO;
+ @JsonCreator
+ public RuleDTO(@JsonProperty("id") String id,
+ @JsonProperty("name") String name,
+ @JsonProperty("conditions") List<ConditionDTO>
conditionDTOs,
+ @JsonProperty("conditionCombiner") Rule.ConditionCombiner
conditionCombiner,
+ @JsonProperty("action") ActionDTO actionDTO) {
+ this.name = name;
+ this.conditionDTO = null;
+ this.conditionDTOs = conditionDTOs;
+ this.conditionCombiner = conditionCombiner;
+ this.actionDTO = actionDTO;
+ Preconditions.checkNotNull(id);
Review Comment:
Please refactor the initial constructor as well that has preconditions at
the end by mistake
--
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]
