fix(proposal): avoid UID collision when converting a proposal without…

[shbb-asdea]

… a known blocker

convertProposal assigned the proposal's UUID to the new VEVENT before deciding between "PUT over an existing blocker" and "INSERT a new calendar object". When findCalendarBlocker missed the blocker (calendar not enumerated by the principal lookup, search backend lag, etc.) the INSERT collided with oc_calendarobjects.calobjects_by_uid_index because the orphan blocker still held the same UID, surfacing as a 500 with SQLSTATE[23505] and leaving the proposal unconverted.

Defer the UID assignment until the blocker lookup result is known: when a blocker is found, keep reusing the proposal UUID so the in-place PUT preserves iCal threading; when no blocker is found, allocate a fresh UUID so the INSERT cannot clash with a stale row.

Refs: #7941

[SebastianKrupinski]

Hi @shbb-asdea

Please link a proper issue ticket, the reference link currently, links to a dependency update PR

[shbb-asdea]

@SebastianKrupinski apologies for the confusion — I had pasted the wrong issue number. The PR description is now updated to reference the correct issue (#7941). Could you take another look?

[SebastianKrupinski]

The issue you linked should have been resolved by this PR #8092

The issue was the original code would delete the existing event, but due to our trash bin logic the event was not actually deleted, so when the original code created a new event there was a collision in the index.

But the above PR resolves this, by updating the blocker, is this still an issue?

[shbb-asdea]

Hi @SebastianKrupinski, thanks for pointing to #8092 — I dug into its diff and the relationship with this issue is clearer now.

#8092 (which closed #8049 — same symptom: duplicate key on calobjects_by_uid_index during proposal convert) changed the convert flow from "delete-blocker + create-event" to "find-blocker + update-in-place via PUT". That removed the collision on the path where the blocker is found — PUT keeps the URI and just rewrites the row, so the unique index is never re-touched.

But the same PR introduced an else branch (ProposalService.php#L369-L377 on main):

$result = $this->findCalendarBlocker($user, $proposal);
if ($result !== null) {
    $this->applyCalendarBlockersOrganizer(...);   // PUT — safe after #8092
} else {
    $userCalendar->createFromString(...);          // INSERT — still collides
}

When findCalendarBlocker (L796-L805) returns null but a blocker row still exists in oc_calendarobjects, the else branch INSERTs with the proposal's UID and crashes on calobjects_by_uid_index exactly the way #8049 / #7941 described.

findCalendarBlocker is built on getCalendarsForPrincipal() + $calendar->search('', [], ['uid' => …]). The unique index is on (calendarid, calendartype, uid) — deleted_at is not part of it. So a blocker that has been trash-binned (or sits in a calendar/state the principal lookup or the search backend doesn't surface) is invisible to findCalendarBlocker, but still claims the UID in the table. That's why #8049 / #7941 keep reproducing after #8092: when the trash-bin row exists, we fall through to the else branch and the same collision happens.

This is consistent with the report by @sobeaa in this thread, where the crash still reproduces on 32.0.6 / Calendar 6.2.0 — i.e. with #8092 already merged.

The regression test added in this PR (testConvertProposalUsesFreshUuidWhenBlockerNotFound) reproduces the failure deterministically by mocking getCalendarsForPrincipal as empty (the precise "blocker invisible to the lookup" condition). It fails on current main and passes after the patch.

The fix is small: defer UID assignment until the lookup result is known.

Blocker found → reuse proposal UUID (PUT in place — unchanged behaviour from #8092, iCal threading preserved).
Blocker not found → fresh UUID for the INSERT, which therefore can't collide with a stale or trash-binned row.
Happy to switch to an alternative if you'd prefer — e.g. deleting the stale row through the DAV layer before INSERT, or extending findCalendarBlocker to also surface trash-binned objects — but a fresh UUID seemed the smallest, most defensive change and preserves the happy-path behaviour #8092 set up.