Skip to content

Ensure subquery in findNewIds never returns NULL - #8590

Open
IchbinkeinReh wants to merge 1 commit into
nextcloud:mainfrom
IchbinkeinReh:fix-4479
Open

IchbinkeinReh wants to merge 1 commit into
nextcloud:mainfrom
IchbinkeinReh:fix-4479

Conversation

@IchbinkeinReh

@IchbinkeinReh IchbinkeinReh commented Jul 2, 2023 •

Copy link
Copy Markdown
Collaborator

Fixes #4479

@ChristophWurst ChristophWurst changed the title fix #4479 by ensuring subquery in findNewIds never returns NULL Ensure subquery in findNewIds never returns NULL Sep 13, 2023
@ChristophWurst

Copy link
Copy Markdown
Member

Change makes sense yet I'm unsure if it fixes #4479 entirely.

To verify all our supported databases understand the query we should add an integration test for \OCA\Mail\Db\MessageMapper::findNewIds


$found = $this->mapper->findNewIds($mailbox, [$id + 1]);

self::assertCount(1, $found);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fails without COALESCE 👍

@ChristophWurst

Copy link
Copy Markdown
Member

On a second look I'm skeptical about \OCA\Mail\Db\MessageMapper::findNewIds. The method is supposed to return all messages newer than the messages identified with the passed ids. Oracle doesn't like big IN clauses, so we chunk the ids array and run the query at least once. The semantics are that we'll get all messages newer than the oldest message of each chunk.

👀 @kesselb

@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
5 of 24 tasks
Comment thread lib/Db/MessageMapper.php Outdated
// MIN returns NULL if there are no rows selected, therefore we use COALESCE to ensure 0 is returned in this case
$subSelect
->select($sortOrder === IMailSearch::ORDER_NEWEST_FIRST
->select('COALESCE(' . ($sortOrder === IMailSearch::ORDER_NEWEST_FIRST

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be wrapped in createFunction

@kesselb

kesselb commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

@st3iny do you remember why we discarded #5629?

@st3iny

st3iny commented Oct 16, 2025

Copy link
Copy Markdown
Member

@st3iny do you remember why we discarded #5629?

IIRC, it was due to the performance concerns of resyncing all the envelopes.

The performance of this is obviously very bad because every message of the mailbox is recreated every time a sync is triggered. I added a small performance fix to mitigate this by just deleting the known uids (if provided) instead of clearing the whole mailbox cache.

@ChristophWurst
ChristophWurst requested a review from kesselb July 22, 2026 06:48
@ChristophWurst ChristophWurst self-assigned this Jul 22, 2026
@ChristophWurst
ChristophWurst force-pushed the fix-4479 branch 2 times, most recently from cd0fa24 to 081c181 Compare November 10, 2026 07:27
MIN/MAX of the known-ids subquery returned NULL when none of the
passed ids still existed in the mailbox (e.g. expunged messages),
which made every comparison against it false and caused mail to
disappear and reappear in a loop. COALESCE to a bound within
sent_at's 4-byte INTEGER range so the boundary stops excluding
rows in either sort direction, including messages with a spoofed
pre-epoch sent_at.

Fixes nextcloud#4479

Signed-off-by: Patrick Bender <patrick@bender-it-services.de>
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
Assisted-by: ClaudeCode:claude-sonnet-5

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MailboxDoesNotSupportModSequencesException problem with mails

4 participants