Mail details view broken

The mail details view appears to be broken when under certain conditions. The conditions I’ve found so far are:

  • Running a query with the actors set to all users (e.g. no filter) (route: /mail/query/{ID}).
  • Selecting an outbound email going to another user which is marked as Sent (route: /mail/detail/{ID}/).

In both cases, the following error is thrown:

Unhandled Exception ("Exception")
	
Rows passed to "loadAllFromArray(...)" include two or more rows with the same ID ("5071"). Rows must have unique IDs. An underlying query may be missing a GROUP BY.

I can’t currently provide further reproduction or analysis, but will look into it the week after next to see if I can narrow it down.

Version

Library Version Date Branchpoint
phabricator bc4f86d27996 Mon, Oct 19
arcanist 4b3baca999a4 Fri, Oct 16

I’m not immediately able to reproduce this, although I suspect it’s a real bug and have a guess about why it’s happening, and have a couple more ideas I’ll try.

Is metamta.one-mail-per-recipient set to false on your install?

After some fiddling, I was successfully able to reproduce this. I believe the full reproduction steps are:

  • Disable the metamta.one-mail-per-recipient configuration setting.
  • Generate a mail message to two or more users.
  • View the message details in /mail/ (or some queries can hit it, too).

This was caused by an existing bug in query construction which was exposed by an earlier correctness change intended to identify bugs like this, in https://secure.phabricator.com/D21400.

I think this is fixed by https://secure.phabricator.com/D21486, which adds the missing GROUP BY. This change also simplifies the query logic somewhat. It should improve the performance of the “Outbox” query, but may cause the “All” query (i.e., with no filters) to overheat where it could previously find results.

I’m not aware of any real use case for the “All” query, so I suspect this is fine in practice: you can’t see mail you didn’t send and didn’t receive, so this query is just the union of “Inbox + Outbox”. If you have some real use case for this, let me know.

1 Like

(And: thanks for the report!)

Awesome, thanks for the update. I’ll check the metamta.one-mail-per-recipient setting when I’m back in the office tomorrow (and will try your patch too), but I don’t believe we’ve changed any settings relating to that kind of thing (except to configure our SMTP host and auth details etc.).

Thanks for the prompt response and fix!

Just pulled in the latest changes from master and I can confirm this is now fixed. Thanks again @epriestley!

Great, thanks for confirming. Let me know if you run into anything else.