You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/05/28 06:36:36 UTC
[GitHub] [james-project] quantranhong1999 opened a new pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
quantranhong1999 opened a new pull request #460:
URL: https://github.com/apache/james-project/pull/460
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-851164920
```
java.lang.AssertionError:
Expecting map:
{user=aesw}
to contain:
[MapEntry[key=user, value=aw]]
but could not find the following map entries:
[MapEntry[key=user, value=aw]]
at org.apache.james.mailbox.store.mail.model.MailboxMapperACLTest.removeAclShouldNotFailWhenRemovingNonExistingRight(MailboxMapperACLTest.java:209)
```
It's falling somewhere else, i will continue to read the test to understand it and find out why it's faliing.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-852831245
Crap it looks like we have a merge conflict...
Can you have a look Quan?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-854616970
Done
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-851166673
Reviewed. Nothing to add
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #460:
URL: https://github.com/apache/james-project/pull/460
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #460:
URL: https://github.com/apache/james-project/pull/460#discussion_r641337354
##########
File path: mailbox/api/src/main/java/org/apache/james/mailbox/MessageManager.java
##########
@@ -152,11 +153,13 @@
private final ComposedMessageId id;
private final Long size;
private final Optional<List<MessageAttachmentMetadata>> messageAttachments;
+ private final ThreadId threadId;
public AppendResult(ComposedMessageId id, Long size, Optional<List<MessageAttachmentMetadata>> messageAttachments) {
this.id = id;
this.size = size;
this.messageAttachments = messageAttachments;
+ this.threadId = new ThreadId(id.getMessageId());
Review comment:
This feels wrong.
1. We can have a factory method for ThreadId to make things more explicit:
```
this.threadId = ThreadId.forBaseMessageId(id.getMessageId());
```
2. We are leaking some business decisions all around our data model.
`new ThreadId(id.getMessageId())` is a decision that I would love to see taken only once.
This means we need to cary over the threadId along in the data model. We need to
- Have a constructor parameter for `threadId` here in `AppendResult`
- Cary the threadId parameter over in `MessageMetaData` POJO.
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSetCreatePerformer.scala
##########
@@ -102,7 +102,8 @@ class EmailSetCreatePerformer @Inject()(serializer: EmailSetSerializer,
.appendMessage(appendCommand, mailboxSession)
val blobId: Option[BlobId] = BlobId.of(appendResult.getId.getMessageId).toOption
- CreationSuccess(clientId, EmailCreationResponse(appendResult.getId.getMessageId, blobId, blobId, Email.sanitizeSize(appendResult.getSize)))
+ val threadId: ThreadId = ThreadId(appendResult.getThreadId.serialize)
Review comment:
I would prefer:
```
ThreadId.fromJava(appendResult.getThreadId)
```
##########
File path: mailbox/api/src/main/java/org/apache/james/mailbox/MessageManager.java
##########
@@ -172,6 +175,10 @@ public Long getSize() {
return messageAttachments.get();
}
+ public ThreadId getThreadId() {
+ return threadId;
+ }
+
@Override
public final boolean equals(Object o) {
if (o instanceof AppendResult) {
Review comment:
We do not keep threadId into account for equals & hashCode?
Is that POJO equalsVerifer tested?
Can you fix it?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-854608629
Can someone rerun the CI please? The failed tests are green on my local (rebased lastest commit).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #460:
URL: https://github.com/apache/james-project/pull/460#issuecomment-851788329
@Arsnael maybe a little extra review please?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #460:
URL: https://github.com/apache/james-project/pull/460#discussion_r642207968
##########
File path: mailbox/event/json/src/test/java/org/apache/james/event/json/AddedSerializationTest.java
##########
@@ -66,7 +67,7 @@
.add("User Custom Flag")
.build();
private static final SortedMap<MessageUid, MessageMetaData> ADDED = ImmutableSortedMap.of(
- MESSAGE_UID, new MessageMetaData(MESSAGE_UID, MOD_SEQ, FLAGS, SIZE, Date.from(INSTANT), MESSAGE_ID));
+ MESSAGE_UID, new MessageMetaData(MESSAGE_UID, MOD_SEQ, FLAGS, SIZE, Date.from(INSTANT), MESSAGE_ID, ThreadId.fromBaseMessageId(MESSAGE_ID)));
Review comment:
Maybe it could be nice in serialization test to have a different value for messgeId and threadId, to show that it is supported?
##########
File path: mailbox/event/json/src/test/java/org/apache/james/event/json/ExpungedSerializationTest.java
##########
@@ -89,13 +90,40 @@
" \"userFlags\":[\"User Custom Flag\"]}," +
" \"size\": 45, " +
" \"internalDate\": \"2018-12-14T09:41:51.541Z\"," +
- " \"messageId\": \"42\"" +
+ " \"messageId\": \"42\"," +
+ " \"threadId\": \"42\"" +
Review comment:
Idem, can't we have distinct values?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #460:
URL: https://github.com/apache/james-project/pull/460#discussion_r641474315
##########
File path: mailbox/event/json/src/test/java/org/apache/james/event/json/ExpungedSerializationTest.java
##########
@@ -89,7 +90,8 @@
" \"userFlags\":[\"User Custom Flag\"]}," +
" \"size\": 45, " +
" \"internalDate\": \"2018-12-14T09:41:51.541Z\"," +
- " \"messageId\": \"42\"" +
+ " \"messageId\": \"42\"," +
+ " \"threadId\": \"42\"" +
Review comment:
This is a breaking change. We need to handle the old format.
Please add the test.
##########
File path: mailbox/event/json/src/test/java/org/apache/james/event/json/AddedSerializationTest.java
##########
@@ -88,7 +89,8 @@
" \"userFlags\":[\"User Custom Flag\"]}," +
" \"size\": 45, " +
" \"internalDate\": \"2018-12-14T09:41:51.541Z\"," +
- " \"messageId\": \"42\"" +
+ " \"messageId\": \"42\"," +
+ " \"threadId\":\"42\"" +
Review comment:
This is a breaking change. We need to handle the old format.
Please add the test.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #460: JAMES-3516 Exposing AppendResult::getThreadId allow advertising the threadId as part of Email/set and Email/Import results
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #460:
URL: https://github.com/apache/james-project/pull/460#discussion_r642206317
##########
File path: mailbox/event/json/src/main/scala/org/apache/james/event/json/DTOs.scala
##########
@@ -91,11 +91,12 @@ object DTOs {
javaMessageMetaData.getSize,
javaMessageMetaData.getInternalDate.toInstant,
javaMessageMetaData.getMessageId,
- javaMessageMetaData.getThreadId)
+ Option(javaMessageMetaData.getThreadId))
}
- case class MessageMetaData(uid: MessageUid, modSeq: ModSeq, flags: Flags, size: Long, internalDate: Instant, messageId: MessageId, threadId: ThreadId) {
- def toJava: JavaMessageMetaData = new JavaMessageMetaData(uid, modSeq, Flags.toJavaFlags(flags), size, Date.from(internalDate), messageId, threadId)
+ case class MessageMetaData(uid: MessageUid, modSeq: ModSeq, flags: Flags, size: Long, internalDate: Instant, messageId: MessageId, threadId: Option[ThreadId]) {
+ def toJava: JavaMessageMetaData = new JavaMessageMetaData(uid, modSeq, Flags.toJavaFlags(flags), size, Date.from(internalDate), messageId, getThreadIdElseFallbackToOldFormat)
+ def getThreadIdElseFallbackToOldFormat: ThreadId = threadId.getOrElse(ThreadId.fromBaseMessageId(messageId))
Review comment:
getThreadIdElseFallbackToOldFormat => retrieveThreadId ?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org