chibenwa commented on a change in pull request #346:
URL: https://github.com/apache/james-project/pull/346#discussion_r602182957
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/impl/DefineRulesCommandHandler.java
##########
@@ -46,7 +46,7 @@ public DefineRulesCommandHandler(EventStore eventStore) {
FilteringAggregateId aggregateId = new
FilteringAggregateId(storeCommand.getUsername());
return Mono.from(eventStore.getEventsOfAggregate(aggregateId))
- .map(history -> FilteringAggregate.load(aggregateId,
history).defineRules(storeCommand.getRules()));
+ .map(history -> FilteringAggregate.load(aggregateId,
history).defineRules(storeCommand.getRules(), storeCommand.getIfInState()));
Review comment:
Can we refactor to:
```suggestion
.map(history -> FilteringAggregate.load(aggregateId,
history).defineRules(storeCommand));
```
As the goal of an aggregate is to handle command?
##########
File path:
server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/FilteringManagementContract.java
##########
@@ -66,7 +66,7 @@ default void listingRulesShouldThrowWhenNullUser(EventStore
eventStore) {
default void listingRulesShouldReturnDefinedRules(EventStore eventStore) {
FilteringManagement testee =
instantiateFilteringManagement(eventStore);
- Mono.from(testee.defineRulesForUser(USERNAME, RULE_1, RULE_2)).block();
+ Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(Version.INITIAL), RULE_1, RULE_2)).block();
Review comment:
Just pass `Optional.empty()` for all existing tests!
##########
File path:
server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/impl/DefineRulesCommandTest.java
##########
@@ -40,13 +43,13 @@ void shouldMatchBeanContract() {
@Test
void constructorShouldThrowWhenNullUser() {
- assertThatThrownBy(() -> new DefineRulesCommand(null,
ImmutableList.of(RULE_1, RULE_2)))
+ assertThatThrownBy(() -> new DefineRulesCommand(null,
ImmutableList.of(RULE_1, RULE_2), Optional.of(new Version(0))))
.isInstanceOf(NullPointerException.class);
}
@Test
void constructorShouldThrowWhenNullRuleList() {
- assertThatThrownBy(() -> new
DefineRulesCommand(Username.of("[email protected]"), null))
+ assertThatThrownBy(() -> new
DefineRulesCommand(Username.of("[email protected]"), null, Optional.of(new
Version(0))))
Review comment:
empty
##########
File path:
server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/impl/DefineRulesCommandTest.java
##########
@@ -40,13 +43,13 @@ void shouldMatchBeanContract() {
@Test
void constructorShouldThrowWhenNullUser() {
- assertThatThrownBy(() -> new DefineRulesCommand(null,
ImmutableList.of(RULE_1, RULE_2)))
+ assertThatThrownBy(() -> new DefineRulesCommand(null,
ImmutableList.of(RULE_1, RULE_2), Optional.of(new Version(0))))
Review comment:
empty
##########
File path:
server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/FilteringManagementContract.java
##########
@@ -133,11 +133,36 @@ default void
definingEmptyRuleListShouldRemoveExistingRules(EventStore eventStor
default void allFieldsAndComparatorShouldWellBeStored(EventStore
eventStore) {
FilteringManagement testee =
instantiateFilteringManagement(eventStore);
- Mono.from(testee.defineRulesForUser(USERNAME, RULE_FROM,
RULE_RECIPIENT, RULE_SUBJECT, RULE_TO, RULE_1)).block();
-
+ Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(Version.INITIAL), RULE_FROM, RULE_RECIPIENT, RULE_SUBJECT, RULE_TO,
RULE_1)).block();
assertThat(Mono.from(testee.listRulesForUser(USERNAME)).block())
.isEqualTo(new Rules(ImmutableList.of(RULE_FROM, RULE_RECIPIENT,
RULE_SUBJECT, RULE_TO, RULE_1), new Version(0)));
}
+ @Test
+ default void
definingRulesFirstTimeWithWrongVersionInitialShouldFail(EventStore eventStore) {
+ FilteringManagement testee =
instantiateFilteringManagement(eventStore);
+
+ assertThatThrownBy(() -> Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(new Version(-2)), RULE_3, RULE_2, RULE_1)).block())
Review comment:
IMO `new Version(-2)` should throw an Illegal arguent exception in itsel
(cn you add a precondition there?)
Also, use `new Version(1)` here...
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/impl/EventSourcingFilteringManagement.java
##########
@@ -56,8 +58,12 @@ public EventSourcingFilteringManagement(EventStore
eventStore) {
}
@Override
- public Publisher<Void> defineRulesForUser(Username username, List<Rule>
rules) {
- return Mono.from(eventSourcingSystem.dispatch(new
DefineRulesCommand(username, rules))).then();
+ public Publisher<Version> defineRulesForUser(Username username, List<Rule>
rules, Optional<Version> ifInState) {
+ return Mono.from(eventSourcingSystem.dispatch(new
DefineRulesCommand(username, rules, ifInState)))
+ .then(Mono.from(eventStore.getEventsOfAggregate(new
FilteringAggregateId(username)))
+ .map(History::getVersionAsJava)
+ .map(optionalVersion -> optionalVersion.map(eventId -> new
Version(eventId.value()))
+ .orElse(null)));
Review comment:
1. Is that really a `null` value?
In which case is it better than an Optional?
Are we begging for bugs?
Also, shouldn't we have the `EventId -> Version` conversion centralized in a
single place? (this would avoid divergences.)
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/impl/FilteringAggregate.java
##########
@@ -81,6 +83,13 @@ private boolean shouldNotContainDuplicates(List<Rule> rules)
{
return uniqueIdCount == rules.size();
}
+ private boolean
shouldHaveTheSameVersionOrExactlyVersionInitialWhenFirstTimeSetRules(Optional<Version>
ifInState) {
+ if (ifInState.isPresent() && ifInState.get().equals(Version.INITIAL)
&& history.getVersionAsJava().isEmpty()) return true;
+ if (history.getVersionAsJava().isPresent() && ifInState.isPresent()
+ && history.getVersionAsJava().get().value() ==
ifInState.get().getVersion()) return true;
+ return false;
Review comment:
- it do not pass the checkstylle
- don't expect me to understand piece of code. even less to maintain it.
Please write comprehensive code for mere humans, use map filter, etc...
Let's write functional code...
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetFilterMethod.java
##########
@@ -155,7 +157,7 @@ public SetFilterMethod(MetricFactory metricFactory,
FilteringManagement filterin
ensureNoDuplicatedRules(rules);
ensureNoMultipleMailboxesRules(rules);
- return Mono.from(filteringManagement.defineRulesForUser(username,
rules))
+ return Mono.from(filteringManagement.defineRulesForUser(username,
rules, Optional.of(Version.INITIAL)))
Review comment:
Optional.empty
##########
File path:
server/data/data-jmap/src/test/java/org/apache/james/jmap/api/filtering/FilteringManagementContract.java
##########
@@ -133,11 +133,36 @@ default void
definingEmptyRuleListShouldRemoveExistingRules(EventStore eventStor
default void allFieldsAndComparatorShouldWellBeStored(EventStore
eventStore) {
FilteringManagement testee =
instantiateFilteringManagement(eventStore);
- Mono.from(testee.defineRulesForUser(USERNAME, RULE_FROM,
RULE_RECIPIENT, RULE_SUBJECT, RULE_TO, RULE_1)).block();
-
+ Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(Version.INITIAL), RULE_FROM, RULE_RECIPIENT, RULE_SUBJECT, RULE_TO,
RULE_1)).block();
assertThat(Mono.from(testee.listRulesForUser(USERNAME)).block())
.isEqualTo(new Rules(ImmutableList.of(RULE_FROM, RULE_RECIPIENT,
RULE_SUBJECT, RULE_TO, RULE_1), new Version(0)));
}
+ @Test
+ default void
definingRulesFirstTimeWithWrongVersionInitialShouldFail(EventStore eventStore) {
+ FilteringManagement testee =
instantiateFilteringManagement(eventStore);
+
+ assertThatThrownBy(() -> Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(new Version(-2)), RULE_3, RULE_2, RULE_1)).block())
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ default void
modifyExistingRulesWithWrongCurrentVersionShouldFail(EventStore eventStore) {
+ FilteringManagement testee =
instantiateFilteringManagement(eventStore);
+
+ Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(Version.INITIAL), RULE_3, RULE_2, RULE_1)).block();
+
+ assertThatThrownBy(() -> Mono.from(testee.defineRulesForUser(USERNAME,
Optional.of(new Version(1)), RULE_2, RULE_1)).block())
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ default void setRulesWithEmptyVersionShouldFail(EventStore eventStore) {
+ FilteringManagement testee =
instantiateFilteringManagement(eventStore);
+
+ assertThatThrownBy(() -> Mono.from(testee.defineRulesForUser(USERNAME,
Optional.empty(), RULE_2, RULE_1)).block())
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
Review comment:
Can you add a test when modifying with the right version?
##########
File path:
server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java
##########
@@ -116,7 +118,7 @@ public void defineRulesForRecipient1(List<Rule.Condition>
conditions) {
.build())
.collect(ImmutableList.toImmutableList());
-
Mono.from(testSystem.getFilteringManagement().defineRulesForUser(RECIPIENT_1_USERNAME,
rules)).block();
+
Mono.from(testSystem.getFilteringManagement().defineRulesForUser(RECIPIENT_1_USERNAME,
rules, Optional.of(Version.INITIAL))).block();
Review comment:
empty
##########
File path:
server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java
##########
@@ -694,6 +695,7 @@ void
mailDirectiveShouldSetLastMatchedRuleWhenMultipleRules(JMAPFilteringTestSys
MailboxId mailbox3Id =
testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_3");
Mono.from(testSystem.getFilteringManagement().defineRulesForUser(RECIPIENT_1_USERNAME,
+ Optional.of(Version.INITIAL),
Review comment:
empty everywhere...
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
