Skip to content

Improving the representation of RFC8621 groups #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sftse
Copy link
Contributor

@sftse sftse commented May 28, 2025

The way mail-parser currently represents mailboxes and groups is a bit verbose to deal with, and makes subsequent handling more complex and error prone. Because of the arguably awkward way RFC8621 decides to represent single mailboxes #98 as a group by section 4.1.2.4 GroupedAddresses, anybody that does need the fidelity of the addresses parsed as in RFC5322 has to decompose them again.

This PR does not advocate changing the representation to RFC5322, instead suggesting more modestly that the enum be abandoned. Because of the awkwardness of RFC8621 GroupedAddresses users that do match on the enum already must be aware that single mailboxes may have different representations, and have to make sure to handle mailboxes the same whether they are in Address::List or Address::Group. As an example, we needed the RFC5322 representation for which we used this code

#[derive(Clone, Debug, PartialEq, Eq)]
enum Address<'x> {
    Mailbox(crate::Addr<'x>),
    Group(crate::Group<'x>),
}
fn into_addresses(src: crate::Address<'_>) -> Vec<Address<'_>> {
    let mut addresses = Vec::new();
    match src {
        crate::Address::List(addrss) => {
            for addr in addrss {
                addresses.push(Address::Mailbox(addr));
            }
        }
        crate::Address::Group(groups) => {
            for grp in groups
            {
                if grp.name.is_some() {
                    addresses.push(Address::Group(grp));
                } else {
                    for addr in grp.addresses {
                        addresses.push(Address::Mailbox(addr));
                    }
                }
            }
        }
    }
    addresses
}

This exemplifies how the enum is more of a hinderance than a benefit as an API, but the common Address::List case does save one allocation over Address::Group. This PR removes the enum, focusing on the way this simplifies the API, but we can salvage the allocation if necessary.

For reference, this is what the above code would look like after this PR.

fn into_addresses(addr: crate::Address<'_>) -> Vec<Address<'_>> {
    let mut addresses = Vec::new();
    for grp in addr.groups {
        if grp.name.is_some() {
            addresses.push(Address::Group(grp));
        } else {
            for addr in grp.addresses {
                addresses.push(Address::Mailbox(addr));
            }
        }
    }
    addresses
}

@sftse sftse changed the title Improving group representation as in RFC8621 Improving the representation of RFC8621 groups May 28, 2025
sftse added 3 commits May 28, 2025 14:17
The enum separating the Vec<Addr> from the Vec<Group> case carries a large
API burden with it.

The Address::List variant introduces code duplication to any consumer which has to write
code to handle both Address::List and Address::Group, even though much of this code
will be the same due to the way RFC8621 4.1.2.4 GroupedAddresses deals with single mailboxes.

By reducing the enum variants, this duplication is no longer necessary.
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