This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch 3.9.x in repository https://gitbox.apache.org/repos/asf/james-project.git
commit d8b2861360f4fd03c14d954cfde97e65acd40f04 Author: Benoit TELLIER <[email protected]> AuthorDate: Tue Jun 16 15:57:16 2026 +0200 [FIX] User rename was returning wrong quota when target address is not empty --- server/container/mailbox-adapter/pom.xml | 4 + .../mailbox/QuotaUsernameChangeTaskStep.java | 50 +------ .../mailbox/QuotaUsernameChangeTaskStepTest.java | 161 ++++++++++----------- 3 files changed, 89 insertions(+), 126 deletions(-) diff --git a/server/container/mailbox-adapter/pom.xml b/server/container/mailbox-adapter/pom.xml index 28f6b60aeb..2180ce46fe 100644 --- a/server/container/mailbox-adapter/pom.xml +++ b/server/container/mailbox-adapter/pom.xml @@ -58,6 +58,10 @@ <groupId>${james.groupId}</groupId> <artifactId>apache-james-mailbox-store</artifactId> </dependency> + <dependency> + <groupId>${james.groupId}</groupId> + <artifactId>apache-james-mailbox-tools-quota-recompute</artifactId> + </dependency> <dependency> <groupId>${james.groupId}</groupId> <artifactId>event-bus-api</artifactId> diff --git a/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStep.java b/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStep.java index abc439de45..7614771f5c 100644 --- a/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStep.java +++ b/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStep.java @@ -19,49 +19,37 @@ package org.apache.james.adapter.mailbox; -import java.time.Instant; import java.util.Optional; import jakarta.inject.Inject; import org.apache.james.core.Username; -import org.apache.james.events.EventBus; -import org.apache.james.events.RegistrationKey; -import org.apache.james.mailbox.model.CurrentQuotas; import org.apache.james.mailbox.model.Quota; -import org.apache.james.mailbox.model.QuotaOperation; import org.apache.james.mailbox.model.QuotaRoot; -import org.apache.james.mailbox.quota.CurrentQuotaManager; import org.apache.james.mailbox.quota.MaxQuotaManager; import org.apache.james.mailbox.quota.QuotaManager; import org.apache.james.mailbox.quota.UserQuotaRootResolver; -import org.apache.james.mailbox.store.event.EventFactory; +import org.apache.james.mailbox.quota.task.RecomputeMailboxCurrentQuotasService; import org.apache.james.user.api.UsernameChangeTaskStep; import org.reactivestreams.Publisher; -import com.google.common.collect.ImmutableSet; - import reactor.core.publisher.Mono; public class QuotaUsernameChangeTaskStep implements UsernameChangeTaskStep { - private static final ImmutableSet<RegistrationKey> NO_REGISTRATION_KEYS = ImmutableSet.of(); private final QuotaManager quotaManager; private final MaxQuotaManager maxQuotaManager; - private final CurrentQuotaManager currentQuotaManager; + private final RecomputeMailboxCurrentQuotasService recomputeMailboxCurrentQuotasService; private final UserQuotaRootResolver userQuotaRootResolver; - private final EventBus eventBus; @Inject public QuotaUsernameChangeTaskStep(QuotaManager quotaManager, - CurrentQuotaManager currentQuotaManager, + RecomputeMailboxCurrentQuotasService recomputeMailboxCurrentQuotasService, UserQuotaRootResolver userQuotaRootResolver, - MaxQuotaManager maxQuotaManager, - EventBus eventBus) { + MaxQuotaManager maxQuotaManager) { this.quotaManager = quotaManager; - this.currentQuotaManager = currentQuotaManager; + this.recomputeMailboxCurrentQuotasService = recomputeMailboxCurrentQuotasService; this.userQuotaRootResolver = userQuotaRootResolver; this.maxQuotaManager = maxQuotaManager; - this.eventBus = eventBus; } @Override @@ -78,13 +66,8 @@ public class QuotaUsernameChangeTaskStep implements UsernameChangeTaskStep { public Publisher<Void> changeUsername(Username oldUsername, Username newUsername) { return Mono.from(quotaManager.getQuotasReactive(userQuotaRootResolver.forUser(oldUsername))) .flatMap(quotas -> Mono.fromCallable(() -> userQuotaRootResolver.forUser(newUsername)) - .flatMap(newUserQuotaRoot -> setQuotaForNewUser(newUserQuotaRoot, quotas) - .then(dispatchNewEventQuota(newUserQuotaRoot, newUsername)))); - } - - private Mono<Void> setQuotaForNewUser(QuotaRoot quotaRoot, QuotaManager.Quotas quotas) { - return setMaxQuota(quotaRoot, quotas) - .then(setCurrentQuota(quotaRoot, quotas)); + .flatMap(newUserQuotaRoot -> setMaxQuota(newUserQuotaRoot, quotas))) + .then(Mono.from(recomputeMailboxCurrentQuotasService.recomputeCurrentQuotas(newUsername))); } private Mono<Void> setMaxQuota(QuotaRoot quotaRoot, QuotaManager.Quotas quotas) { @@ -104,23 +87,4 @@ public class QuotaUsernameChangeTaskStep implements UsernameChangeTaskStep { .get(Quota.Scope.User))) .flatMap(quotaCountLimit -> Mono.from(maxQuotaManager.setMaxMessageReactive(quotaRoot, quotaCountLimit))); } - - private Mono<Void> setCurrentQuota(QuotaRoot quotaRoot, QuotaManager.Quotas quotas) { - return Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(quotaRoot, - new CurrentQuotas(quotas.getMessageQuota().getUsed(), quotas.getStorageQuota().getUsed())))); - } - - private Mono<Void> dispatchNewEventQuota(QuotaRoot quotaRoot, Username username) { - return Mono.from(quotaManager.getQuotasReactive(quotaRoot)) - .flatMap(quotas -> eventBus.dispatch( - EventFactory.quotaUpdated() - .randomEventId() - .user(username) - .quotaRoot(quotaRoot) - .quotaCount(quotas.getMessageQuota()) - .quotaSize(quotas.getStorageQuota()) - .instant(Instant.now()) - .build(), - NO_REGISTRATION_KEYS)); - } } diff --git a/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStepTest.java b/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStepTest.java index 8c789c6f0b..d1bf2ee9e1 100644 --- a/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStepTest.java +++ b/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/QuotaUsernameChangeTaskStepTest.java @@ -24,6 +24,7 @@ import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.SoftAssertions.assertSoftly; import static org.awaitility.Durations.TEN_SECONDS; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import org.apache.james.core.Username; @@ -35,17 +36,22 @@ import org.apache.james.events.Event; import org.apache.james.events.EventBus; import org.apache.james.events.EventListener; import org.apache.james.events.Group; +import org.apache.james.mailbox.MailboxSession; +import org.apache.james.mailbox.MessageManager; import org.apache.james.mailbox.events.GenericGroup; import org.apache.james.mailbox.events.MailboxEvents; +import org.apache.james.mailbox.inmemory.InMemoryMailboxManager; import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources; import org.apache.james.mailbox.model.CurrentQuotas; +import org.apache.james.mailbox.model.MailboxPath; import org.apache.james.mailbox.model.Quota; -import org.apache.james.mailbox.model.QuotaOperation; import org.apache.james.mailbox.model.QuotaRoot; import org.apache.james.mailbox.quota.CurrentQuotaManager; import org.apache.james.mailbox.quota.MaxQuotaManager; import org.apache.james.mailbox.quota.QuotaManager; import org.apache.james.mailbox.quota.UserQuotaRootResolver; +import org.apache.james.mailbox.quota.task.RecomputeMailboxCurrentQuotasService; +import org.apache.james.mime4j.dom.Message; import org.awaitility.Awaitility; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -55,8 +61,11 @@ import reactor.core.publisher.Mono; class QuotaUsernameChangeTaskStepTest { private static final Username ALICE = Username.of("alice"); private static final Username BOB = Username.of("bob"); + // Quotas resulting from a single appended message, see appendAMessage. + private static final CurrentQuotas ONE_MESSAGE_QUOTAS = new CurrentQuotas(QuotaCountUsage.count(1L), QuotaSizeUsage.size(103L)); private QuotaUsernameChangeTaskStep testee; + private InMemoryMailboxManager mailboxManager; private QuotaManager quotaManager; private CurrentQuotaManager currentQuotaManager; private UserQuotaRootResolver quotaRootResolver; @@ -67,6 +76,7 @@ class QuotaUsernameChangeTaskStepTest { @BeforeEach void setUp() { InMemoryIntegrationResources resources = InMemoryIntegrationResources.defaultResources(); + mailboxManager = resources.getMailboxManager(); quotaManager = resources.getQuotaManager(); currentQuotaManager = resources.getCurrentQuotaManager(); quotaRootResolver = resources.getDefaultUserQuotaRootResolver(); @@ -88,84 +98,85 @@ class QuotaUsernameChangeTaskStepTest { } }); - testee = new QuotaUsernameChangeTaskStep( - quotaManager, + RecomputeMailboxCurrentQuotasService recomputeMailboxCurrentQuotasService = new RecomputeMailboxCurrentQuotasService( currentQuotaManager, + resources.getCurrentQuotaCalculator(), quotaRootResolver, - maxQuotaManager, + mailboxManager.getSessionProvider(), + mailboxManager, + quotaManager, eventBus); + + testee = new QuotaUsernameChangeTaskStep( + quotaManager, + recomputeMailboxCurrentQuotasService, + quotaRootResolver, + maxQuotaManager); } @Test - void shouldMigrateQuotas() throws Exception { + void shouldMigrateMaxQuotas() throws Exception { QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.count(50)); maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.size(100)); - Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(bobQuotaRoot, new CurrentQuotas( - QuotaCountUsage.count(5), QuotaSizeUsage.size(10) - )))).block(); Mono.from(testee.changeUsername(BOB, ALICE)).block(); QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(5)).computedLimit(QuotaCountLimit.count(50)).build()); - - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(10)).computedLimit(QuotaSizeLimit.size(100)).build()); + softly.assertThat(aliceQuotas.getMessageQuota().getLimit()).isEqualTo(QuotaCountLimit.count(50)); + softly.assertThat(aliceQuotas.getStorageQuota().getLimit()).isEqualTo(QuotaSizeLimit.size(100)); }); } @Test - void migrateShouldNotThrowWhenNoQuotas() throws Exception { - assertThatCode(() -> Mono.from(testee.changeUsername(BOB, ALICE)).block()) - .doesNotThrowAnyException(); - QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); - assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(0)).computedLimit(QuotaCountLimit.unlimited()).build()); + void shouldRecomputeCurrentQuotasFromNewUserMailboxes() throws Exception { + appendAMessage(ALICE); - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(0)).computedLimit(QuotaSizeLimit.unlimited()).build()); - }); + Mono.from(testee.changeUsername(BOB, ALICE)).block(); + + assertThat(Mono.from(currentQuotaManager.getCurrentQuotas(quotaRootResolver.forUser(ALICE))).block()) + .isEqualTo(ONE_MESSAGE_QUOTAS); } @Test - void migrateShouldSucceedWhenUnlimitedQuotas() throws Exception { + void shouldNotOverwriteDestinationCurrentQuotasWithSourceOnes() throws Exception { + // Source (BOB) holds a current quota value... QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); - maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.unlimited()); - maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.unlimited()); - Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(bobQuotaRoot, new CurrentQuotas( - QuotaCountUsage.count(5), QuotaSizeUsage.size(10) - )))).block(); + maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.count(50)); + maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.size(100)); + + // ...while the destination (ALICE) already has its own content. + appendAMessage(ALICE); Mono.from(testee.changeUsername(BOB, ALICE)).block(); - QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); - assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(5)).computedLimit(QuotaCountLimit.unlimited()).build()); + // ALICE current quotas reflect her actual mailboxes content, not BOB's figures. + assertThat(Mono.from(currentQuotaManager.getCurrentQuotas(quotaRootResolver.forUser(ALICE))).block()) + .isEqualTo(ONE_MESSAGE_QUOTAS); + } - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(10)).computedLimit(QuotaSizeLimit.unlimited()).build()); - }); + @Test + void migrateShouldNotThrowWhenNoQuotas() throws Exception { + assertThatCode(() -> Mono.from(testee.changeUsername(BOB, ALICE)).block()) + .doesNotThrowAnyException(); + + assertThat(Mono.from(currentQuotaManager.getCurrentQuotas(quotaRootResolver.forUser(ALICE))).block()) + .isEqualTo(CurrentQuotas.emptyQuotas()); } @Test void migrateShouldSucceedWhenOnlyMessagesQuota() throws Exception { QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.count(10)); + Mono.from(testee.changeUsername(BOB, ALICE)).block(); QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(0)).computedLimit(QuotaCountLimit.count(10)).build()); - - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(0)).computedLimit(QuotaSizeLimit.unlimited()).build()); + softly.assertThat(aliceQuotas.getMessageQuota().getLimit()).isEqualTo(QuotaCountLimit.count(10)); + softly.assertThat(aliceQuotas.getStorageQuota().getLimit()).isEqualTo(QuotaSizeLimit.unlimited()); }); } @@ -173,44 +184,13 @@ class QuotaUsernameChangeTaskStepTest { void migrateShouldSucceedWhenOnlyStorageQuota() throws Exception { QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.size(10)); - Mono.from(testee.changeUsername(BOB, ALICE)).block(); - - QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); - assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(0)).computedLimit(QuotaCountLimit.unlimited()).build()); - - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(0)).computedLimit(QuotaSizeLimit.size(10)).build()); - }); - } - - @Test - void migrateShouldSucceedWhenAliceAlreadyQuotas() throws Exception { - QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); - maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.count(50)); - maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.size(100)); - Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(bobQuotaRoot, new CurrentQuotas( - QuotaCountUsage.count(5), QuotaSizeUsage.size(10) - )))).block(); - - QuotaRoot aliceQuotaRoot = quotaRootResolver.forUser(ALICE); - maxQuotaManager.setMaxMessage(aliceQuotaRoot, QuotaCountLimit.count(55)); - maxQuotaManager.setMaxStorage(aliceQuotaRoot, QuotaSizeLimit.size(150)); - Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(aliceQuotaRoot, new CurrentQuotas( - QuotaCountUsage.count(7), QuotaSizeUsage.size(8) - )))).block(); Mono.from(testee.changeUsername(BOB, ALICE)).block(); QuotaManager.Quotas aliceQuotas = quotaManager.getQuotas(quotaRootResolver.forUser(ALICE)); - assertSoftly(softly -> { - softly.assertThat(aliceQuotas.getMessageQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(5)).computedLimit(QuotaCountLimit.count(50)).build()); - - softly.assertThat(aliceQuotas.getStorageQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(10)).computedLimit(QuotaSizeLimit.size(100)).build()); + softly.assertThat(aliceQuotas.getMessageQuota().getLimit()).isEqualTo(QuotaCountLimit.unlimited()); + softly.assertThat(aliceQuotas.getStorageQuota().getLimit()).isEqualTo(QuotaSizeLimit.size(10)); }); } @@ -219,28 +199,43 @@ class QuotaUsernameChangeTaskStepTest { QuotaRoot bobQuotaRoot = quotaRootResolver.forUser(BOB); maxQuotaManager.setMaxMessage(bobQuotaRoot, QuotaCountLimit.count(50)); maxQuotaManager.setMaxStorage(bobQuotaRoot, QuotaSizeLimit.size(100)); - Mono.from(currentQuotaManager.setCurrentQuotas(QuotaOperation.from(bobQuotaRoot, new CurrentQuotas( - QuotaCountUsage.count(5), QuotaSizeUsage.size(10) - )))).block(); - Mono.from(testee.changeUsername(BOB, ALICE)).block(); + appendAMessage(ALICE); + Mono.from(testee.changeUsername(BOB, ALICE)).block(); Awaitility.await() .atMost(TEN_SECONDS) - .untilAsserted(() -> assertThat(eventStore.size()).isEqualTo(1)); + .untilAsserted(() -> assertThat(eventStore).anyMatch(event -> event instanceof MailboxEvents.QuotaUsageUpdatedEvent)); - MailboxEvents.QuotaUsageUpdatedEvent quotaUsageUpdatedEvent = (MailboxEvents.QuotaUsageUpdatedEvent) eventStore.get(0); + MailboxEvents.QuotaUsageUpdatedEvent quotaUsageUpdatedEvent = eventStore.stream() + .filter(event -> event instanceof MailboxEvents.QuotaUsageUpdatedEvent) + .map(MailboxEvents.QuotaUsageUpdatedEvent.class::cast) + .reduce((first, second) -> second) + .orElseThrow(); assertSoftly(softly -> { softly.assertThat(quotaUsageUpdatedEvent.getCountQuota()) - .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(5)).computedLimit(QuotaCountLimit.count(50)).build()); + .isEqualTo(Quota.<QuotaCountLimit, QuotaCountUsage>builder().used(QuotaCountUsage.count(1)).computedLimit(QuotaCountLimit.count(50)).build()); softly.assertThat(quotaUsageUpdatedEvent.getSizeQuota()) - .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(10)).computedLimit(QuotaSizeLimit.size(100)).build()); + .isEqualTo(Quota.<QuotaSizeLimit, QuotaSizeUsage>builder().used(QuotaSizeUsage.size(103)).computedLimit(QuotaSizeLimit.size(100)).build()); softly.assertThat(quotaUsageUpdatedEvent.getUsername()) .isEqualTo(ALICE); softly.assertThat(quotaUsageUpdatedEvent.getQuotaRoot()) .isEqualTo(quotaRootResolver.forUser(ALICE)); }); } -} \ No newline at end of file + + private void appendAMessage(Username username) throws Exception { + MailboxSession session = mailboxManager.createSystemSession(username); + MailboxPath inbox = MailboxPath.inbox(username); + mailboxManager.createMailbox(inbox, session); + MessageManager messageManager = mailboxManager.getMailbox(inbox, session); + messageManager.appendMessage(MessageManager.AppendCommand.from( + Message.Builder.of() + .setTo("[email protected]") + .setBody("This is a message", StandardCharsets.UTF_8)), + session); + mailboxManager.endProcessingRequest(session); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
