chibenwa commented on a change in pull request #534:
URL: https://github.com/apache/james-project/pull/534#discussion_r668582750
##########
File path:
mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java
##########
@@ -324,6 +325,16 @@ public final int hashCode() {
*/
Publisher<MessageId> search(MultimailboxesSearchQuery expression,
MailboxSession session, long limit) throws MailboxException;
+ /**
+ *
Review comment:
Returns the list of MessageId of messages belonging to that Thread
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/ThreadGetMethod.scala
##########
@@ -1,52 +1,80 @@
-/****************************************************************
+/** **************************************************************
* Licensed to the Apache Software Foundation (ASF) under one *
* or more contributor license agreements. See the NOTICE file *
* distributed with this work for additional information *
* regarding copyright ownership. The ASF licenses this file *
* to you under the Apache License, Version 2.0 (the *
* "License"); you may not use this file except in compliance *
* with the License. You may obtain a copy of the License at *
- * *
- * http://www.apache.org/licenses/LICENSE-2.0 *
- * *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
Review comment:
Oups you changed that license format
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/ThreadGetContract.scala
##########
@@ -27,10 +29,27 @@ import org.apache.http.HttpStatus.SC_OK
import org.apache.james.GuiceJamesServer
import org.apache.james.jmap.core.ResponseObject.SESSION_STATE
import org.apache.james.jmap.http.UserCredential
-import
org.apache.james.jmap.rfc8621.contract.Fixture.{ACCEPT_RFC8621_VERSION_HEADER,
BOB, BOB_PASSWORD, DOMAIN, authScheme, baseRequestSpecBuilder}
+import
org.apache.james.jmap.rfc8621.contract.Fixture.{ACCEPT_RFC8621_VERSION_HEADER,
ANDRE, BOB, BOB_PASSWORD, DOMAIN, authScheme, baseRequestSpecBuilder}
+import org.apache.james.mailbox.MessageManager
+import org.apache.james.mailbox.model.MailboxPath
+import org.apache.james.mime4j.dom.Message
+import org.apache.james.mime4j.stream.RawField
+import org.apache.james.modules.MailboxProbeImpl
import org.apache.james.utils.DataProbeImpl
import org.junit.jupiter.api.{BeforeEach, Test}
+object ThreadGetContract {
+ private def createTestMessage: Message = Message.Builder
Review comment:
Unused :D
##########
File path:
mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java
##########
@@ -324,6 +325,16 @@ public final int hashCode() {
*/
Publisher<MessageId> search(MultimailboxesSearchQuery expression,
MailboxSession session, long limit) throws MailboxException;
+ /**
+ *
+ * @param threadId
+ * target Thread
+ * @param session
+ * the context for this call, not null
+ * @return a list of MessageId of messages belong to that Thread
Review comment:
* @return the list of MessageId of messages belonging to that Thread
##########
File path:
mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java
##########
@@ -333,7 +333,7 @@ public final int hashCode() {
* the context for this call, not null
* @return a list of MessageId of messages belong to that Thread
*/
- Flux<MessageId> getThread(ThreadId threadId, MailboxSession session)
throws MailboxException;
+ Publisher<MessageId> getThread(ThreadId threadId, MailboxSession session)
throws MailboxException;
Review comment:
Methods with publishers should not throw.
They should return publisher that eventually completes with an error
(`Flux.error`)
The error is returned when blocking the publisher not while generating the
publisher.
##########
File path:
mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/SearchThreadIdGuessingAlgorithm.java
##########
@@ -71,12 +72,12 @@ public SearchThreadIdGuessingAlgorithm(MailboxManager
mailboxManager, MessageIdM
public Flux<MessageId> getMessageIdsInThread(ThreadId threadId,
MailboxSession session) throws MailboxException {
MultimailboxesSearchQuery expression =
MultimailboxesSearchQuery.from(SearchQuery.matchAll()).build();
- return Flux.from(mailboxManager.search(expression, session,
Long.MAX_VALUE))
+ return Flux.from(mailboxManager.search(expression, session,
Integer.MAX_VALUE))
.collectList()
.flatMapMany(messageIds ->
messageIdManager.getMessagesReactive(messageIds, FetchGroup.MINIMAL, session))
.filter(messageResult ->
messageResult.getThreadId().equals(threadId))
- .map(messageResult -> messageResult.getMessageId());
-
+ .map(messageResult -> messageResult.getMessageId())
+ .switchIfEmpty(Mono.error(new ThreadNotFoundException(threadId)));
Review comment:
.switchIfEmpty(Mono.error(() -> new ThreadNotFoundException(threadId)));
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/ThreadGetContract.scala
##########
@@ -160,16 +210,24 @@ trait ThreadGetContract {
assertThatJson(response)
.isEqualTo(
s"""{
- | "sessionState": "${SESSION_STATE.value}",
- | "methodResponses": [
- | [
- | "error",
- | {
- | "type": "accountNotFound"
- | },
- | "c1"
- | ]
- | ]
- |}""".stripMargin)
+ | "sessionState": "2c9f1b12-b35a-43e6-9af2-0106fb53a943",
+ | "methodResponses": [
+ | [
+ | "Thread/get",
+ | {
+ | "accountId":
"29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "state":
"2c9f1b12-b35a-43e6-9af2-0106fb53a943",
+ | "list": [{
+ | "id": "$threadId",
+ | "emailIds": ["$message1Id",
"$message2Id"]
+ | }],
+ | "notFound": [
+ |
+ | ]
+ | },
+ | "c1"
+ | ]
+ | ]
+ |}""".stripMargin)
}
}
Review comment:
- In-Reply-To references messsage1 but different subject -> NOMATCH
- REFERENCES -> match
-Maybe 1-2 other fonctional tests.
##########
File path:
mailbox/store/src/test/java/org/apache/james/mailbox/store/ThreadIdGuessingAlgorithmContract.java
##########
@@ -202,4 +231,41 @@ void
givenOldMailWhenAddNonRelatedMailsThenGuessingThreadIdShouldBasedOnGenerate
assertThat(threadId.getBaseMessageId()).isEqualTo(newBasedMessageId);
}
+ @Test
+ void
givenThreeMailsInAThreadThenGetThreadShouldReturnAListWithMessageIdsInThatThread()
throws MailboxException {
+ MailboxMessage message1 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message2 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message3 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+
+ Mailbox mailbox = new Mailbox(MailboxPath.inbox(USER), UID_VALIDITY,
mailboxId);
+ messageMapper.add(mailbox, message1);
+ messageMapper.add(mailbox, message2);
+ messageMapper.add(mailbox, message3);
+
+ Flux<MessageId> messageIds =
testee.getMessageIdsInThread(ThreadId.fromBaseMessageId(newBasedMessageId),
mailboxSession);
+
+
assertThat(messageIds.collectList().block()).isEqualTo(List.of(message1.getMessageId(),
message2.getMessageId(), message3.getMessageId()));
+ }
+
+ @Test
+ void
givenNonMailInAThreadThenGetThreadShouldThrowThreadNotFoundException() throws
MailboxException {
+ Flux<MessageId> messageIds =
testee.getMessageIdsInThread(ThreadId.fromBaseMessageId(newBasedMessageId),
mailboxSession);
+
+ assertThatThrownBy(() ->
testee.getMessageIdsInThread(ThreadId.fromBaseMessageId(newBasedMessageId),
mailboxSession).collectList().block()).getCause().isInstanceOf(ThreadNotFoundException.class);
+ }
+
+ private SimpleMailboxMessage createMessage(MailboxId mailboxId, ThreadId
threadId) {
+ MessageId messageId = mapperProvider.generateMessageId();
+ String content = "Some content";
+ int bodyStart = 16;
+ return new SimpleMailboxMessage(messageId,
+ threadId,
+ new Date(),
+ content.length(),
+ bodyStart,
+ new ByteContent(content.getBytes()),
+ new Flags(),
+ new PropertyBuilder().build(),
+ mailboxId);
+ }
}
Review comment:
One mail in a tread?
mail 1, mail 3 in thread 1
mail 2 in thread 2
get thread 1 -> mail 1 + mail 3 (not related emails are not returned)
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/ThreadGetMethod.scala
##########
@@ -1,52 +1,80 @@
-/****************************************************************
+/** **************************************************************
* Licensed to the Apache Software Foundation (ASF) under one *
* or more contributor license agreements. See the NOTICE file *
* distributed with this work for additional information *
* regarding copyright ownership. The ASF licenses this file *
* to you under the Apache License, Version 2.0 (the *
* "License"); you may not use this file except in compliance *
* with the License. You may obtain a copy of the License at *
- * *
- * http://www.apache.org/licenses/LICENSE-2.0 *
- * *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
* Unless required by applicable law or agreed to in writing, *
* software distributed under the License is distributed on an *
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
* KIND, either express or implied. See the License for the *
* specific language governing permissions and limitations *
* under the License. *
- ****************************************************************/
+ * ************************************************************** */
package org.apache.james.jmap.method
import eu.timepit.refined.auto._
import javax.inject.Inject
import org.apache.james.jmap.core.CapabilityIdentifier.{CapabilityIdentifier,
JMAP_CORE, JMAP_MAIL}
-import org.apache.james.jmap.core.Id.Id
import org.apache.james.jmap.core.Invocation.{Arguments, MethodName}
-import org.apache.james.jmap.core.{Invocation, UuidState}
+import org.apache.james.jmap.core.{AccountId, Invocation, UuidState}
import org.apache.james.jmap.json.{ResponseSerializer, ThreadSerializer}
-import org.apache.james.jmap.mail.{Thread, ThreadGetRequest, ThreadGetResponse}
+import org.apache.james.jmap.mail.{Thread, ThreadGetRequest,
ThreadGetResponse, ThreadNotFound, UnparsedThreadId}
import org.apache.james.jmap.routes.SessionSupplier
-import org.apache.james.mailbox.MailboxSession
+import org.apache.james.mailbox.model.{MessageId, ThreadId => JavaThreadId}
+import org.apache.james.mailbox.{MailboxManager, MailboxSession}
import org.apache.james.metrics.api.MetricFactory
import play.api.libs.json.{JsError, JsSuccess}
-import reactor.core.scala.publisher.SMono
+import reactor.core.scala.publisher.{SFlux, SMono}
+
+import scala.util.Try
+
+object ThreadGetResult {
+ def empty: ThreadGetResult = ThreadGetResult(Set.empty,
ThreadNotFound(Set.empty))
+
+ def merge(result1: ThreadGetResult, result2: ThreadGetResult):
ThreadGetResult = result1.merge(result2)
+
+ def found(thread: Thread): ThreadGetResult =
+ ThreadGetResult(Set(thread), ThreadNotFound(Set.empty))
+
+ def notFound(unparsedThreadId: UnparsedThreadId): ThreadGetResult =
+ ThreadGetResult(Set.empty, ThreadNotFound(Set(unparsedThreadId)))
+}
+
+case class ThreadGetResult(threads: Set[Thread], notFound: ThreadNotFound) {
+ def merge(other: ThreadGetResult): ThreadGetResult =
+ ThreadGetResult(this.threads ++ other.threads,
this.notFound.merge(other.notFound))
+
+ def asResponse(accountId: AccountId): ThreadGetResponse =
+ ThreadGetResponse(
+ accountId = accountId,
+ state = UuidState.INSTANCE,
+ list = threads.toList,
+ notFound = notFound)
+}
class ThreadGetMethod @Inject()(val metricFactory: MetricFactory,
- val sessionSupplier:
SessionSupplier) extends MethodRequiringAccountId[ThreadGetRequest] {
+ val sessionSupplier: SessionSupplier,
+ val messageIdFactory: MessageId.Factory,
Review comment:
Please introduce a ThreadId.Factory class here (ThreadId.Factory would
of course call the MessageId.Factory)
This reduces temporal coupling...
##########
File path:
mailbox/api/src/main/java/org/apache/james/mailbox/exception/ThreadNotFoundException.java
##########
@@ -0,0 +1,32 @@
+/******************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ******************************************************************/
+
+package org.apache.james.mailbox.exception;
+
+import org.apache.james.mailbox.model.ThreadId;
+
+public class ThreadNotFoundException extends MailboxException {
+ public ThreadNotFoundException(String message) {
+ super(message);
+ }
Review comment:
IMO that string method is likely not needed...
##########
File path:
mailbox/store/src/test/java/org/apache/james/mailbox/store/ThreadIdGuessingAlgorithmContract.java
##########
@@ -202,4 +231,41 @@ void
givenOldMailWhenAddNonRelatedMailsThenGuessingThreadIdShouldBasedOnGenerate
assertThat(threadId.getBaseMessageId()).isEqualTo(newBasedMessageId);
}
+ @Test
+ void
givenThreeMailsInAThreadThenGetThreadShouldReturnAListWithMessageIdsInThatThread()
throws MailboxException {
+ MailboxMessage message1 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message2 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message3 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+
+ Mailbox mailbox = new Mailbox(MailboxPath.inbox(USER), UID_VALIDITY,
mailboxId);
+ messageMapper.add(mailbox, message1);
+ messageMapper.add(mailbox, message2);
+ messageMapper.add(mailbox, message3);
+
+ Flux<MessageId> messageIds =
testee.getMessageIdsInThread(ThreadId.fromBaseMessageId(newBasedMessageId),
mailboxSession);
+
+
assertThat(messageIds.collectList().block()).isEqualTo(List.of(message1.getMessageId(),
message2.getMessageId(), message3.getMessageId()));
Review comment:
Use guava ImmutableList.of()
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/ThreadGetContract.scala
##########
@@ -113,34 +140,57 @@ trait ThreadGetContract {
.asString
assertThatJson(response)
- .inPath("methodResponses[0][1]")
.isEqualTo(
s"""{
- | "accountId":
"29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
- | "state": "${SESSION_STATE.value}",
- | "list": [
- | {
- | "id": "123456",
- | "emailIds": ["123456"]
- | },
- | {
- | "id": "789",
- | "emailIds": ["789"]
- | }
- | ]
+ | "sessionState": "${SESSION_STATE.value}",
+ | "methodResponses": [
+ | [
+ | "error",
+ | {
+ | "type": "accountNotFound"
+ | },
+ | "c1"
+ | ]
+ | ]
|}""".stripMargin)
}
@Test
- def badAccountIdShouldBeRejected(): Unit = {
+ def
givenMailsBelongToAThreadThenGetThatThreadShouldReturnExactThreadObject(server:
GuiceJamesServer): Unit = {
+ val bobPath = MailboxPath.inbox(BOB)
+ server.getProbe(classOf[MailboxProbeImpl]).createMailbox(bobPath)
+
+ // given 2 mail with same thread
+ val message1: MessageManager.AppendResult =
server.getProbe(classOf[MailboxProbeImpl])
+ .appendMessageAndGetAppendResult(BOB.asString(), bobPath,
+ MessageManager.AppendCommand.from(Message.Builder.of.setSubject("Test")
+ .setMessageId("Message-ID")
+ .setField(new RawField("In-Reply-To", "someInReplyTo"))
Review comment:
First message don't need the `In-Reply-To` field :D
##########
File path:
mailbox/scanning-search/src/test/java/org/apache/james/mailbox/store/search/SearchThreadIdGuessingAlgorithmTest.java
##########
@@ -19,21 +19,29 @@
package org.apache.james.mailbox.store.search;
+import org.apache.james.mailbox.MailboxSession;
import org.apache.james.mailbox.inmemory.InMemoryCombinationManagerTestSystem;
+import org.apache.james.mailbox.inmemory.InMemoryMailboxManager;
import org.apache.james.mailbox.inmemory.InMemoryMessageId;
+import org.apache.james.mailbox.inmemory.mail.InMemoryMapperProvider;
import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
import org.apache.james.mailbox.model.MessageId;
import org.apache.james.mailbox.store.CombinationManagerTestSystem;
import org.apache.james.mailbox.store.ThreadIdGuessingAlgorithmContract;
+import org.apache.james.mailbox.store.mail.MessageMapper;
import org.apache.james.mailbox.store.mail.SearchThreadIdGuessingAlgorithm;
import org.apache.james.mailbox.store.mail.ThreadIdGuessingAlgorithm;
+import org.apache.james.mailbox.store.mail.model.MapperProvider;
public class SearchThreadIdGuessingAlgorithmTest extends
ThreadIdGuessingAlgorithmContract {
+ InMemoryMailboxManager mailboxManager;
Review comment:
private?
##########
File path:
mailbox/store/src/test/java/org/apache/james/mailbox/store/ThreadIdGuessingAlgorithmContract.java
##########
@@ -202,4 +231,41 @@ void
givenOldMailWhenAddNonRelatedMailsThenGuessingThreadIdShouldBasedOnGenerate
assertThat(threadId.getBaseMessageId()).isEqualTo(newBasedMessageId);
}
+ @Test
+ void
givenThreeMailsInAThreadThenGetThreadShouldReturnAListWithMessageIdsInThatThread()
throws MailboxException {
+ MailboxMessage message1 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message2 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+ MailboxMessage message3 = createMessage(mailboxId,
ThreadId.fromBaseMessageId(newBasedMessageId));
+
+ Mailbox mailbox = new Mailbox(MailboxPath.inbox(USER), UID_VALIDITY,
mailboxId);
+ messageMapper.add(mailbox, message1);
+ messageMapper.add(mailbox, message2);
+ messageMapper.add(mailbox, message3);
+
+ Flux<MessageId> messageIds =
testee.getMessageIdsInThread(ThreadId.fromBaseMessageId(newBasedMessageId),
mailboxSession);
+
+
assertThat(messageIds.collectList().block()).isEqualTo(List.of(message1.getMessageId(),
message2.getMessageId(), message3.getMessageId()));
Review comment:
Indent
--
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]
