emersion
Support for rfc5465: The IMAP NOTIFY Extension
This patch series implements support for the NOTIFY extension.
The first commit implements support in imapclient, the main focus of my work. I've been using this on a client to monitor changes with success.
The second commit implements support in imapserver.
The third commit adds a minimal scaffold implementation in imapmemserver. This implementation simply rejects any request (which it technically allows as per the spec). It's not an actually useful implementation, and is mostly there so we can have minimal client-server unit tests. Implementing proper NOTIFY support for imapmemserver likely requires substantial changes which fall beyond the scope of this series.
This is likely easiest to review on a per-commit basis, and about half of the LoC are just unit tests for encoding/decoding.
v2: removed a very long example from the docs. It's very easy for this to fall out of sync and not really useful.
Tests with dovecot failed. When writing these tests I didn't realise that they also run with another server in CI.
I lean towards dropping the third commit— the one which implements the scaffold for NOTIFY in imapmemserver (along with the test which run server+client).
Thoughts?
We can also drop the incomplete imapmemserver support and enable the client-server NOTIFY tests only for dovecot. I see the useDovecot flag which should be useful for that.
I'll wait for feedback before advancing this this in any way.
v3: added test for how disconnections are handled (both with and without NOTIFY). Fix Overflow() channel hanging on disconnection.
I've re-written the client test to run only with dovecot, which has real NOTIFY support. Those test will be skipped with the imapmemserver, but are a lot more meaningful than before.
I've ripped out the scaffold for the imapmemserver's NOTIFY support. Such broken support was useless beyond these tests.
~~I've added another commit (which probably needs to be squashed later) which allows recognising whether a NOTIFY stream ended because of an explicit NOTIFICATIONOVERFLOW or because the connection closed.~~
I missed the review above. Using UnilateralDataHandler is much cleaner!
Wow, cool, support for 5465!
I find it difficult to assess the test coverage here, perhaps because of the verbosity. @emersion suggests tables, and I actually wrote the examples in the RFC together with unit tests that might be described as tables. That was C++, in Go I think I'd use an array of 5-tuples:
- a function literal to set up notifications
- a string with the expected/correct IMAP command
- a string with a mailbox to select
- a string that is sent by a mock server after the select has completed
- a function literal that asserts that go-imap acts correctly on the mock server string
Each example in section 5 corresponds to one of those tests, and that gave full coverage, easily verifiable. (Rereading now, I notice that the interaction between 5267 and 5465 isn't described in this way. Sigh. Sorry. I didn't have 5267 support myself and I suppose I didn't pay attention.)
Second, I like the use of unilateralDataHandler, but feel that it misses something. It can be nil, but handling unilateral data is almost mandatory now. (If gmail sends you an unsolicited BYE, that generally just means that a server there is overloaded and you should reconnect and be routed to another.) Nil seems to be a poor default, but I can't suggest a good way to write a general handler in Go… sorry about the lack of clarity here.
Tests were verbose, but I originally preferred to keep them that way since they're at least straightforward without too many clever abstractions.
TableDrivenTests is much shorter and even simpler to read, so migrated to that format.