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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
