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/08/10 04:57:50 UTC
[GitHub] [james-project] vttranlina commented on a change in pull request #582: JAMES-3544 JMAP draft uploads should be backed by the UploadRepository
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: notifications-unsubscribe@james.apache.org
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