fix(dav): scope CalDAV UID lookups to calendar objects

[joshtrichards]

• Resolves: # (hard to say without digging thru many Issues; possibly many, perhaps only a few/none)

Summary

This is equally a correctness, performance, bug elimination, and robustness PR. But it's still tiny¹ and I think straightforward.

It tightens CalDavBackend::getCalendarObjectByUID(), mostly so that it only resolves objects from regular calendars, rather than subscription or federated-calendar objects. The method’s documentation is also updated, particularly surrounding the the ambiguity of unrestricted lookups when the same UID exists in multiple owned calendars. It's possible the JOIN change will be performance bump, but that depends on the sophistication of the in-use database query optimizers.

Specifics:

• Switch from LEFT JOIN to INNER JOIN because the WHERE is right-focused and the query filters on c.principaluri (so rows without a matching calendars (c) record are excluded anyway), so it makes no sense to LEFT JOIN unless I'm somehow mistaken? (Also looks like this is already done presumably safely elsewhere, such as in the function immediately after, getCalendarObjectById()).
• Only resolve calendar objects that are - well - actually calendar objects.
• Exclude objects in deleted calendars not just already deleted objects.
• Add a one-row limit because this method only returns a singular result and, technically, there is no uniqueness rule that guarantees UID uniqueness across all calendars - only within a collection. This still doesn't make it entirely deterministic: whichever matching row the database happens to yield first is returned but it's more correct (well, or throwing an exception if we get >1 result)... ²
• Document the relevance of the presence - or lack thereof - of the $calendarUri parameter as an optional collection-scope filter and it's impact on caller responsibility.
• Formatting (minor).

Notes:

• UID uniqueness is collection-scoped. Therefore, callers using an unrestricted lookup must treat the calendar portion of the returned path as authoritative; a matching object may belong to a different owned calendar than the one currently being processed.

Possible follow-up:

• I think getCalendarObjectById() might have at least one of the above Issues too; worth a follow-up look.
• BirthdayService and any other consumers of getCalendarObjectByUID() that use mode 2.
• Audit other CalDavBackend functions for similar issues.

TODO

• Check/improve/add test coverage
• Double-check function docblock accuracy and clarity

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)
• Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
• Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

• The content of this PR was partly or fully generated using AI

Footnotes

1. Well, excluding the docs I suppose! ↩

2. setMaxResults(1) only avoids retrieving unnecessary rows; it does not choose the correct calendar. The UID lookup itself has two valid modes: 1. Collection-scoped (caller supplies $calendarUri; deterministic); 2. Cross-calendar (this function supplies the CalendarUri, but the caller has to pay attention to it or else they'll perform the follow-up operation on the wrong calendar). Looks like consumers such as ImportService already use mode 1, but BirthdayService uses mode 2 so it may need to look to make sure it's not problematic (existing behavior; not created by this PR). ↩

[tcitworld]

It tightens CalDavBackend::getCalendarObjectByUID(), mostly so that it only resolves objects from regular calendars, rather than subscription or federated-calendar objects.

This prevents scheduling to work with subscriptions or federated calendars, not that we really support that, but it would prevent it.

[joshtrichards]

This prevents scheduling to work with subscriptions or federated calendars, not that we really support that, but it would prevent it.

Yeah, but that already doesn't work with the way this function, query, and return are structured today (unless I'm overlooking something).

And callers of this function today also assume the result will always be a regular calendar type.

I think we'd be better off with dedicated function(s) for those needs rather than overloading this one.

[tcitworld]

And callers of this function today also assume the result will always be a regular calendar type.

SabreDAV uses this method (it's part of the interface) and doesn't expect anything on this level, so I'm afraid there could be other future uses that SabreDAV might introduce, which could lead us into issues.