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 2022/05/24 04:45:39 UTC
[GitHub] [james-project] vttranlina commented on a diff in pull request #1011: JAMES-3737 Reactive IMAP CREATE + RENAME
vttranlina commented on code in PR #1011:
URL: https://github.com/apache/james-project/pull/1011#discussion_r880040204
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java:
##########
@@ -334,94 +332,102 @@ private boolean userHasLookupRightsOn(Mailbox mailbox, MailboxSession session) {
@Override
public Optional<MailboxId> createMailbox(MailboxPath mailboxPath, MailboxSession mailboxSession) throws MailboxException {
+ return MailboxReactorUtils.blockOptional(createMailboxReactive(mailboxPath, mailboxSession));
+ }
+
+ public Mono<MailboxId> createMailboxReactive(MailboxPath mailboxPath, MailboxSession mailboxSession) {
LOGGER.debug("createMailbox {}", mailboxPath);
- assertMailboxPathBelongToUser(mailboxSession, mailboxPath);
+ return assertMailboxPathBelongToUserReactive(mailboxSession, mailboxPath)
+ .then(doCreateMailboxReactive(mailboxPath, mailboxSession));
+ }
+ private Mono<MailboxId> doCreateMailboxReactive(MailboxPath mailboxPath, MailboxSession mailboxSession) {
if (mailboxPath.getName().isEmpty()) {
LOGGER.warn("Ignoring mailbox with empty name");
+ return Mono.empty();
} else {
- MailboxPath sanitizedMailboxPath = mailboxPath.sanitize(mailboxSession.getPathDelimiter());
- sanitizedMailboxPath.assertAcceptable(mailboxSession.getPathDelimiter());
-
- if (block(mailboxExists(sanitizedMailboxPath, mailboxSession))) {
- throw new MailboxExistsException(sanitizedMailboxPath.asString());
- }
-
- List<MailboxId> mailboxIds = createMailboxesForPath(mailboxSession, sanitizedMailboxPath);
-
- if (!mailboxIds.isEmpty()) {
- return Optional.ofNullable(Iterables.getLast(mailboxIds));
+ try {
+ MailboxPath sanitizedMailboxPath = mailboxPath.sanitize(mailboxSession.getPathDelimiter());
+ sanitizedMailboxPath.assertAcceptable(mailboxSession.getPathDelimiter());
+
+ return mailboxExists(sanitizedMailboxPath, mailboxSession)
+ .flatMap(exists -> {
+ if (exists) {
+ return Mono.error(new MailboxExistsException(sanitizedMailboxPath.asString()));
+ } else {
+ return createMailboxesForPath(mailboxSession, sanitizedMailboxPath).last();
+ }
+ });
+ } catch (MailboxNameException e) {
+ return Mono.error(e);
}
Review Comment:
```suggestion
return Mono.fromCallable(() -> mailboxPath.sanitize(mailboxSession.getPathDelimiter()))
.map(Throwing.function(sanitizedMailboxPath -> sanitizedMailboxPath.assertAcceptable(mailboxSession.getPathDelimiter())))
.flatMap(sanitizedMailboxPath -> mailboxExists(sanitizedMailboxPath, mailboxSession)
.flatMap(exists -> {
if (exists) {
return Mono.error(new MailboxExistsException(sanitizedMailboxPath.asString()));
} else {
return createMailboxesForPath(mailboxSession, sanitizedMailboxPath).last();
}
}))
.onErrorResume(MailboxNameException.class, Mono::error);
```
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java:
##########
@@ -334,94 +332,102 @@ private boolean userHasLookupRightsOn(Mailbox mailbox, MailboxSession session) {
@Override
public Optional<MailboxId> createMailbox(MailboxPath mailboxPath, MailboxSession mailboxSession) throws MailboxException {
+ return MailboxReactorUtils.blockOptional(createMailboxReactive(mailboxPath, mailboxSession));
+ }
+
+ public Mono<MailboxId> createMailboxReactive(MailboxPath mailboxPath, MailboxSession mailboxSession) {
LOGGER.debug("createMailbox {}", mailboxPath);
- assertMailboxPathBelongToUser(mailboxSession, mailboxPath);
+ return assertMailboxPathBelongToUserReactive(mailboxSession, mailboxPath)
+ .then(doCreateMailboxReactive(mailboxPath, mailboxSession));
+ }
+ private Mono<MailboxId> doCreateMailboxReactive(MailboxPath mailboxPath, MailboxSession mailboxSession) {
if (mailboxPath.getName().isEmpty()) {
LOGGER.warn("Ignoring mailbox with empty name");
+ return Mono.empty();
} else {
- MailboxPath sanitizedMailboxPath = mailboxPath.sanitize(mailboxSession.getPathDelimiter());
- sanitizedMailboxPath.assertAcceptable(mailboxSession.getPathDelimiter());
-
- if (block(mailboxExists(sanitizedMailboxPath, mailboxSession))) {
- throw new MailboxExistsException(sanitizedMailboxPath.asString());
- }
-
- List<MailboxId> mailboxIds = createMailboxesForPath(mailboxSession, sanitizedMailboxPath);
-
- if (!mailboxIds.isEmpty()) {
- return Optional.ofNullable(Iterables.getLast(mailboxIds));
+ try {
+ MailboxPath sanitizedMailboxPath = mailboxPath.sanitize(mailboxSession.getPathDelimiter());
+ sanitizedMailboxPath.assertAcceptable(mailboxSession.getPathDelimiter());
+
+ return mailboxExists(sanitizedMailboxPath, mailboxSession)
+ .flatMap(exists -> {
+ if (exists) {
+ return Mono.error(new MailboxExistsException(sanitizedMailboxPath.asString()));
+ } else {
+ return createMailboxesForPath(mailboxSession, sanitizedMailboxPath).last();
+ }
+ });
+ } catch (MailboxNameException e) {
+ return Mono.error(e);
}
}
- return Optional.empty();
}
- private List<MailboxId> createMailboxesForPath(MailboxSession mailboxSession, MailboxPath sanitizedMailboxPath) {
+ private Flux<MailboxId> createMailboxesForPath(MailboxSession mailboxSession, MailboxPath sanitizedMailboxPath) {
// Create parents first
// If any creation fails then the mailbox will not be created
// TODO: transaction
List<MailboxPath> intermediatePaths = sanitizedMailboxPath.getHierarchyLevels(getDelimiter());
boolean isRootPath = intermediatePaths.size() == 1;
- return intermediatePaths
- .stream()
- .flatMap(Throwing.<MailboxPath, Stream<MailboxId>>function(mailboxPath -> manageMailboxCreation(mailboxSession, isRootPath, mailboxPath)).sneakyThrow())
- .collect(ImmutableList.toImmutableList());
+ return Flux.fromIterable(intermediatePaths)
+ .concatMap(path -> manageMailboxCreation(mailboxSession, isRootPath, path));
}
- private Stream<MailboxId> manageMailboxCreation(MailboxSession mailboxSession, boolean isRootPath, MailboxPath mailboxPath) throws MailboxException {
+ private Mono<MailboxId> manageMailboxCreation(MailboxSession mailboxSession, boolean isRootPath, MailboxPath mailboxPath) {
if (mailboxPath.isInbox()) {
- if (block(hasInbox(mailboxSession))) {
- return duplicatedINBOXCreation(isRootPath, mailboxPath);
- }
-
- return performConcurrentMailboxCreation(mailboxSession, MailboxPath.inbox(mailboxSession)).stream();
+ return Mono.from(hasInbox(mailboxSession))
+ .flatMap(hasInbox -> {
+ if (hasInbox) {
+ return duplicatedINBOXCreation(isRootPath, mailboxPath);
+ }
+ return performConcurrentMailboxCreation(mailboxSession, MailboxPath.inbox(mailboxSession));
+ });
}
- return performConcurrentMailboxCreation(mailboxSession, mailboxPath).stream();
+ return performConcurrentMailboxCreation(mailboxSession, mailboxPath);
}
- private Stream<MailboxId> duplicatedINBOXCreation(boolean isRootPath, MailboxPath mailbox) throws InboxAlreadyCreated {
+ private Mono<MailboxId> duplicatedINBOXCreation(boolean isRootPath, MailboxPath mailbox) {
if (isRootPath) {
- throw new InboxAlreadyCreated(mailbox.getName());
+ return Mono.error(new InboxAlreadyCreated(mailbox.getName()));
}
- return Stream.empty();
+ return Mono.empty();
}
- private List<MailboxId> performConcurrentMailboxCreation(MailboxSession mailboxSession, MailboxPath mailboxPath) throws MailboxException {
- List<MailboxId> mailboxIds = new ArrayList<>();
+ private Mono<MailboxId> performConcurrentMailboxCreation(MailboxSession mailboxSession, MailboxPath mailboxPath) {
MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(mailboxSession);
- locker.executeWithLock(mailboxPath, () ->
- block(mapper.executeReactive(mapper.create(mailboxPath, UidValidity.generate())
- .doOnNext(mailbox -> mailboxIds.add(mailbox.getMailboxId()))
- .flatMap(mailbox ->
- // notify listeners
- eventBus.dispatch(EventFactory.mailboxAdded()
- .randomEventId()
- .mailboxSession(mailboxSession)
- .mailbox(mailbox)
- .build(),
- new MailboxIdRegistrationKey(mailbox.getMailboxId()))))
- .onErrorResume(e -> {
- if (e instanceof MailboxExistsException) {
- LOGGER.info("{} mailbox was created concurrently", mailboxPath.asString());
- } else if (e instanceof MailboxException) {
- return Mono.error(e);
- }
- return Mono.empty();
- })), MailboxPathLocker.LockType.Write);
-
- return mailboxIds;
- }
-
- private void assertMailboxPathBelongToUser(MailboxSession mailboxSession, MailboxPath mailboxPath) throws MailboxException {
+ return Mono.from(locker.executeReactiveWithLockReactive(mailboxPath,
+ mapper.executeReactive(mapper.create(mailboxPath, UidValidity.generate())
+ .flatMap(mailbox ->
+ // notify listeners
+ eventBus.dispatch(EventFactory.mailboxAdded()
+ .randomEventId()
+ .mailboxSession(mailboxSession)
+ .mailbox(mailbox)
+ .build(),
+ new MailboxIdRegistrationKey(mailbox.getMailboxId()))
+ .thenReturn(mailbox.getMailboxId()))
+ .onErrorResume(e -> {
+ if (e instanceof MailboxExistsException) {
+ LOGGER.info("{} mailbox was created concurrently", mailboxPath.asString());
+ } else if (e instanceof MailboxException) {
+ return Mono.error(e);
+ }
+ return Mono.empty();
+ })), MailboxPathLocker.LockType.Write));
Review Comment:
```suggestion
.onErrorResume(MailboxExistsException.class, e -> {
LOGGER.info("{} mailbox was created concurrently", mailboxPath.asString());
return Mono.empty();
})
.onErrorResume(MailboxException.class, Mono::error)), MailboxPathLocker.LockType.Write));
```
More beauty
--
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