vttranlina commented on a change in pull request #582:
URL: https://github.com/apache/james-project/pull/582#discussion_r685682628
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageAppender.java
##########
@@ -176,35 +170,29 @@ private Flags getFlags(CreationMessage message) {
}
private ImmutableList<MessageAttachmentMetadata>
getMessageAttachments(MailboxSession session, ImmutableList<Attachment>
attachments) throws MailboxException {
Review comment:
Should we remove `throws MailboxException`?
Because the catch this exception is already
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/BlobManagerImpl.java
##########
@@ -41,15 +41,15 @@
import com.github.fge.lambdas.Throwing;
-public class StoreBlobManager implements BlobManager {
+public class BlobManagerImpl implements BlobManager {
Review comment:
Good name!
Sometime, the old name makes me confuse
##########
File path:
server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/methods/SetMessagesCreationProcessorTest.java
##########
@@ -174,13 +173,13 @@ public void setUp() throws MailboxException,
DomainListException, UnknownHostExc
fakeSystemMailboxesProvider = new TestSystemMailboxesProvider(() ->
optionalOutbox, () -> optionalDrafts);
session = MailboxSessionUtil.create(USER);
- MIMEMessageConverter mimeMessageConverter = new
MIMEMessageConverter(mock(AttachmentContentLoader.class));
- messageAppender = new MessageAppender(mockedMailboxManager,
mockMessageIdManager, mockedAttachmentManager, mimeMessageConverter,
JMAPConfiguration.builder().enable().build());
+ MIMEMessageConverter mimeMessageConverter = new
MIMEMessageConverter(mock(BlobManager.class));
Review comment:
The mock for `BlobManager` is exists, we can reuse it.
Maybe one instance is better.
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/BlobManagerImpl.java
##########
@@ -62,11 +71,29 @@ public BlobId toBlobId(MessageId messageId) {
@Override
public Blob retrieve(BlobId blobId, MailboxSession mailboxSession) throws
MailboxException, BlobNotFoundException {
- return getBlobFromAttachment(blobId, mailboxSession)
- .orElseGet(() -> getBlobFromMessage(blobId, mailboxSession)
+ return getBlobFromUpload(blobId, mailboxSession)
Review comment:
```java
getBlobFromUpload(blobId, mailboxSession)
.or(Throwing.supplier(() -> getBlobFromAttachment(blobId,
mailboxSession)).sneakyThrow())
.or(() -> getBlobFromMessage(blobId, mailboxSession))
.orElseThrow(() -> new BlobNotFoundException(blobId));
```
Not necessary, but it is easier to understand
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageAppender.java
##########
@@ -176,35 +170,29 @@ private Flags getFlags(CreationMessage message) {
}
private ImmutableList<MessageAttachmentMetadata>
getMessageAttachments(MailboxSession session, ImmutableList<Attachment>
attachments) throws MailboxException {
- Map<AttachmentId, AttachmentMetadata> attachmentsById =
attachmentManager.getAttachments(attachments.stream()
- .map(attachment ->
AttachmentId.from(attachment.getBlobId().getRawValue()))
- .collect(ImmutableList.toImmutableList()), session)
+ return attachments
.stream()
-
.collect(ImmutableMap.toImmutableMap(AttachmentMetadata::getAttachmentId,
Function.identity()));
-
- ThrowingFunction<Attachment, Optional<MessageAttachmentMetadata>>
toMessageAttachment = att -> messageAttachment(att, attachmentsById);
-
-
- return attachments.stream()
- .map(Throwing.function(toMessageAttachment).sneakyThrow())
- .flatMap(Optional::stream)
+ .flatMap(attachment -> {
+ try {
+ Blob blob = blobManager.retrieve(attachment.getBlobId(),
session);
+ return Stream.of(MessageAttachmentMetadata.builder()
+ .attachment(AttachmentMetadata.builder()
+
.attachmentId(AttachmentId.from(blob.getBlobId().getRawValue()))
Review comment:
blob.getBlobId().asAttachmentId()
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/AttachmentChecker.java
##########
@@ -63,14 +62,17 @@ public AttachmentChecker(AttachmentManager
attachmentManager) {
private List<BlobId> listAttachmentsNotFound(List<Attachment> attachments,
MailboxSession session) throws MailboxException {
ThrowingPredicate<Attachment> notExists =
- attachment ->
!attachmentManager.exists(getAttachmentId(attachment), session);
+ attachment -> {
+ try {
+ blobManager.retrieve(attachment.getBlobId(), session);
Review comment:
IMO, Retrieving blobs for checking they exist is quite expensive. Should
we have a new method for that?
##########
File path:
server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/BlobManagerImpl.java
##########
@@ -62,11 +71,29 @@ public BlobId toBlobId(MessageId messageId) {
@Override
public Blob retrieve(BlobId blobId, MailboxSession mailboxSession) throws
MailboxException, BlobNotFoundException {
- return getBlobFromAttachment(blobId, mailboxSession)
- .orElseGet(() -> getBlobFromMessage(blobId, mailboxSession)
+ return getBlobFromUpload(blobId, mailboxSession)
+ .or(Throwing.supplier(() -> getBlobFromAttachment(blobId,
mailboxSession)).sneakyThrow())
+ .orElseGet(() -> getBlobFromMessage(blobId, mailboxSession)
.orElseThrow(() -> new BlobNotFoundException(blobId)));
}
+ private Optional<Blob> getBlobFromUpload(BlobId blobId, MailboxSession
mailboxSession) {
+ if (blobId.getRawValue().startsWith(UPLOAD_PREFIX)) {
+ UploadId uploadId =
UploadId.from(blobId.getRawValue().substring(UPLOAD_PREFIX.length()));
Review comment:
`blobId.asUploadId()` ? It can be reuse in future
--
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]
