JAMES-3074 UidValidity strong typing #3132

Arsnael · 2020-02-20T10:56:01Z

No description provided.

mbaechler

There's a lot of traps with the usage of uidvalidity.
As it's often the case, putting a strong type reveal long-standing real bugs

thanks for this work

mbaechler · 2020-02-20T13:23:12Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java

+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+
+public class UidValidity {


you are missing the actual definition of UIDVALIDITY from IMAP RFC :

nz-number = digit-nz *DIGIT ; Non-zero unsigned 32-bit integer ; (0 < n < 4,294,967,296)

Sadly we don't have unsigned 32-bit integer in Java, it's why we decided to use a long (that can contain the range [0-2^32]). However, you should ensure we can't create a UidValidity object that is outside that range.

Not that easy:

protected int randomUidValidity() { return Math.abs(RANDOM.nextInt()); }

We created many unvalid UID VALIDITY, we need to be still lenient and accept already created fields. However newer UID VALIDITY should match condition matthieu highlight.

You assume returning a negative integer worked. I would argue it did not and we never noticed.

1 minute search : GNOME/evolution@e81e64f#diff-e4a32bc822c57a9a503008fe6358736fR100

Let's fix it. It happens that we can change the existing values if we detect they were invalid and it will trigger a client re-sync. Not that bad, right?

Not that bad, but this needs low level support (update if invalid) on every backend.

This is expensive to implement.

So I would:

Create a dedicated JIRA ticket for this "invalid UID_VALIDITY are generated"

Enact the tactical move: merge the strong typing + ensure we don't generate new invalid values. This will at list help with new deployments.

Document the strategical move: fix all and every invalid UID_VALIDITY (at the cost of resynchronicsation)

Would you agree @mbaechler ?

Of course. A small step now is better than nothing at all.

mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxAssertingToolTest.java

mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java

mbaechler · 2020-02-20T13:31:57Z

mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java

        // using the timestamp as uidValidity
-        long timestamp = System.currentTimeMillis();
+        UidValidity timestamp = UidValidity.of(System.currentTimeMillis());


jshell> System.currentTimeMillis()/ Integer.MAX_VALUE $2 ==> 736

That means this piece of code doesn't respect UIDVALIDITY definition.

BTW here I believe this code path SHOULD be using UidValidity.random(), I don't see reasons to be relying on system clock.

Because it's what RFC 3501 advise.
However, not with millis because it doesn't fit in unsigned 32 bit integer.

How can you ensure 2 mailboxes don't get the same value if they are created at the exact same time in two different threads?

It doesn't matter to have 2 differents mailboxes having the same uidvalidity

I don't get it

Imagine... I create mailboxA and mailboxB at the same time. They have same UID_VALIDITY, let say 36.

Now I fetch my mails, then shut thunderbird down.

Then I rename mailboxA in mailboxC
Rename mailboxB in mailboxA
Rename mailboxC in mailboxB

Now I turn thunderbird on again. Fetch the mailboxes again and sees mailboxA with UIDVALIDITY 36 + mailboxB with UID validity 36 and don't know it SHOULD drop it's cache, and resynchronise...

Yet another example about IMAP RFCs mess IMO. That's why I think keeping this in our code, even if it is an IMAP RFC advice is not wise.

Oh, good scenario, it demonstrates UIDVALIDITY is part of mailbox identity and as mailbox path is a weak identifier, you suggest we use UIDVALIDITY as a unique id.

I agree with that solution.

BTW, I already pointed out that using millisecond in UIDVALIDITY is not working neither because it's 32 bits.

Should we keep a record of this discussion somewhere @mbaechler ? ADR? Or link in the ticket?

I would vote for a comment in the code

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java

...otocols/protocols-pop3/src/main/java/org/apache/james/pop3server/mailbox/MailboxAdapter.java

Arsnael · 2020-02-21T01:43:42Z

Hmmm I must agree I just had in mind to strong type what we already have, while completely missing the real purpose of a UidValidity. This PR definitely needs more work, thanks for the feedbacks :)

.../apache/james/mailbox/elasticsearch/events/ElasticSearchListeningMessageSearchIndexTest.java

chibenwa · 2020-02-21T01:44:36Z

mailbox/maildir/src/main/java/org/apache/james/mailbox/maildir/MaildirFolder.java

        // using the timestamp as uidValidity
-        long timestamp = System.currentTimeMillis();
+        UidValidity timestamp = UidValidity.of(System.currentTimeMillis());


BTW here I believe this code path SHOULD be using UidValidity.random(), I don't see reasons to be relying on system clock.

mbaechler · 2020-02-21T09:24:52Z

Hmmm I must agree I just had in mind to strong type what we already have, while completely missing the real purpose of a UidValidity. This PR definitely needs more work, thanks for the feedbacks :)

We just discovered plenty of bugs just by introducing a type. It means we are so right when we want strong typing: it helps not break invariants and detect when we break them.

I don't expect you to fix everything overnight but we should not let this PR rot because there are real issues to fix.

chibenwa · 2020-02-21T09:35:56Z

I don't expect you to fix everything overnight but we should not let this PR rot because there are real issues to fix.

That's why I believe we need to enact a tactical decision while letting the strategical one for later (I don't think we currently have resources to do fix existing values).

mbaechler · 2020-02-24T10:25:53Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java

@@ -217,8 +218,8 @@ public PreDeletionHooks getPreDeletionHooks() {
    /**
     * Generate and return the next uid validity
     */
-    protected int randomUidValidity() {
-        return Math.abs(RANDOM.nextInt());
+    protected UidValidity randomUidValidity() {


we should have a single way of generating UidValidity objects and it should be UidValidity.create()
We should document that UidValidity.of should be used only for deserialization

Arsnael · 2020-02-25T04:14:39Z

Only did a rebase with master for resolving conflicts

nvduc91 · 2020-02-25T04:35:26Z

Read it

Arsnael · 2020-02-25T04:43:14Z

[e04a9dfe2830dd3a9d635d7a0060cc7aec1a676b] [ERROR] Failures: 
[e04a9dfe2830dd3a9d635d7a0060cc7aec1a676b] [ERROR]   CassandraGenericMailboxMapperTest>MailboxMapperTest.createShouldPersistTheMailbox:93 Expected UID Validity to be <UidValidity{uidValidity=42}> but was <UidValidity{uidValidity=42}>
[e04a9dfe2830dd3a9d635d7a0060cc7aec1a676b] [ERROR]   CassandraGenericMailboxMapperTest>MailboxMapperTest.findMailboxByIdShouldReturnExistingMailbox:251 Expected UID Validity to be <UidValidity{uidValidity=42}> but was <UidValidity{uidValidity=42}>
[e04a9dfe2830dd3a9d635d7a0060cc7aec1a676b] [ERROR]   CassandraGenericMailboxMapperTest>MailboxMapperTest.renameShouldRenameTheMailbox:126 Expected UID Validity to be <UidValidity{uidValidity=42}> but was <UidValidity{uidValidity=42}>

trantienduchn · 2020-02-25T06:31:05Z

Read it

remk · 2020-02-25T14:06:20Z

Read it. Good initiative !

chibenwa · 2020-02-26T04:34:59Z

(forced pushed to modify ticket number in initial commits)

…ping

mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java

Arsnael · 2020-02-26T08:30:07Z

I approve the changes, thanks @chibenwa for the help !

remk · 2020-02-26T10:54:23Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java

+            .doesNotThrowAnyException();
+    }
+
+    @Disabled("On average upon generating 1.000.000 UidValidity we notice 125 duplicates")


What are the implications ? Can you state it in the comment ?
And if it is acceptable maybe the test could be one asserting that we have less than 0.0x % of collisions ?

What are the implications ? Can you state it in the comment ?

IMAP clients might be missing some mailbox switches.

And if it is acceptable maybe the test could be one asserting that we have less than 0.0x % of collisions ?

What's the point of testing that IMAP RFC is broken?

Are you sure you have to blame the RFC? (from 3501)

3) If the mailbox is deleted and a new mailbox with the same name is created at a later date, the server must either keep track of unique identifiers from the previous instance of the mailbox, or it must assign a new UIDVALIDITY value to the new instance of the mailbox. A good UIDVALIDITY value to use in this case is a 32-bit representation of the creation date/time of the mailbox. It is alright to use a constant such as 1, but only if it guaranteed that unique identifiers will never be reused, even in the case of a mailbox being deleted (or renamed) and a new mailbox by the same name created at some future time.

IMAP clients might be missing some mailbox switches.

The collision has to happen on a mailbox named like a previous one. I don't see that as a problem given the constraint of 32 bit integer, I can't think of a better solution than random ...

mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java

trantienduchn · 2020-02-26T11:25:56Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java

+    /**
+     * 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.


s/IMPA/IMAP

chibenwa · 2020-02-27T04:49:19Z

test this please

Using Optional is a better pattern for representing a not-yet-known value.

- 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

… base

chibenwa · 2020-02-27T08:41:36Z

Force pushed to solve a conflict

…ping

mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java

mbaechler · 2020-02-27T14:29:53Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java

+            .doesNotThrowAnyException();
+    }
+
+    @Disabled("On average upon generating 1.000.000 UidValidity we notice 125 duplicates")


IMAP clients might be missing some mailbox switches.

The collision has to happen on a mailbox named like a previous one. I don't see that as a problem given the constraint of 32 bit integer, I can't think of a better solution than random ...

mbaechler · 2020-02-27T14:31:29Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java

+            .doesNotThrowAnyException();
+    }
+
+    @Disabled("On average upon generating 1.000.000 UidValidity we notice 125 duplicates")


IMAP clients might be missing some mailbox switches.

The collision has to happen on a mailbox named like a previous one. I don't see that as a problem given the constraint of 32 bit integer, I can't think of a better solution than random ...

…ping

chibenwa · 2020-03-02T03:19:33Z

Merged

Arsnael added this to the Sprint 15 - Robusta Beans milestone Feb 20, 2020

Arsnael self-assigned this Feb 20, 2020

Arsnael mentioned this pull request Feb 20, 2020

JAMES-3057 make MailboxId of Mailbox object immutable #3128

Closed

mbaechler reviewed Feb 20, 2020

View reviewed changes

chibenwa reviewed Feb 21, 2020

View reviewed changes

mbaechler reviewed Feb 24, 2020

View reviewed changes

Arsnael force-pushed the j3057-uid-validity-strong-typing branch from 734a6a2 to e04a9df Compare February 25, 2020 04:13

Arsnael added the refactoring Code enhencements that don't alter software behaviour label Feb 25, 2020

Arsnael changed the title ~~[Refactoring] UidValidity strong typing~~ JAMES-3074 [Refactoring] UidValidity strong typing Feb 25, 2020

Arsnael removed the refactoring Code enhencements that don't alter software behaviour label Feb 25, 2020

Arsnael changed the title ~~JAMES-3074 [Refactoring] UidValidity strong typing~~ JAMES-3074 UidValidity strong typing Feb 25, 2020

chibenwa force-pushed the j3057-uid-validity-strong-typing branch 2 times, most recently from 3d1f9bd to e643e08 Compare February 26, 2020 04:34

chibenwa pushed a commit to chibenwa/james-project that referenced this pull request Feb 26, 2020

[NO_REVIEW≠ linagora#3132 JAMES−3074 JAMES-3074 UidValidity strong ty…

cefe4a3

…ping

chibenwa approved these changes Feb 26, 2020

View reviewed changes

Arsnael commented Feb 26, 2020

View reviewed changes

mailbox/api/src/test/java/org/apache/james/mailbox/model/UidValidityTest.java Show resolved Hide resolved

remk reviewed Feb 26, 2020

View reviewed changes

mailbox/api/src/main/java/org/apache/james/mailbox/model/UidValidity.java Show resolved Hide resolved

trantienduchn approved these changes Feb 26, 2020

View reviewed changes

remk approved these changes Feb 26, 2020

View reviewed changes

chibenwa force-pushed the j3057-uid-validity-strong-typing branch from ca715a0 to f59df28 Compare February 27, 2020 04:57

Arsnael and others added 9 commits February 27, 2020 15:38

JAMES−3074 Create UidValidity type

330d5c9

JAMES-3074 Extends UidValidity strong typing in the James code base

ce1f6dc

JAMES-3057 MaildirFolder should not rely on an invalid UidValidity

4767fab

Using Optional is a better pattern for representing a not-yet-known value.

JAMES-3074 Tests should be using UidValidity::ofValid

6efc629

fixup! JAMES-3074 Extends UidValidity strong typing in the James code…

1bf137b

… base

JAMES-3074 Warn against the usage of UidValidity::of

6283934

fixup! JAMES-3074 Extends UidValidity strong typing in the James code…

d9c97b5

… base

fixup! JAMES-3074 Extends UidValidity strong typing in the James code…

9a38b2c

… base

chibenwa pushed a commit to chibenwa/james-project that referenced this pull request Feb 27, 2020

[NO_REVIEW≠ linagora#3132 JAMES−3074 JAMES-3074 UidValidity strong ty…

d91420d

…ping

chibenwa force-pushed the j3057-uid-validity-strong-typing branch from f59df28 to 9a38b2c Compare February 27, 2020 08:49

remk approved these changes Feb 27, 2020

View reviewed changes

mbaechler reviewed Feb 27, 2020

View reviewed changes

chibenwa added 2 commits February 28, 2020 09:30

fixup! JAMES-3074 Generate valid, random based UidValidity

19b8b84

JAMES-3074 s/UidValidity::random/UidValidity::generate/

928e04d

chibenwa pushed a commit to chibenwa/james-project that referenced this pull request Feb 28, 2020

[NO_REVIEW≠ linagora#3132 JAMES−3074 JAMES-3074 UidValidity strong ty…

e5ddd1a

…ping

mbaechler approved these changes Feb 28, 2020

View reviewed changes

chibenwa added the waiting_merge We are about to merge this! label Mar 2, 2020

chibenwa closed this Mar 2, 2020

JAMES-3074 UidValidity strong typing #3132

JAMES-3074 UidValidity strong typing #3132

Conversation

Arsnael commented Feb 20, 2020

mbaechler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arsnael commented Feb 21, 2020

Choose a reason for hiding this comment

mbaechler commented Feb 21, 2020

chibenwa commented Feb 21, 2020

Choose a reason for hiding this comment

Arsnael commented Feb 25, 2020

nvduc91 commented Feb 25, 2020

Arsnael commented Feb 25, 2020

trantienduchn commented Feb 25, 2020

remk commented Feb 25, 2020

chibenwa commented Feb 26, 2020

Arsnael commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Feb 27, 2020

chibenwa commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Mar 2, 2020