cyrusimap / cyrus-imapd

Cyrus IMAP is an email, contacts and calendar server
http://cyrusimap.org
Other
543 stars 149 forks source link

JMAP: email/queryChanges doesn’t remove Email when filtering for $flagged #2905

Closed iNPUTmice closed 4 years ago

iNPUTmice commented 5 years ago

Steps to reproduce

Expected result

Actual result

Additional information

Edit: Getting logs from my email client is currently WIP and the queries/scenarios are getting too complex to simply reproduce them with curl. Let me know if you need JSON logs and I can probably work on logging next.

rsto commented 4 years ago

Sadly, Cyrus can't return query changes for this filter, because the filter is using mutable Email properties. Note that the Email/query response correctly states canCalculateChanges: false. An Email/query can still return a non-null queryState, even if this state cannot be used for queryChanges.

What the Cyrus implementation did wrong was to take the provided queryState without checking if it can indeed correctly calculate changes for this filter. I have fixed this in the commit that closes this issue.

Thanks for reporting this and for the detailed issue report.

iNPUTmice commented 4 years ago

Thank you for fixing this.

Sadly, Cyrus can't return query changes for this filter, because the filter is using mutable Email properties.

I see. Would it be in the realm of possibility to do that one at one point? Displaying flagged messages is a first class citizen of my email app; and while I’ve now implemented proper support for queries that cannot calculate changes, it is a lot of cache invalidation that I have to do. I find this similarly problematic for queries that have text and inMailboxesOtherThan set, of which i also have a lot.

rsto commented 4 years ago

I don't see it happen in the near future for keywords, sorry. text queries on the other hand should be OK for queryChanges, as the underlying data is immutable. I also thought we return canCalculateChanges==true for the mailbox filters?

iNPUTmice commented 4 years ago

I also thought we return canCalculateChanges==true for the mailbox filters?

That works fine for inMailbox; but for inMailboxOtherThan it doesn’t. (I’m using that to filter out deleted and spam emails like the spec suggests).

A sample response looks like this:

 [
            "Email/query",
            {
                "filter": {
                    "inMailboxOtherThan": [
                        "d3173423-9c73-4757-bf96-cb8c24d0b8f9"
                    ],
                    "text": "xmpp"
                },
                "queryState": "5618:0",
                "canCalculateChanges": false,
                "position": 0,
                "total": 118,
                "ids": [
                    "M43c9c66c0e6a595817b590cf",
                    "Mef9107d59ac5145c0f21deaf",
                    "Mad42a0f2c052ddba7a8975bd",
                    "Mc4b2d32877fc445191905993",
                    "M21d9175095d7f4b64ec79ea1",
                    "Mbad404ef026c6adc79350fe4",
                    "M963bc532f7a8082dac2717b7",
                    "M27c181cf4ddaa11a9bcb6411",
                    "Mb9a6b215080d5de23386ab2c",
                    "Mc46c88aab614fb26a2da20da",
                    "M2dc0159d24bc5864c75e58b3",
                    "M5c1ab502485b984d07f7f68a",
                    "Mc34c4bf734eac5d84d675e2d",
                    "Mab066e3b0ed716b3ad8ae106",
                    "M0df670624c7f8f571d85a6bf",
                    "M9cc555c55ef31e5c18d695dd",
                    "M98300e6ee16d6d9cdd855ce8",
                    "M1be00ec592abc8bb6d02fa44",
                    "Ma9e54e32fbec08471489eb12",
                    "Mc4e42ee1495af0bb2a4ca096"
                ],
                "collapseThreads": true,
                "accountId": "beta@ltt.rs"
            },
            "9"
]
rsto commented 4 years ago

Thanks for checking that out. Yeah, inMailbox is a special filter, because we can deduce the changes from the expunged flag on the message. That won't be all that simple for inMailboxOtherThan. As it stands, I currently don't see much we can do in the short term.

If the backend is a Cyrus server, then you can pass the https://cyrusimap.org/ns/jmap/performance capability in the request. This will tell you both if the query response was cached in the isCached field, and more importantly the filters field will contain a list of data layers that were inspected to generate the result.

The search costs currently reported are: index, conversations, annotations, cache, xapian and body (see https://github.com/cyrusimap/cyrus-imapd/blob/master/imap/jmap_mail.c#L1509). Generally, index, conversations and cache should be very fast, annotations probably a bit slower. Avoid queries that use xapian and body in your clients hot-paths, if you can.