Skip to content

edit message db#3343

Open
mchenani wants to merge 2 commits intomc/edit-message-content-typefrom
mc/edit-message-db
Open

edit message db#3343
mchenani wants to merge 2 commits intomc/edit-message-content-typefrom
mc/edit-message-db

Conversation

@mchenani
Copy link
Copy Markdown
Contributor

@mchenani mchenani commented Mar 17, 2026

TL;DR

Adds database support for message editing functionality with a new message_edits table and associated query operations.

What changed?

  • Added database migration to create message_edits table with fields for tracking edit history including edit ID, group ID, original message ID, editor inbox ID, edited content, and timestamp
  • Created StoredMessageEdit struct and QueryMessageEdit trait with methods for retrieving edits by message ID, getting latest edits, and checking if messages have been edited
  • Added EditMessage content type (17) to the ContentType enum
  • Implemented Editable trait to determine which message types can be edited (only text, markdown, reply, attachment, remote attachment, transaction reference, multi remote attachment, and wallet send calls are editable)
  • Added comprehensive test coverage for all edit query operations including multi-message queries and latest edit retrieval

How to test?

Run the existing test suite which includes tests for:

  • Storing and retrieving edit records
  • Checking if messages have been edited
  • Getting the latest edit when multiple edits exist
  • Querying edits for multiple messages at once
  • Retrieving edits by group
  • Getting latest edits for a batch of messages

Why make this change?

This change provides the foundational database layer needed to support message editing in XMTP conversations. The design handles out-of-order message delivery by allowing edits to arrive before original messages, and efficiently tracks edit history with proper indexing for performance.

Note

Add message_edits database table and query API for edit message support

  • Adds a new message_edits table via a Diesel migration with fields for id, group_id, original_message_id, edited_by_inbox_id, edited_content, and edited_at_ns, plus indexes on (original_message_id, edited_at_ns DESC) and group_id.
  • Introduces StoredMessageEdit struct and QueryMessageEdit trait with methods to fetch edits by id, original message, latest edit, batch latest edits, group edits, and existence checks.
  • Adds EditMessage = 17 variant to the ContentType enum, wired into Display, From<String>, and Diesel FromSql conversions; is_deletable returns false and is_editable returns false for this type.
  • Introduces a new Editable trait (alongside existing Deletable) and adds it as a required bound on DbQuery, so all DB handle types must implement QueryMessageEdit.

Macroscope summarized 2be66f4.

Copy link
Copy Markdown
Contributor Author

mchenani commented Mar 17, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Mar 17, 2026

Claude finished @mchenani's task —— View job


Code Review Complete

Review Date: March 18, 2026, 06:11 UTC

Critical Issue

SQL Injection Vulnerability in get_latest_edits_for_messages (crates/xmtp_db/src/encrypted_store/message_edit.rs:200-208)

The dynamic SQL query construction is vulnerable to SQL injection:

let placeholders = message_ids
    .iter()
    .map(|_| "?")
    .collect::<Vec<_>>()
    .join(", ");

let query = format!(
    "SELECT id, group_id, original_message_id, edited_by_inbox_id, edited_content, edited_at_ns
     FROM (
         SELECT *, ROW_NUMBER() OVER (PARTITION BY original_message_id ORDER BY edited_at_ns DESC) as rn
         FROM message_edits
         WHERE original_message_id IN ({})
     ) WHERE rn = 1",
    placeholders
);

While the placeholders themselves are safe (?), this approach bypasses Diesel's compile-time SQL safety guarantees. If the length of message_ids could ever be influenced by untrusted input or grow unbounded, this creates a DoS vector. More critically, if this pattern is copied elsewhere with actual string interpolation instead of ? placeholders, it would create an injection vulnerability.

Recommendation: Use Diesel's query builder with raw SQL only for the window function part, not the entire query. Consider using a CTE or subquery that Diesel can validate at compile time.

Performance Concern

Missing index on (original_message_id, edited_at_ns DESC) may not be optimal for batch queries (migration up.sql:29)

The composite index idx_message_edits_original_timestamp is perfect for single-message queries, but the window function query in get_latest_edits_for_messages may not utilize it efficiently when querying many messages at once. Consider adding a covering index or monitoring query performance in production.

Minor Issues

  1. Inconsistent ordering in get_edits_by_original_message_id (message_edit.rs:153)

    • Orders by edited_at_ns.asc() while get_latest_edit_by_original_message_id uses .desc()
    • If chronological order is intended, this is fine, but document why different from the "latest" variant
  2. No bounds check on message_ids length (message_edit.rs:187-221)

    • get_latest_edits_for_messages could generate a very long SQL query if message_ids is large
    • Consider batching or adding a maximum length check
  3. Test coverage gap (noted by Codecov)

    • group_message.rs shows only 7.14% coverage for the new Editable trait implementation
    • The trait logic is straightforward, but automated tests would prevent regression

Positive Observations

  • Well-designed schema with proper foreign key constraints
  • Thoughtful handling of out-of-order message delivery (no FK on original_message_id)
  • Comprehensive test suite for core functionality
  • Clean separation of concerns with trait-based query API
  • Proper use of indexes for common query patterns

@mchenani mchenani marked this pull request as ready for review March 17, 2026 22:28
@mchenani mchenani requested a review from a team as a code owner March 17, 2026 22:28
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Mar 17, 2026

Approvability

Verdict: Approved

Adds database schema and query infrastructure for message editing functionality. All changes are within the author's designated code ownership domain, the additions are well-tested with comprehensive unit tests, and the changes don't modify existing runtime behavior - they add new tables and query methods that will be consumed by future work.

No code changes detected at 2be66f4. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 87.05036% with 54 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (mc/edit-message-content-type@9d62fd1). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/xmtp_db/src/encrypted_store/message_edit.rs 89.80% 41 Missing ⚠️
...rates/xmtp_db/src/encrypted_store/group_message.rs 7.14% 13 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##             mc/edit-message-content-type    #3343   +/-   ##
===============================================================
  Coverage                                ?   82.86%           
===============================================================
  Files                                   ?      378           
  Lines                                   ?    51365           
  Branches                                ?        0           
===============================================================
  Hits                                    ?    42566           
  Misses                                  ?     8799           
  Partials                                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant