Skip to content

bug(db): get_group_messages_with_reactions drops 7 content types when no content_types filter is passed #3969

Description

@Dusk1e

Summary

get_group_messages_with_reactions (crates/xmtp_db/src/encrypted_store/group_message.rs)
strips reactions out of the main message query so they aren't returned twice — they're
folded into each message's reactions field instead.

When the caller passes content_types, it does that correctly: take the caller's list,
remove Reaction.

Some(content_types) => {
    let mut content_types = content_types.clone();
    content_types.retain(|content_type| *content_type != ContentType::Reaction);
    Some(content_types)
}

When the caller passes no filter, it substitutes a hardcoded allowlist instead:

None => Some(vec![
    ContentType::Text,
    ContentType::GroupMembershipChange,
    ContentType::GroupUpdated,
    ContentType::ReadReceipt,
    ContentType::Reply,
    ContentType::Attachment,
    ContentType::RemoteAttachment,
    ContentType::TransactionReference,
    ContentType::Unknown,
]),

That list has 9 entries. ContentType has 17 variants. Excluding Reaction, the
default query should match 16; it matches 9.

Impact

Messages with these content types are silently missing from the result — no error,
no log line:

variant discriminant
WalletSendCalls 10
LeaveRequest 11
Markdown 12
Actions 13
Intent 14
MultiRemoteAttachment 15
DeleteMessage 16

This is reachable from the public FFI surface:

FfiConversation::find_messages_with_reactions (bindings/mobile/src/mls.rs:2818)
MlsGroup::find_messages_with_reactions (crates/xmtp_mls/src/groups/mod.rs:1805)
get_group_messages_with_reactions

FfiListMessagesOptions::content_types is Option<Vec<FfiContentType>> and defaults
to None, so any iOS/Android caller that doesn't set an explicit filter takes the
hardcoded branch. Those apps never see markdown messages, inline action messages,
multi-remote attachments, or wallet-send-calls transaction requests in a conversation
listed through this API.

The sibling get_group_messages path is unaffected — it passes content_types
straight through to eq_any only when Some, so find_messages returns all of these
types normally. Only the with-reactions variant drops them, which makes the symptom
look like a client rendering bug rather than a query bug.

The list appears to have been correct when written and then gone stale: every content
type added after it was introduced is absent from it. ContentType::all() already
exists in the same file (line 238) and is currently unused.

Suggested fix

Derive the default from ContentType::all() and apply the same retain both branches
already want, so the rule is stated once and new variants are picked up automatically:

let mut content_types = modified_args
    .content_types
    .clone()
    .unwrap_or_else(ContentType::all);
content_types.retain(|content_type| *content_type != ContentType::Reaction);
modified_args.content_types = Some(content_types);

Callers passing an explicit list are unaffected (same retain as before), including
the Some(vec![Reaction]) edge case, which still yields an empty list.

Failing test

Stores one message of every ContentType and asserts the default query returns all
non-reaction types. Fails on main, passes with the change above.

#[xmtp_common::test]
fn test_messages_with_reactions_returns_all_non_reaction_content_types() {
    with_connection(|conn| {
        let group = generate_group(None);
        group.store(conn).unwrap();

        let all_types = ContentType::all();
        for (i, content_type) in all_types.iter().enumerate() {
            generate_message(
                None,
                Some(&group.id),
                Some(1_000 + i as i64),
                Some(*content_type),
                None,
                None,
            )
            .store(conn)
            .unwrap();
        }

        let returned = conn
            .get_group_messages_with_reactions(&group.id, &MsgQueryArgs::default())
            .unwrap();

        let returned_types: Vec<ContentType> =
            returned.iter().map(|m| m.message.content_type).collect();

        for content_type in all_types.iter().filter(|t| **t != ContentType::Reaction) {
            assert!(
                returned_types.contains(content_type),
                "{content_type:?} message was dropped from get_group_messages_with_reactions"
            );
        }
        assert_eq!(returned.len(), all_types.len() - 1);
    })
}

Verified against cargo test -p xmtp_db --lib: the test fails on main with

WalletSendCalls message was dropped from get_group_messages_with_reactions
test result: FAILED. 0 passed; 1 failed

and with the change above the full crate suite is green (217 passed; 0 failed; 8 ignored).

Opening a PR with the fix and this test.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions