Repository navigation
Update appwrite push ledger name - #13815
Conversation
Appwrite is already in scope, so the qualifier was redundant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'pushLedger' => [ | ||
| '$collection' => ID::custom(DATABASE::METADATA), | ||
| '$id' => ID::custom('appwritePushLedger'), | ||
| '$id' => ID::custom('pushLedger'), |
There was a problem hiding this comment.
Existing projects store their QoS 1 messages in appwritePushLedger. This change makes collection setup create a separate, empty pushLedger collection without renaming the old collection or copying its records. After an upgrade, MQTT replay reads only the new collection and cleanup no longer prunes the old one, so previously stored offline messages cannot be recovered and the old records remain orphaned.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/config/collections/common.php
Line: 2161-2163
Comment:
**Ledger data is stranded**
Existing projects store their QoS 1 messages in `appwritePushLedger`. This change makes collection setup create a separate, empty `pushLedger` collection without renaming the old collection or copying its records. After an upgrade, MQTT replay reads only the new collection and cleanup no longer prunes the old one, so previously stored offline messages cannot be recovered and the old records remain orphaned.
**Knowledge Base Used:**
- [MQTT protocol gateway](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/mqtt-protocol-gateway.md)
- [Platform configuration and policies](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/platform-configuration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Its fine cause its not released
There was a problem hiding this comment.
That’s fair — if appwritePushLedger hasn’t appeared in a released version, there are no existing production projects or ledger records to migrate. In that case, renaming the collection directly is safe, and this finding doesn’t apply.
| private function cursorKey(Connection $connection): string | ||
| { | ||
| return 'appwrite:push:cursor:' . $connection->prefix | ||
| return 'push:cursor:' . $connection->prefix |
There was a problem hiding this comment.
A persistent client may have acknowledged through sequence C under appwrite:push:cursor:* while messages C+1 through T later accumulated. After this rename, the handler cannot find that cursor because it reads only push:cursor:*; a missing cursor defaults to the current tail T. Replay then starts after T, permanently skipping the client's unacknowledged backlog.
Knowledge Base Used: MQTT protocol gateway
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Mqtt/Handler.php
Line: 330
Comment:
**Cursor rename skips messages**
A persistent client may have acknowledged through sequence C under `appwrite:push:cursor:*` while messages C+1 through T later accumulated. After this rename, the handler cannot find that cursor because it reads only `push:cursor:*`; a missing cursor defaults to the current tail T. Replay then starts after T, permanently skipping the client's unacknowledged backlog.
**Knowledge Base Used:** [MQTT protocol gateway](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/mqtt-protocol-gateway.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Its fine cause its not released
There was a problem hiding this comment.
That’s fair. If the old appwrite:push:cursor:* key has never been released or used by persistent clients, there is no existing cursor state to preserve, so renaming it is safe. I’m withdrawing this finding.
| 'pushLedger' => [ | ||
| '$collection' => ID::custom(DATABASE::METADATA), | ||
| '$id' => ID::custom('appwritePushLedger'), | ||
| '$id' => ID::custom('pushLedger'), |
There was a problem hiding this comment.
The repository requires collection names to be lowercase plurals, but pushLedger is singular and camelCase. Use a conforming collection ID consistently across configuration and consumers. This explicit repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/config/collections/common.php
Line: 2161-2163
Comment:
**Collection name violates convention**
The repository requires collection names to be lowercase plurals, but `pushLedger` is singular and camelCase. Use a conforming collection ID consistently across configuration and consumers. This explicit repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/appwrite/appwrite/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| $this->database->createCollection('pushLedger', [], [], $any, false); | ||
| $this->database->createAttribute('pushLedger', 'topic', Database::VAR_STRING, 255, true); | ||
| $this->database->createAttribute('pushLedger', 'data', Database::VAR_STRING, 65535, true); | ||
| $this->database->createAttribute('pushLedger', 'messageId', Database::VAR_STRING, 255, false); | ||
| $this->database->createAttribute('pushLedger', 'messageInternalId', Database::VAR_STRING, 255, false); | ||
| $this->database->createAttribute('pushLedger', 'sequence', Database::VAR_INTEGER, 0, true); |
There was a problem hiding this comment.
This fixture mechanically copies the production collection name and schema, then queries that internal collection directly. Updating the test in lockstep with the implementation lets an arbitrary storage rename stay green while completely missing the observable upgrade failure for projects that retain the old ledger and cursor keys. The repository requires tests to cover observable behavior rather than mirror source or configuration, so this must be replaced with upgrade and reconnect replay coverage before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Knowledge Base Used: MQTT protocol gateway
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/Appwrite/Messaging/Adapter/Push/AppwriteTest.php
Line: 113-118
Comment:
**Test mirrors implementation details**
This fixture mechanically copies the production collection name and schema, then queries that internal collection directly. Updating the test in lockstep with the implementation lets an arbitrary storage rename stay green while completely missing the observable upgrade failure for projects that retain the old ledger and cursor keys. The repository requires tests to cover observable behavior rather than mirror source or configuration, so this must be replaced with upgrade and reconnect replay coverage before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
**Knowledge Base Used:** [MQTT protocol gateway](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/mqtt-protocol-gateway.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist