Repository navigation
feat(mail): rebuild email suggestions around who you write to - #1063
Conversation
Suggestions now come from the cheapest source that has any: the site directory (enabled Suite users and JMAP accounts) merged with the account's address index, then JMAP contacts, then JMAP mail. A query that finds nothing anywhere is remembered for a minute. The address index stores when the account last sent mail to an address and whether it is a saved contact, instead of a correspondence count. Both merge in a way that is safe to repeat, so re-indexing changes nothing and a rebuild upserts instead of starting over. Received mail never lifts an address, so newsletters and no-reply senders stay down. Ranking moves to suite.mail.email_suggestions.ranking: name over domain match, complete word over partial, then people written to (most recent first), then colleagues and contacts, then everyone else. The unused search_email_addresses endpoint is removed. The index schema changed, so the rebuild patch runs again.
The directory of Suite users and JMAP accounts is cached for five minutes, so a new colleague could take that long to show up. Saving, deleting or renaming a User, or saving or deleting a JMAP Account, now clears the cache once the change commits. Clearing before the commit would let a request in between cache the old rows again. The TTL stays as a backstop for writes that skip doc events.
s-aga-r
changed the title
frappe/frappe#44376 moved MAX_PASSWORD_SIZE from frappe.auth to frappe.utils.password. The old import breaks test discovery on Frappe develop. Fall back to frappe.auth for Frappe versions before the move.
Frappe now keeps each failed-login count under its own cache key instead of a field of the login_failed_count hash. The tests read the hash directly, so the counts looked missing. Read them through LoginAttemptTracker.login_failed_count, which works with both layouts.
![frappe-guard[bot]](https://avatars.githubusercontent.com/in/5408516?s=60&v=4){kind=small}
| # Saves clear the cache; the TTL only backs up writes that skip doc events. | ||
| @redis_cache(ttl=5 * 60) | ||
| def get_directory_addresses() -> list[dict]: | ||
| """Every enabled Suite user and every JMAP account named by an address, as colleagues.""" | ||
|
|
||
| user = frappe.qb.DocType("User") | ||
| has_role = frappe.qb.DocType("Has Role") | ||
| users = ( | ||
| frappe.qb.from_(user) | ||
| .join(has_role) | ||
| .on(has_role.parent == user.name) | ||
| .select(user.full_name.as_("name"), user.name.as_("email")) | ||
| .where((user.enabled == 1) & (has_role.parenttype == "User") & (has_role.role == "Suite User")) | ||
| .distinct() | ||
| .run(as_dict=True) | ||
| ) | ||
| accounts = [{"name": None, "email": name} for name in frappe.get_all("JMAP Account", pluck="_name")] | ||
|
|
||
| return _deduplicate([{**address, "is_colleague": True} for address in users + accounts]) | ||
|
|
||
|
|
||
| def clear_directory_addresses(*args) -> None: | ||
| """Doc event: drop the cached directory once the User or JMAP Account change commits. | ||
|
|
||
| Not before: a request between the save and the commit would cache the old rows again. | ||
| """ | ||
|
|
||
| frappe.db.after_commit.add(get_directory_addresses.clear_cache) |
There was a problem hiding this comment.
🟡 P2 Directory cache is per-user, so invalidation misses other users
frappe.utils.caching.redis_cache scopes its key to frappe.session.user unless shared=True, so get_directory_addresses.clear_cache() in clear_directory_addresses only drops the saving user's entry. Every other user keeps the old directory (a removed user or account still suggested) until the 5-minute TTL expires, which contradicts the PR's "refresh the organization list when a User or a JMAP Account is saved, deleted or renamed". The directory is identical for all users, so it should be one shared key.
| # Saves clear the cache; the TTL only backs up writes that skip doc events. | |
| @redis_cache(ttl=5 * 60) | |
| def get_directory_addresses() -> list[dict]: | |
| """Every enabled Suite user and every JMAP account named by an address, as colleagues.""" | |
| user = frappe.qb.DocType("User") | |
| has_role = frappe.qb.DocType("Has Role") | |
| users = ( | |
| frappe.qb.from_(user) | |
| .join(has_role) | |
| .on(has_role.parent == user.name) | |
| .select(user.full_name.as_("name"), user.name.as_("email")) | |
| .where((user.enabled == 1) & (has_role.parenttype == "User") & (has_role.role == "Suite User")) | |
| .distinct() | |
| .run(as_dict=True) | |
| ) | |
| accounts = [{"name": None, "email": name} for name in frappe.get_all("JMAP Account", pluck="_name")] | |
| return _deduplicate([{**address, "is_colleague": True} for address in users + accounts]) | |
| def clear_directory_addresses(*args) -> None: | |
| """Doc event: drop the cached directory once the User or JMAP Account change commits. | |
| Not before: a request between the save and the commit would cache the old rows again. | |
| """ | |
| frappe.db.after_commit.add(get_directory_addresses.clear_cache) | |
| @redis_cache(ttl=5 * 60, shared=True) |
Prompt for AI agents
In suite/mail/email_suggestions/__init__.py at lines 118-145: Directory cache is per-user, so invalidation misses other users
`frappe.utils.caching.redis_cache` scopes its key to `frappe.session.user` unless `shared=True`, so `get_directory_addresses.clear_cache()` in `clear_directory_addresses` only drops the saving user's entry. Every other user keeps the old directory (a removed user or account still suggested) until the 5-minute TTL expires, which contradicts the PR's "refresh the organization list when a User or a JMAP Account is saved, deleted or renamed". The directory is identical for all users, so it should be one shared key.
Suggested replacement for those lines:
@redis_cache(ttl=5 * 60, shared=True)
Check the claim against the code first. Fix it only if it holds.
There was a problem hiding this comment.
This does not hold, so I did not change it.
redis_cache scopes a key to a user only when user=True or a user name is passed. With the default user=None, RedisWrapper.make_key gives one key per site (<db_name>|<key>), so all users share one cached directory. clear_cache() calls delete_keys(func_key), which deletes every key for the function. One clear after commit therefore refreshes the directory for every user.
The suggested shared=True is not safe here. make_key then returns the key without the site prefix ("shared across sites"), so on a bench with many sites, one site could serve its user list to another site.
There was a problem hiding this comment.
You're right — I checked the semantics and the finding doesn't hold. Withdrawing it.
Why the original claim was wrong
redis_cache(ttl=...)with the defaultuser=Nonedoes not scope by user.RedisWrapper.make_keyonly appends|{user}when a user is passed (user=Trueor a name); otherwise the key is{site}|{func_key}— one entry per site, shared by every user of that site.get_directory_addresses.clear_cache()deletes by function key, so it drops that single site-wide entry. Oneafter_commitclear inclear_directory_addresses(suite/mail/email_suggestions/__init__.py:152) therefore refreshes the directory for all users, which is what the PR promises.- The suggested
shared=Truewould have been a real bug:make_key(shared=True)returns the key with no site prefix, so on a multi-site bench one site's user list could be served to another. Your rejection of the suggestion is correct.
One optional nit (not a blocker)
The comment at suite/mail/email_suggestions/__init__.py:125 says "Saves clear the cache; the TTL only backs up writes that skip doc events" but doesn't say the cache is site-wide. Since the directory is identical for every user, a one-line note there ("site-wide: the directory is the same for every user") would stop this exact review comment from being raised again. Purely cosmetic — no code change needed.
No further action from me on this thread.
| client = get_account_client(self.account) | ||
| with client.batch() as b: | ||
| q = b.mail.email.query( | ||
| filter={"operator": "OR", "conditions": [{field: query} for field in ADDRESS_FIELDS]}, | ||
| sort=[{"property": "receivedAt", "isAscending": False}], | ||
| position=0, | ||
| limit=limit, | ||
| calculate_total=False, | ||
| ) | ||
| g = b.mail.email.get(ids=q.ref_ids(), properties=list(ADDRESS_FIELDS)) | ||
|
|
||
| addresses = [ | ||
| address | ||
| for email in (item.to_wire() for item in g.result.items) | ||
| for field in ADDRESS_FIELDS | ||
| for address in email.get(field) or [] | ||
| if matches_email_address(query, address) | ||
| ] | ||
| addresses = _deduplicate(addresses) | ||
| self.index_addresses(addresses) | ||
| return rank_email_addresses(query, addresses, limit) |
There was a problem hiding this comment.
🟡 P2 JMAP fallback reads results without checking handle errors
g.result.items is dereferenced unconditionally, so a refused Email/get (or a failed Email/query whose ref_ids() cannot resolve) raises AttributeError/MethodError out of get_suggestions instead of falling through to the miss cache. The sibling _get_emails in suite/mail/api/scheduled.py routes every handle through _refused_lookup/format_method_error for exactly this reason, and the deleted _email_address_suggestions at least only touched handle.result.ids.
Prompt for AI agents
In suite/mail/email_suggestions/__init__.py at lines 76-96: JMAP fallback reads results without checking handle errors
`g.result.items` is dereferenced unconditionally, so a refused `Email/get` (or a failed `Email/query` whose `ref_ids()` cannot resolve) raises `AttributeError`/`MethodError` out of `get_suggestions` instead of falling through to the miss cache. The sibling `_get_emails` in `suite/mail/api/scheduled.py` routes every handle through `_refused_lookup`/`format_method_error` for exactly this reason, and the deleted `_email_address_suggestions` at least only touched `handle.result.ids`.
Check the claim against the code first. Fix it only if it holds.
There was a problem hiding this comment.
Valid, fixed in 4275e44.
A refused lookup raises MethodError (not AttributeError) from .result. The contact lookup had the same gap through _query_contact_cards. get_suggestions now catches JMAP_REFUSALS around both remote lookups. It logs the refusal with format_method_error and returns no suggestions. A refusal is not stored as a miss, so the next keystroke asks the server again. Errors in the local lookup still raise.
Tests added: a refused mail lookup, a refused contact lookup (mail is not asked), and a refused lookup that is not remembered as a miss.
|
Guard · Confidence 3/5 (apex) · risk medium · 2 findings (2 P2) Affects 134 callers in 24 files. 1 caller may break. Summary
Confidence and Risk
Findings
Impactflowchart LR
n0["failed_logins"]:::changed
n1["test_lockout_uses_isolated_key_space"]:::changed
n2["test_api_key_wrong_secret_challenges_and_tracks"]:::changed
n3["get_email_suggestions"]:::changed
n4["frontend/src/apps/calendar/components/ParticipantSelector"]
n4 --> n3
n5["frontend/src/apps/mail/components/Controls/ContactCombobox"]
n5 --> n3
n6["frontend/src/apps/mail/components/Controls/RecipientInput"]
n6 --> n3
n7["useMailCommandPaletteSearch"]
n7 --> n3
n8["createContactSearch"]
n8 --> n3
n9["1 test of get_email_suggestions"]:::tests
n9 --> n3
n10["search_email_addresses"]:::changed
n10 --> n10
n3 --> n10
n11["1 test of search_email_addresses"]:::tests
n11 --> n10
n12["_email_address_suggestions"]:::changed
n3 --> n12
n13["_cache_contact_cards"]:::changed
n14["get_contact_cards"]
n14 --> n13
n15["_contact_addresses"]:::changed
n13 --> n15
n16["rebuild_email_address_index"]
n16 --> n15
n17["_cache_messages"]:::changed
n18["get_messages"]
n18 --> n17
n19["set_seen_status"]
n19 --> n17
n20["set_flagged_status"]
n20 --> n17
n21["4 tests of _cache_messages"]:::tests
n21 --> n17
n22["_remove_cached_messages"]:::changed
n23["_move_email_to_drafts"]
n23 --> n22
n24["delete_messages"]
n24 --> n22
n25["empty_mailbox"]
n25 --> n22
n26["move_messages_to_mailbox"]
n26 --> n22
n27["set_messages_mailboxes"]
n27 --> n22
n28["add_messages_to_mailbox"]
n28 --> n22
n29["remove_messages_from_mailbox"]
n29 --> n22
n30["set_spam_status"]
n30 --> n22
n31["_message_addresses"]:::changed
n17 --> n31
n16 --> n31
n32["EmailSuggestions"]:::changed
classDef changed fill:#fff3cd,stroke:#d39e00
classDef external fill:#e7f1ff,stroke:#0b5ed7
classDef tests fill:#f1f3f5,stroke:#adb5bd,color:#495057
Important Files Changed
Diagramflowchart TD
A[get_email_suggestions] --> B{recent miss?}
B -- yes --> Z[return empty]
B -- no --> C[get_local_suggestions]
C --> C1[address index hits]
C --> C2[get_directory_addresses cache]
C1 --> C3[deduplicate + rank]
C2 --> C3
C3 -- hits --> Z2[return ranked]
C3 -- none --> D[get_contact_suggestions]
D -- hits --> Z2
D -- none --> E[get_mail_suggestions]
E --> E1[JMAP Email/query + Email/get]
E1 --> E2[index_addresses]
E2 --> Z2
E -- none --> F[set miss key 60s]
F --> Z
Reviews (1) · Last reviewed commit: feat(mail): rebuild email suggestions around who the account writes to · Re-trigger · Fix all |
A refused JMAP contact or mail lookup raised out of get_email_suggestions, so typing in a recipient field showed an error. Log the refusal and return no suggestions instead. A refusal is not stored as a miss, so the next keystroke asks the server again. Errors in the local lookup still raise.
Summary
search_email_addressesendpoint. The app does not call it. It usesget_email_suggestions.MAX_PASSWORD_SIZEfromfrappe.authtofrappe.utils.password. The Drive WebDAV login now imports it from the new place, and falls back to the old place on older Frappe.develop. Frappe now stores each failed-login count under its own cache key, not in thelogin_failed_counthash. The tests now read the count throughLoginAttemptTracker.login_failed_count, which works with both layouts.How suggestions work
The app waits until you stop typing for 0.2 seconds. Then it looks for matches in this order:
support@yourcompany.com.If no step finds a match, the app remembers that exact search for one minute. If you type the same text again in that minute, it does not ask the mail server again.
How a match works
Each word that you type must match the start of a word in the name or the email address.
janjane dexamaneHow results are ordered
Examples
jsanodoeWhat the index stores
Each address in the index has:
last_sent_at: the last time the account sent mail to the address, or 0 if neveris_contact: true if the address is on a saved contactEach field changes in only one direction. A name stays until a new name arrives. The date only moves forward. The contact mark only turns on. So it is safe to index the same mail again, and it changes nothing. This lets us remove the code that stopped the count from growing on each sync. "Rebuild search index" now adds to the index. It does not delete the index first.
The app knows that you sent a message when the sender is one of the account's own addresses (its identities). Drafts do not count.
Before you merge
#4). Until the rebuild jobs finish, suggestions come from the organization list and the mail server only.Test plan
suite.mail.tests.test_email_suggestions: the order of the sources, merging, ranking, the mail fallback, the one-minute miss cache, and the organization list refresh after commit.suite.mail.tests.test_email_address_index: runs against a real Tantivy index in a temporary folder. Indexing again changes nothing, the date only moves forward, the contact mark stays on, and invalid addresses are skipped.suite.mail.tests.test_message_caching: recipients of sent mail get a date. Received mail and drafts do not.suite.tests.test_search_storeandsuite.tests.test_data_store--app suiterun: it has 26 errors with this change and 24 without it. The two extra errors are Drive file tests that fail with a database deadlock in about half of the runs, with or without this change.test_mail_flags_and_search,test_mail_contacts). Stalwart is not set up on the test site. These are the only tests that send the real JMAP requests (the OREmail/querywith theEmail/getback-reference).Test coverage: Backend 69.8% (+0.4%) · Frontend 32.8% (+0%)
Delta is against the latest available
developcoverage. Coverage is informational. Workflow run for commit4275e44.