You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by bt...@apache.org on 2020/03/02 03:16:13 UTC
[james-project] 16/29: JAMES-3074 Generate valid,
random based UidValidity
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 014887097decf35bbb34aca694bb46c001cd861b
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Wed Feb 26 11:29:22 2020 +0700
JAMES-3074 Generate valid, random based UidValidity
- store generation trivialy lead to invalid value on `0` (edge case)
- maildir being based on a timestamp was generating too large values
Currently allocated values CAN be invalid, but at the very least we
should stop generating invalid values.
Regarding the choice to be random based:
Despite RFC-3501 recommendations, we chose random as a UidVality
generation mechanism. Timestamp base approach can lead to UidValidity
being the same for two mailboxes simultaneously generated, leading
to some IMPA clients not being able to detect changes.
See https://issues.apache.org/jira/browse/JAMES-3074 for details
---
.../apache/james/mailbox/model/UidValidity.java | 46 +++++++++++++
.../james/mailbox/model/UidValidityTest.java | 80 ++++++++++++++++++++++
.../james/mailbox/maildir/MaildirFolder.java | 7 +-
.../james/mailbox/store/StoreMailboxManager.java | 11 +--
4 files changed, 130 insertions(+), 14 deletions(-)
diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java
index 646b43c..ce6b841 100644
--- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java
+++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java
@@ -19,20 +19,66 @@
package org.apache.james.mailbox.model;
+import java.security.SecureRandom;
+import java.util.function.Supplier;
+
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
public class UidValidity {
+ private static final SecureRandom RANDOM = new SecureRandom();
+ private static final long UPPER_EXCLUSIVE_BOUND = 1L << 32;
+
+ /**
+ * Despite RFC-3501 recommendations, we chose random as a UidVality generation mechanism.
+ * Timestamp base approach can lead to UidValidity being the same for two mailboxes simultaneously generated, leading
+ * to some IMPA clients not being able to detect changes.
+ *
+ * See https://issues.apache.org/jira/browse/JAMES-3074 for details
+ */
+ public static UidValidity random() {
+ return fromSupplier(RANDOM::nextLong);
+ }
+
+ @VisibleForTesting
+ static UidValidity fromSupplier(Supplier<Long> longSupplier) {
+ long randomValue = Math.abs(longSupplier.get());
+ long sanitizedRandomValue = 1 + (randomValue % (UPPER_EXCLUSIVE_BOUND - 1));
+ return ofValid(sanitizedRandomValue);
+ }
+
public static UidValidity of(long uidValidity) {
return new UidValidity(uidValidity);
}
+ public static UidValidity ofValid(long uidValidity) {
+ Preconditions.checkArgument(isValid(uidValidity), "uidValidity needs to be a non-zero unsigned 32-bit integer");
+ return new UidValidity(uidValidity);
+ }
+
+ private static boolean isValid(long uidValidityAsLong) {
+ return uidValidityAsLong > 0 && uidValidityAsLong < UPPER_EXCLUSIVE_BOUND;
+ }
+
private final long uidValidity;
private UidValidity(long uidValidity) {
this.uidValidity = uidValidity;
}
+ public boolean isValid() {
+ // RFC-3501 section
+ /*
+ nz-number = digit-nz *DIGIT
+ ; Non-zero unsigned 32-bit integer
+ ; (0 < n < 4,294,967,296)
+ */
+ return isValid(uidValidity);
+ }
+
+
public long asLong() {
return uidValidity;
}
diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java
index a77e074..f48c527 100644
--- a/mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java
+++ b/mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java
@@ -20,7 +20,12 @@
package org.apache.james.mailbox.model;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import java.util.stream.IntStream;
+
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.RepeatedTest;
import org.junit.jupiter.api.Test;
import nl.jqno.equalsverifier.EqualsVerifier;
@@ -38,4 +43,79 @@ class UidValidityTest {
UidValidity uidValidity = UidValidity.of(expectedValue);
assertThat(uidValidity.asLong()).isEqualTo(expectedValue);
}
+
+ @Test
+ void zeroUidValidityShouldBeInvalid() {
+ assertThat(UidValidity.of(0).isValid()).isFalse();
+ }
+
+ @Test
+ void negativeUidValidityShouldBeInvalid() {
+ assertThat(UidValidity.of(-1).isValid()).isFalse();
+ }
+
+ @Test
+ void tooBigUidValidityShouldBeInvalid() {
+ assertThat(UidValidity.of(4294967296L).isValid()).isFalse();
+ }
+
+ @Test
+ void idValidityShouldBeValid() {
+ assertThat(UidValidity.of(42).isValid()).isTrue();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenZeroIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> 0L))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenNegativeIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> -42L))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenUpperBoundExclusiveIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> 4294967296L))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenUpperBoundInclusiveIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> 4294967295L))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenHigherThanUpperBoundIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> 4294967297L))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ void fromSupplierShouldNotThrowWhenLowerThanUpperBoundIsGenerated() {
+ assertThatCode(() -> UidValidity.fromSupplier(() -> 4294967294L))
+ .doesNotThrowAnyException();
+ }
+
+ // Sampling test
+ @RepeatedTest(10000)
+ void randomShouldGenerateValidValues() {
+ assertThatCode(UidValidity::random)
+ .doesNotThrowAnyException();
+ }
+
+ @Disabled("On average upon generating 1.000.000 UidValidity we notice 125 duplicates")
+ @Test
+ void randomShouldNotLeadToCollision() {
+ int count = 1000000;
+ long distinctCount = IntStream.range(0, count)
+ .mapToObj(any -> UidValidity.random())
+ .distinct()
+ .count();
+
+ assertThat(distinctCount).isEqualTo(count);
+ }
}
diff --git a/mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java b/mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java
index efad990..ac8432f 100644
--- a/mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java
+++ b/mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java
@@ -341,10 +341,9 @@ public class MaildirFolder {
* @return the new uidValidity
*/
private UidValidity resetUidValidity() throws IOException {
- // using the timestamp as uidValidity
- UidValidity timestamp = UidValidity.of(System.currentTimeMillis());
- setUidValidity(timestamp);
- return timestamp;
+ UidValidity uidValidity = UidValidity.random();
+ setUidValidity(uidValidity);
+ return uidValidity;
}
/**
diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java
index 6e61512..e7cd367 100644
--- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java
+++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java
@@ -21,7 +21,6 @@ package org.apache.james.mailbox.store;
import static org.apache.james.mailbox.store.mail.AbstractMessageMapper.UNLIMITED;
-import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumSet;
@@ -106,7 +105,6 @@ public class StoreMailboxManager implements MailboxManager {
private static final Logger LOGGER = LoggerFactory.getLogger(StoreMailboxManager.class);
public static final char SQL_WILDCARD_CHAR = '%';
public static final EnumSet<MessageCapabilities> DEFAULT_NO_MESSAGE_CAPABILITIES = EnumSet.noneOf(MessageCapabilities.class);
- private static final SecureRandom RANDOM = new SecureRandom();
private final StoreRightManager storeRightManager;
private final EventBus eventBus;
@@ -215,13 +213,6 @@ public class StoreMailboxManager implements MailboxManager {
return preDeletionHooks;
}
- /**
- * Generate and return the next uid validity
- */
- protected UidValidity randomUidValidity() {
- return UidValidity.of(Math.abs(RANDOM.nextInt()));
- }
-
@Override
public MailboxSession createSystemSession(Username userName) {
return sessionProvider.createSystemSession(userName);
@@ -382,7 +373,7 @@ public class StoreMailboxManager implements MailboxManager {
MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(mailboxSession);
try {
mapper.execute(Mapper.toTransaction(() -> {
- Mailbox mailbox = mapper.create(mailboxPath, randomUidValidity());
+ Mailbox mailbox = mapper.create(mailboxPath, UidValidity.random());
mailboxIds.add(mailbox.getMailboxId());
// notify listeners
eventBus.dispatch(EventFactory.mailboxAdded()
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org