Repository navigation
fix(messaging): reach targetless users through Appwrite push when it isn't the default - #14586
Conversation
|
Review complete. No blocking issues — 1 nitpick below. 🧹 Nitpicks (1) — 🟢 1 low
This PR makes the messaging worker treat the Appwrite push provider as a regular deliverable provider: it is added to the
Reviewed commit: 5988a68 |
🟢 Tier S · Ready to merge
The worker resolves an enabled Appwrite push provider independently of the default and uses it to reach targetless users, while tracking providers actually used by send batches for message metadata. MQTT E2E coverage exercises targetless delivery with another provider as default and failure when Appwrite push is disabled. The latest test update removes message-search assertions and simplifies the FCM test helper. Latest changes: The new commits remove the provider-search assertions because the messageSearch filter makes them unobservable, and remove the now-unneeded custom FCM name parameter.
📂 Walkthrough · 2
Reviewed the commits since |
5988a68 to
e211d5a
Compare
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
3af37c0 to
3377959
Compare
…isn't the default A users[] push reached a user with no push target on users/<userId> only when the default (first enabled) push provider was Appwrite. With FCM or APNs set up first, the message failed with "No valid recipients found." even though an Appwrite provider was enabled. Resolve the enabled Appwrite push provider once per send and use it for the targetless fallback, whichever provider is the default. Users holding other push targets are unchanged, so no notification is sent twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3377959 to
bdbea83
Compare
There was a problem hiding this comment.
🟡 Tier B · 1 blocking finding to address. Summary
The worker appended every provider in its resolved-provider cache to the message's search text and the message.providers span. That cache is seeded with the default provider, and now the enabled Appwrite provider, so a send delivered entirely through another provider was still searchable by providers it never used. Record the provider of each batch result, wake-ups included, and build the search and span from that set. The cache keeps its seeding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3a821b0 to
cc6f082
Compare
The findings that requested changes are resolved. See the summary comment for what is still open.
…akes unobservable The messageSearch attribute filter recomputes a message's search text on every write from its id, description, status, title and type, so provider names are never stored and cannot be asserted through the search API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
59046f8
into
fix/mqtt-topic-send-by-topic-id

What does this PR do?
createPush({ users: [...] })reaches a user who has no push target through Appwrite push onusers/<userId>, so an app can receive direct messages without registering a target. Until now that no-target fallback instreamRecipientsran only when$defaultwas Appwrite.$defaultisfindOne(providers, enabled, type = push), in practice the first push provider created. A project that set up FCM or APNs before Appwrite push gotNo valid recipients found.for those users, even with an Appwrite provider enabled.The default provider doesn't send by itself. A user with no targets has no FCM or APNs token, so in that case nothing reached them at all.
Fix
checkAppwritePushEnabled()becomesgetAppwritePushProvider(). It returns the enabled Appwrite push provider (the default when that's Appwrite), resolved once per send and seeded into the provider cache.The project's default provider is left alone on purpose. Targets saved without a
providerIdfollow the default, and making Appwrite the default would reroute their FCM and APNs tokens to MQTT.message.providersonly lists providers the send usedThe worker built the
message.providersspan, and its own appendedsearchtext, from$providers, the resolved-provider cache. That cache is seeded with the default provider, and now with the enabled Appwrite provider. So a send delivered entirely through another provider still reported providers it never touched.sendBatch()now returns the provider it ran on. The worker collects those providers from page batches and wake-up batches, and builds the span from that set. The cache keeps its seeding, so there are no extra queries.The message
searchfield itself was never affected. ThemessageSearchattribute filter (app/init/database/filters.php) recomputes it on every write from the ID, description, status, title and type, so the worker's appended provider names are overwritten on save. That append loop is effectively dead code and is left as is here.Matrix: user sends with Appwrite + FCM enabled
users/Uusers/UUsers holding an FCM or APNs target but no Appwrite target are deliberately left alone. They already get the full notification, and adding MQTT would deliver it twice.
Trade-off
For each targetless user, a send:
users/<userId>topic row on first use;pushLedgerrow (kept for 7 days for replay);deliveredTotalandmessages.sent, whether or not a device is connected.This already happened when Appwrite was the default. It now applies whenever an Appwrite provider is enabled. We keep it on purpose: receiving on
users/<userId>with no target setup is a deliberate convenience.Test Plan
E2E in
tests/e2e/Services/Mqtt/MqttServerTest.php, each in a fresh project so FCM really is the first enabled push provider:testUserTopicReceivesPushWhenAnotherProviderIsDefault: FCM is created first, then Appwrite. A targetless user receives the message onusers/<userId>and the message endssent.testUserPushWithoutEnabledAppwriteProviderFails: FCM is enabled and Appwrite disabled. A targetless user's message endsfailed.The FCM provider uses placeholder credentials. The user holds no FCM token, so Google is never called.
The Mqtt CI lane mapping and the
rust-toolchainrepin come from #14580.Locally I ran
php -l, Pint and PHPStan on the changed files. PHPStan reports no new errors; the only ones are theUtopia\Database\Idnot-found errors thatmainalso shows because the local vendor is stale. E2E runs in CI.Related PRs and Issues
🤖 Generated with Claude Code