Message store events notification

[brong]

From: Sébastien Michel Bugzilla-Id: 3605 Version: 2.5.x (next) Owner: Sébastien Michel

[brong]

From: Sébastien Michel

We developed some time ago an out-band notification mechanism on mailbox events (Cyrus 2.3). This piece of software is based on the RFC 5423 - Internet Message Store Events - that defines a number of event types and event parameters. We modified Cyrus's processes (imapd, pop3d, lmtpd) and CLI source code to send events to notifyd daemon (subset of event types from RFC 5423)

find here : https://github.com/worldline-messaging/cyrus-imapd/compare/msgevent an update of our work that fully support event types defined in RFC 5423, ready for future 2.5.

You can follow discussions about this features on this threads : http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&searchterm=msgevent&msg=3006 http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&searchterm=msgevent&msg=3073

http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&searchterm=msgevent&msg=3188

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

Re-assigning to Andreas.

[brong]

From: Andreas Osowski (Kolab Systems)

I have just finished the rebase against worldline-messaging/msgevent.

https://github.com/aosowski/cyrus-imapd/tree/ticket/3605

[brong]

From: Greg Banks

First tranche of review comments. More later.

commit "Allow time_to_iso8601 to print fraction of second"

• The new 2nd argument to time_to_iso8601 is called fracsec in one place and secfrac in another, which is needlessly inconsistent.

• It seems to be in milliseconds but the only way I can tell is by reading the code, neither the variable name nor the comment above the function explains this.

• You're using the new argument for two purposes: as a boolean to indicate that sub-second precision should be used, and at the same time as the actual number of milliseconds to print. If the calling code were to get a real timestamp whose milliseconds part just happens to be zero, the wrong thing will happen.

• Your buffer length checking is broken; you're using sprintf() not snprintf().

• Passing milliseconds around is rather unnatural with in a POSIX program. Perhaps a better way to do this would have been to add a new function

  int timeval_to_iso8601(const struct timeval , char buf, size_t len);

  and then you can just pass in what you already have in the one place that you need to, in imap/mboxevent.c.

• You've updated the unit test code so that it builds, but you've not added a unit test for the new feature.

• There are two commits with the same name, which is fine for the version control system but makes it hard for us discussing them.

commit "allow dot '.' in enum setting"

Looks good

commit "Added support for event notifications on message store based on RFC 5423"

This commit could be broken up into smaller chunks; big multi-purpose commits like this are really hard to review :( One good practice is to add a new API in the first commit (i.e. mboxevent.[ch] and Makefile changes) with no callers, and then gradually add new callsites for the API one by one each in their own commit.

• When you implement an RFC you should add it to the list in doc/specs.html.

file lib/imapoptions

• You add new options "event_notifspam" and "eventnotifier" which don't use underscores, and several other new options which do. Please be consistent! Personally I prefer underscores.

• I'm curious about why the option "event_timestamp_format" is needed. The RFC only talks about RFC3339 format (i.e. ISO8601) format. The RFC is a bit vaguely worded and I can't quite figure out whether using any other format is actually allowed. Later: oh I see, it's for the Javascript native time format. I can see why you'd want that, but perhaps it might be better to include it as a separate vendor parameter? Also, the RFC says that timestamps are expressed in local time (by which I assume they mean the notifying server's local time), but this code is generating timestamps in UTC.

file imap/mboxevent.h

(I read the final version of this file rather than try to track all the re-workings in it)

• It seems that there's a lot of re-work of the mboxevent.h code as we go through the commits :(

• You've used the identifiers from the RFC directly as enum entries, which is certainly easy but doesn't result in code that matches the Cyrus coding style. I see you went back later and fixed up enum event_param_type, which is wonderful, but the enum with the message types still has no enum tag, no prefix on entries and CamelCase.

• Why call your main structure event_state rather than event or even mboxevent? I notice too that some functions are called event_foo() and some mboxevent_foo() with no obvious reason why.

• Why does event_newstate() both return an event_state* and take an event_state ? This seems redundant. Later: oh, I see why. Maybe it would be a better idea to separate these two functions into something like event_new() and event_enqueue(). Or at the very least give the event_state parameter a name which makes it obvious that it's a pointer to the head of a LIST and not a pointer to a single object. In the context of Cyrus code this is very confusing - see mailbox_close() for example.

• mboxevent_add_sysflags() and mboxevent_add_usrflags() - these concepts are generally known as system_flags and user_flags in the code, i.e. spelled out.

• If mboxevent_extract_record() does what the comment says, the mailbox and index_record arguments should probably be const.

• Does the value returned from mboxevent_toURL() need to be freed by the caller?

file imap/mboxevent.c

(I read the final version of this file rather than try to track all the re-workings in it)

• I'm confused about enum event_param - why is it in alphabetical rather than numeric order, with explicit numbers? This is a maintenance headache. Also, the entries are camelCase which makes it hard to read in the code. For example, I later saw

  for (i = 0; i <= messageContent; i++) {

  and was wondering where the variable messageContent was from.

• In mboxevent_init(), you really should look at strarray_split(), as you've open-coded a near equivalent of it, twice.

• I hope that the "event_exclude_folders" option is not the only way to exclude folders. It seems a bit clunky and hard to scale. How about an annotation on folders? Both sieve and expire use that technique.

• If event_parameter.name is being used to point to static unallocated strings it might be best to make it a const char. In fact, given that each event has a fixed maximum set of parameters, it might be easier to separate that out into a const array of const chars.

• event_newstate() takes a parameter "int type", which should really use the enum.

• mboxevent_free() frees an entire list of events. This is not a good idea - there are likely to be places in the code where you need to free just the one.

• In mboxevent_free() you're reaching into a struct imapurl to free it's parts. This is not a good idea - all the code that deals with struct iumapurl internals should be in imapurl.c. If you need to allocate and free imapurl objects, you should add functions to imapurl.c to do that (and add unit tests too please).

• In mboxevent_notify(), again it would be nice to use the variable name to make clear that the argument is a list and not a single object.

• In mboxevent_notify() there is some special case code which may swap around the first two events in the list. The list manipulation logic assumes without checking that there were no third or subsequent elements, and leaks them if there were.

• The way in which event.uidset is stored as a string seems silly when we have a perfectly good struct seqset for that.

• In mboxevent_add_sysflags(), mboxevent_add_usrflags(), and mboxevent_add_flag(), you need to look at strarray_add_case(). Also, in mboxevent_add_sysflags() you check case-sensitively for \deleted but \Deleted, which is a bug.

• Given what mboxevent_extract_access() does, perhaps mboxevent_set_access() might be a better name.

[brong]

From: Sébastien Michel

> First tranche of review comments. More later. Thanks for your review, nice to have a critical look, the initial code is old (cyrus 2.1/2.3) and I tried to update it relative to 2.5, but I'm not yet totally familiar with this one.

> commit "Allow time_to_iso8601 to print fraction of second" > > - The new 2nd argument to time_to_iso8601 is called fracsec in one place > and secfrac in another, which is needlessly inconsistent. > > - It seems to be in milliseconds but the only way I can tell is by reading > the code, neither the variable name nor the comment above the function > explains this. > > - You're using the new argument for two purposes: as a boolean to indicate > that sub-second precision should be used, and at the same time as the > actual number of milliseconds to print. If the calling code were to > get a real timestamp whose milliseconds part just happens to be zero, > the wrong thing will happen. > > - Your buffer length checking is broken; you're using sprintf() not snprintf(). > > - Passing milliseconds around is rather unnatural with in a POSIX program. > Perhaps a better way to do this would have been to add a new function > > int timeval_to_iso8601(const struct timeval , char buf, size_t len); > > and then you can just pass in what you already have in the one place > that you need to, in imap/mboxevent.c. > > - You've updated the unit test code so that it builds, but you've not added > a unit test for the new feature. > > - There are two commits with the same name, which is fine for the version > control system but makes it hard for us discussing them. OK

> commit "allow dot '.' in enum setting" > > Looks good I need to replace this patch with commit 6735484f76470b02c439cc553149b0beb0e34e81 from branch dev/sieve/vacation-seconds that is more general

> commit "Added support for event notifications on message store based on RFC > 5423" > > This commit could be broken up into smaller chunks; big multi-purpose > commits like this are really hard to review :( One good practice is to > add a new API in the first commit (i.e. mboxevent.[ch] and Makefile > changes) with no callers, and then gradually add new callsites for the > API one by one each in their own commit.

It's still possible thanks to git. I see what can I do

> - When you implement an RFC you should add it to the list in doc/specs.html. You're rigtht... but what a strange RFC that doesn't define any protocol nor message format !

> file lib/imapoptions > > - You add new options "event_notifspam" and "eventnotifier" which don't > use underscores, and several other new options which do. Please be > consistent! Personally I prefer underscores. > > - I'm curious about why the option "event_timestamp_format" is needed. > The RFC only talks about RFC3339 format (i.e. ISO8601) format. The > RFC is a bit vaguely worded and I can't quite figure out whether using > any other format is actually allowed. > Later: oh I see, it's for the Javascript native time format. I can see > why you'd want that, but perhaps it might be better to include it as > a separate vendor parameter?

It is backward compatible to our old implementation. I should remove these and keep some patch in our internal git.

> Also, the RFC says that timestamps are expressed in local time (by > which I assume they mean the notifying server's local time), but this > code is generating timestamps in UTC. > > file imap/mboxevent.h [...] > - Why does event_newstate() both return an event_state* and take an > event_state ? This seems redundant. > Later: oh, I see why. Maybe it would be a better idea to separate > these two functions into something like event_new() and event_enqueue(). > Or at the very least give the event_state parameter a name which > makes it obvious that it's a pointer to the head of a LIST and not > a pointer to a single object. In the context of Cyrus code this is > very confusing - see mailbox_close() for example. Good ideas.

> - mboxevent_add_sysflags() and mboxevent_add_usrflags() - these concepts > are generally known as system_flags and user_flags in the code, i.e. > spelled out. > > - If mboxevent_extract_record() does what the comment says, the mailbox > and index_record arguments should probably be const. > > - Does the value returned from mboxevent_toURL() need to be freed by > the caller? > > file imap/mboxevent.c > > (I read the final version of this file rather than try to track > all the re-workings in it) > > - I'm confused about enum event_param - why is it in alphabetical > rather than numeric order, with explicit numbers? This is a > maintenance headache. [...]

I wanted to keep the alphabetical order as defined in RFC, but ensure the order of parameters when formatting. Also to optimize the parsing by having the most relevant firsts. I can decorrelate to clarify but at the cost of burdening the formatting. > for (i = 0; i <= messageContent; i++) { > > and was wondering where the variable messageContent was from. > > - In mboxevent_init(), you really should look at strarray_split(), as > you've open-coded a near equivalent of it, twice.

It doesn't seem to exist at the time of writing, nice to see it.

> - I hope that the "event_exclude_folders" option is not the only way > to exclude folders. It seems a bit clunky and hard to scale. How > about an annotation on folders? Both sieve and expire use that > technique.

It was also discussed to use SPECIAL-USE. Finally Bron preferred to not restrict inside Cyrus : http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&amp;msg=3190

I should remove this setting for a better way later

> ... > - In mboxevent_free() you're reaching into a struct imapurl to free > it's parts. This is not a good idea - all the code that deals > with struct iumapurl internals should be in imapurl.c. If you > need to allocate and free imapurl objects, you should add functions > to imapurl.c to do that (and add unit tests too please).

Our initial intention was to avoid to spread our patchs into cyrus source code in order to merge easily to new releases. It doesn't really make sense now.

[brong]

From: Greg Banks

(In reply to comment #4) > > First tranche of review comments. More later. > Thanks for your review, nice to have a critical look, the initial code is old > (cyrus 2.1/2.3) and I tried to update it relative to 2.5, but I'm not yet > totally familiar with this one.

No worries, that's an expected part of the merge process.

> > - When you implement an RFC you should add it to the list in doc/specs.html. > You're rigtht... but what a strange RFC that doesn't define any protocol nor > message format !

Indeed :(

> > - In mboxevent_init(), you really should look at strarray_split(), as > > you've open-coded a near equivalent of it, twice. > > It doesn't seem to exist at the time of writing, nice to see it.

Yeah, we try to make those utility functions as useful as possible.

> > - I hope that the "event_exclude_folders" option is not the only way > > to exclude folders. It seems a bit clunky and hard to scale. How > > about an annotation on folders? Both sieve and expire use that > > technique. > > It was also discussed to use SPECIAL-USE. Finally Bron preferred to not > restrict inside Cyrus : > http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&amp;msg=3190 > > I should remove this setting for a better way later

Ok, so Bron said

> Assuming that the data is available in the notification, and the cost of > sending events isn't too high, isn't it better just to run an external > filter that filters out the events you're not interested in?

but the RFC says

> For scalability reasons, some degree of filtering at event generation > is necessary. At the very least, the ability to turn on and off > groups of related events and to suppress inclusion of large > parameters (such as messageContent) is needed.

which implies that the RFC authors at least thought that it was best to do filtering at the event source, and having read the RFC I tend to agree.

But I still think specifying folder names in the config file is clunky, and would much prefer it if you could specify special-use items in the config file. Default value should be something like \Spam and \Trash. Although I'm not sure what the correct thing to do would be if the user COPYd a message between folders where one has events suppressed and the other doesn't.

> > > > ... > > - In mboxevent_free() you're reaching into a struct imapurl to free > > it's parts. This is not a good idea - all the code that deals > > with struct iumapurl internals should be in imapurl.c. If you > > need to allocate and free imapurl objects, you should add functions > > to imapurl.c to do that (and add unit tests too please). > > Our initial intention was to avoid to spread our patchs into cyrus source code > in order to merge easily to new releases. It doesn't really make sense now.

Sure.

[brong]

From: Greg Banks

Second tranche of reviews, more later.

file imap/mboxevent.c continued

• In mboxevent_extract_record(), the return value of mailbox_cache_get_msgid() is a new string which needs to be free()d. You're leaking it.

• In mboxevent_extract_record(), message-ids might be more conveniently stored as a strarray() and then built into a string on demand using strarray_join() in mboxevent_notify()

• In mboxevent_extract_record(), don't use strndup(), use xstrndup(). Likewise in mboxevent_extract_content().

• So the issue of messageContent is interesting. This sounds to me like it was easy to specify but not all that useful in this day of MIME messages, if the purpose of messageContent is to provide a human with a brief idea of what the message is about. At Fastmail we use a separate process which pulls apart MIME structures, does MIME decoding, strips HTML, and generates a short UTF-8 string intended as a user-visible preview of the message. This preview is stored in a per-message annotation. It would great if we could include that annotation in the mboxevent.

• In mboxevent_extract_quota(), you use a magical 1024 to report the diskUsed parameter. There's an array const int quota_units[QUOTA_NUMRESOURCES] designed for this.

• In mboxevent_extract_mailbox(), there's broken leading whitespace in the multi-line comment at the end of the function.

• mboxevent_toURL() should probably go into imap/mailbox.c but without capital letters in the function name.

• In json_escape_str(), hex_chars[] doesn't seem at all sensible. This

  sprintf(seq, "\u00%c%c", hex_chars[c >> 4], hex_chars[c & 0xf]);

  could be more sensibly implemented as

  sprintf(seq, "\u04x", (unsigned int)c);

• Also in json_escape_str(), there's a buf_printf() that would be useful than a sprint()/buf_append_cstr() pair. Also, there's really not a lot of point saving up ranges of unescaped characters and calling buf_appendmap() when you could just do a buf_putc() on every unescaped character as you encounter it. I think in a function like this, simplicity and obvious correctness is more important than a few CPU cycles.

• In json_escape_str(), why escape '/' ? RFC4627 says

> All Unicode characters may be placed within the > quotation marks except for the characters that must be escaped: > quotation mark, reverse solidus, and the control characters (U+0000 > through U+001F).

• Actually, come to that, why are you writing your own JSON generator when there's a bunch of perfectly good ones in C listed at json.org? Surely one of them will have a compatible licence and be well designed and tested.

commit "add strarray_size function to return the number of elements in an array"

Looks good. A unit test would be nice.

file imap/append.c

• In append_fromstage() I'm really not impressed by the trick you use of constructing a quota_t using an handcrafted initialiser. This is too sensitive to the internals of struct quota, which are being changed in other work that's going on. It would be far better if you could grab this same information as a side effect of the code in mailbox.c that already calculates it properly, and decorate an existing event_state chain with it. Also, you're setting the QUOTA_STORAGE resource but not QUOTA_MESSAGES.

[brong]

From: Greg Banks

Final trance of comments.

So, on further thought, there's some room for simplification in enum event_param_type. Having separate types for _STRING and _DYNSTRING is probably worth the complexity, given the number of strdups you're saving. But I think all the integral types could be collapsed into a single type which is a uint64_t. The only _INT parameters are diskQuota and maxMessages, and in both cases they have no valid value < 0 (they can be -1, but that means "no limit is enforced", and should be represented by omitting the parameter rather than emitting -1). The _QUOTAT parameters only report absolute quota values not deltas, so they don't need to be signed either. The _MODSEQT ones need to be unsigned.

In a similar vein, I don't think there's any need to go saving a copy of an broken-out struct imapurl anywhere. The only reason for doing that would be if you needed to update the URL conditionally or gradually as more information is discovered. You do currently do that, in mboxevent_notify() you add a UID to event->mailboxID. However, note that the RFC describes that parameter thus

> mailboxID > Included in events that affect mailboxes. A URI describing the > mailbox. In the case of MailboxRename, this refers to the new > name.

anb RFC2192 section 7 says that an imap: URL with UID= in it describes a message, not a mailbox. So in this case you're actually emitting the wrong form of URL. So if you only emit the correct form or URL, you don't need to keep a struct imapurl around. So you could just take the mailbox* when it's passed down in the API, use a temporary struct imapurl to build the string form of the URL, throw the imapurl away, and save the string as event->mailboxID.

• The logic in append_apply_flags() is wrong, it will report in the event the flags that the client attempted to set, rather than the flags that the client successfully set after ACL checking.

• At the end of append_copy() you do this

  event_state->oldmailboxid = mboxevent_toURL(mailbox);

  which could be moved into mboxevent_extract_copied_record().

• In mailbox_expunge(), if mailbox_rewrite_index_record() returns failure, the event_state object will be leaked.

• In mboxevent_extract_mailbox(), you call the new function mailbox_count_unseen(), which scans the entire mailbox to count the unseen messages. About half of the callers have a struct index_state lying around which already contains that information.

• In cmd_rename(), the code starting with the comment / send a MailboxRename event notification if enabled / can be entirely skipped by the 'goto submboxes' in the previous line under some circumstances, which will leak the event_state object.

• In index_expunge(), there does not seem to be any good reason to use a field in struct index_state to store the event_state* pointer, as it's entire lifecycle is in that function.

• Also in index_expunge(), it's probably a good idea to call mboxevent_notify() after index_unlock(), to avoid doing networking while holding the index lock.

• In index_fetch(), there does not seem to be any good reason to use a field in struct index_state to store the event_state* pointer, you could just pass it down to _fetch_setseen().

• Also in index_fetch(), it's probably a good idea to call mboxevent_notify() after index_unlock(), to avoid doing networking while holding the index lock.

• You're generating the MessageRead event when in index_fetch() when the \Seen flag is set as a side effect of FETCHing a message body, which is good. However you're not generating an event in index_storeflag() when the \Seen flag is set explicitly using STORE. The RFC says

> MessageRead > One or more messages in the mailbox were marked as read or seen by > a user.

which definitely sounds like it includes a STORE command.

• Hmm, the way events are updated in index_storeflag() is quite clunky. It might be cleaner to make index_storeflag() keep a running summary of the union of all system and user flags set, and then create one or two events up in index_store().

• I'm not convinced that the set of events generated as a side effect of STORE is correct in all cases. In particular, consider this initial state

  uid 1, flags \Deleted uid 2, flags

  and then the command

  1 STORE 1:2 +FLAGS \Deleted

  will generate a MessageTrash event for uids 1 and 2, even though only uid 2 was actually changed by the command.

• In mboxlist_setquotas(), the variable quotachange_state is used both as the head of a list of event_states and also as a pointer to a particular distinguished event_state, which is very confusing.

• In mboxlist_setquotas(), the logic beginning at the comment / ... if now under quota / knows too much about the internals of a struct quota and should be in a function in quota_db.c.

• In mboxlist_changesub(), shouldn't you be calling mboxevent_extract_mailbox() instead of manually filling out the event->mailboxid field?

• In mboxname_notify() the new string returned from json_formatter() is leaked.

[brong]

From: Sébastien Michel

(In reply to comment #6) > Second tranche of reviews, more later.

> ... > - So the issue of messageContent is interesting. This sounds to > me like it was easy to specify but not all that useful in this > day of MIME messages, if the purpose of messageContent is to > provide a human with a brief idea of what the message is about. > At Fastmail we use a separate process which pulls apart MIME > structures, does MIME decoding, strips HTML, and generates a > short UTF-8 string intended as a user-visible preview of the > message. This preview is stored in a per-message annotation. > It would great if we could include that annotation in the > mboxevent.

yeah! per-message annotation is very useful for lots of thing. I thought to store image thumbnails to speed up preview We can plan such development later

> - In mboxevent_extract_quota(), you use a magical 1024 to > report the diskUsed parameter. There's an array > const int quota_units[QUOTA_NUMRESOURCES] designed for this. Agreed. Both RFC 2087 and RFC 5423 display quota usage in units of 1024

> - In json_escape_str(), hex_chars[] doesn't seem at all > sensible. This > > sprintf(seq, "\u00%c%c", hex_chars[c >> 4], hex_chars[c & 0xf]); > > could be more sensibly implemented as > > sprintf(seq, "\u04x", (unsigned int)c); > > - Also in json_escape_str(), there's a buf_printf() that would be > useful than a sprint()/buf_append_cstr() pair. Also, there's > really not a lot of point saving up ranges of unescaped characters > and calling buf_appendmap() when you could just do a buf_putc() > on every unescaped character as you encounter it. I think in a > function like this, simplicity and obvious correctness is more > important than a few CPU cycles. > > - In json_escape_str(), why escape '/' ? RFC4627 says > > > All Unicode characters may be placed within the > > quotation marks except for the characters that must be escaped: > > quotation mark, reverse solidus, and the control characters (U+0000 > > through U+001F).

Indeed. But little below the grammar indicates to escape '/' : >string = quotation-mark *char quotation-mark > > char = unescaped / > escape ( > %x22 / ; " quotation mark U+0022 > %x5C / ; \ reverse solidus U+005C > %x2F / ; / solidus U+002F > %x62 / ; b backspace U+0008 > %x66 / ; f form feed U+000C > %x6E / ; n line feed U+000A > %x72 / ; r carriage return U+000D > %x74 / ; t tab U+0009 > %x75 4HEXDIG ) ; uXXXX U+XXXX However online validators don't expect to escape solidus...

> - Actually, come to that, why are you writing your own JSON generator > when there's a bunch of perfectly good ones in C listed at json.org? > Surely one of them will have a compatible licence and be well designed > and tested. It was my first intention. But I noticed that using external libraries seems to be avoided in Cyrus : don't using glib for data structure, lib ICU for charset handling, library for base64 conversion and others. Is there any guideline regarding this point ? json-c seems popular and is included in fedora and have MIT licence

> - In append_fromstage() I'm really not impressed by the trick you use > of constructing a quota_t using an handcrafted initialiser. This is > too sensitive to the internals of struct quota, which are being changed > in other work that's going on. It would be far better if you could > grab this same information as a side effect of the code in mailbox.c > that already calculates it properly, and decorate an existing event_state > chain with it. Also, you're setting the QUOTA_STORAGE resource but > not QUOTA_MESSAGES. I Agree. I also thought to rewrite this piece of code.

[brong]

From: Sébastien Michel

(In reply to comment #7) > Final trance of comments. > > So, on further thought, there's some room for simplification > in enum event_param_type. Having separate types for _STRING > and _DYNSTRING is probably worth the complexity, given the > number of strdups you're saving. But I think all the integral > types could be collapsed into a single type which is a uint64_t. > The only _INT parameters are diskQuota and maxMessages, and in > both cases they have no valid value < 0 (they can be -1, but that > means "no limit is enforced", and should be represented by > omitting the parameter rather than emitting -1). The _QUOTAT > parameters only report absolute quota values not deltas, so > they don't need to be signed either. The _MODSEQT ones need to > be unsigned. > Good idea.

> In a similar vein, I don't think there's any need to go saving > a copy of an broken-out struct imapurl anywhere. The only reason > for doing that would be if you needed to update the URL conditionally > or gradually as more information is discovered. You do currently > do that, in mboxevent_notify() you add a UID to event->mailboxID. > However, note that the RFC describes that parameter thus > > > mailboxID > > Included in events that affect mailboxes. A URI describing the > > mailbox. In the case of MailboxRename, this refers to the new > > name. > > anb RFC2192 section 7 says that an imap: URL with UID= in it > describes a message, not a mailbox. So in this case > you're actually emitting the wrong form of URL.

You're right and it's more clear now, it's a mistake.

> So if you only > emit the correct form or URL, you don't need to keep a struct > imapurl around. So you could just take the mailbox* when it's > passed down in the API, use a temporary struct imapurl to build > the string form of the URL, throw the imapurl away, and save > the string as event->mailboxID.

But I should add conditionally the UID as described in the RFC : > Events referring to a specific message can use an > IMAP URL [RFC5092] to do so. Events referring to a set of messages > can use an IMAP URL to the mailbox plus an IMAP UID (Unique > Identifier) set.

I understand that we should use the 'uri' parameter for MessageNew and MessageXXXX for single message (and eventually MailboxID that seems redundant). Using mailboxID and uidsed for MessageXXXX in case of several messages (and optionnaly uri parameter that seems redundant)

I should also translate the mailbox identifier which is in the internal form. But translate to external is not everywhere possible since I don't have access to struct namespace. If you have a suggestion I'll be happy !

> - The logic in append_apply_flags() is wrong, it will report in the > event the flags that the client attempted to set, rather than the > flags that the client successfully set after ACL checking. Oh yes

> > - In mboxevent_extract_mailbox(), you call the new function > mailbox_count_unseen(), which scans the entire mailbox to count > the unseen messages. About half of the callers have a struct > index_state lying around which already contains that information. I missed that information

> > - You're generating the MessageRead event when in index_fetch() when > the \Seen flag is set as a side effect of FETCHing a message body, > which is good. However you're not generating an event in > index_storeflag() when the \Seen flag is set explicitly using STORE. > The RFC says > > > MessageRead > > One or more messages in the mailbox were marked as read or seen by > > a user. > > which definitely sounds like it includes a STORE command. Yes I do :

> if (state->internalseen) { > / set the seen flag / > if (im->isseen) > im->record.system_flags |= FLAG_SEEN; > else > im->record.system_flags &= ~FLAG_SEEN; > } > [...] > if (mboxevent_add_sysflags(flagsset_state, ~old & im->record.system_flags)| > mboxevent_add_usrflags(flagsset_state, state->mailbox, newusr)) > mboxevent_extract_record(flagsset_state, state->mailbox, &im->record);

It works only for internalseen and not for shared seen that is assumed

> > - I'm not convinced that the set of events generated as a side effect > of STORE is correct in all cases. In particular, consider this > initial state > > uid 1, flags \Deleted > uid 2, flags > > and then the command > > 1 STORE 1:2 +FLAGS \Deleted > > will generate a MessageTrash event for uids 1 and 2, even though only > uid 2 was actually changed by the command. No it's OK, I already test this case in my test suite

> > - In mboxlist_changesub(), shouldn't you be calling > mboxevent_extract_mailbox() instead of manually filling out the > event->mailboxid field? I don't have a struct mailbox at this point

[brong]

From: Greg Banks

(In reply to comment #8) > (In reply to comment #6)

> > > - Actually, come to that, why are you writing your own JSON generator > > when there's a bunch of perfectly good ones in C listed at json.org? > > Surely one of them will have a compatible licence and be well designed > > and tested. > It was my first intention. But I noticed that using external libraries seems to > be avoided in Cyrus

AFAICS this is more due to historical accident and poor coding practices than any kind of actual policy, but then I've only been around for a couple of years. I think this it is a major flaw of Cyrus that it re-implements, poorly, a lot of code for which good and widely available libraries exist.

> : don't using glib for data structure, lib ICU for charset > handling, library for base64 conversion and others. Is there any guideline > regarding this point ?

My personal rule has been: if a well-designed library can be found with a suitable licence and a minimal dependency tree and available in both RH and Ubuntu, then use it. I find it ridiculous that we're maintaining our own code to do all those things you mentioned. The only reason we still are is a) I have finite time and b) I want to make sure I'm not breaking anything.

My own personal TODO list includes replacing all the open-code list manipulation code with http://ccodearchive.net/info/list.html

> json-c seems popular and is included in fedora and have MIT licence

and it's available on Ubuntu...smells like a winner.

[brong]

From: Greg Banks

(In reply to comment #9) > (In reply to comment #7)

> > > So if you only > > emit the correct form or URL, you don't need to keep a struct > > imapurl around. So you could just take the mailbox* when it's > > passed down in the API, use a temporary struct imapurl to build > > the string form of the URL, throw the imapurl away, and save > > the string as event->mailboxID. > > But I should add conditionally the UID as described in the RFC : > > Events referring to a specific message can use an > > IMAP URL [RFC5092] to do so. Events referring to a set of messages > > can use an IMAP URL to the mailbox plus an IMAP UID (Unique > > Identifier) set. > > I understand that we should use the 'uri' parameter for MessageNew and > MessageXXXX for single message (and eventually MailboxID that seems redundant). > Using mailboxID and uidsed for MessageXXXX in case of several messages (and > optionnaly uri parameter that seems redundant)

I think the RFC says that for any of the events MessageExpires, MessageExpunges, MessageRead, MessageTrash, FlagsSet, or FlagsClear you should include the fields

mailboxID: imap URL of the mailbox

uri: also the imap URL of the mailbox

uidset: list of uids affected

and for any other events which mention a message, you should include

mailboxID: imap URL of the mailbox

uri: imap URL of the message

In other words, the form of the 'uri' parameter should depend only on the event type and not on the number of messages referenced. Also, the 'uri' parameter should always be present.

> I should also translate the mailbox identifier which is in the internal form.

Yes.

> But translate to external is not everywhere possible since I don't have access > to struct namespace. If you have a suggestion I'll be happy !

The whole namespace thing is very annoying. It's effectively a global, but it's been isolated into imapd.c and a pointer needs to be passed around nearly everywhere. In the long term the best thing is probably to actually have a global pointer, but in the short term you'll need to do something uglier.

One thing you could do is make the list of pending event structures a global, and flush it out at the end of each imap command, e.g. with a single call from cmdloop(). That call could then take a namespace* to convert the stored internal mailbox name to an external name before building URLs. This would be less invasive to various structs that you've modified, but the downside might be that the "abort the notification" semantics would be a lot hairier.

Alternatively, you could pass the namespace* to the mboxevent code during service initialisation, possibly as an argument to mboxevent_init(), and store the pointer in a static variable in mboxevent.c

> > - I'm not convinced that the set of events generated as a side effect > > of STORE is correct in all cases. [...] > No it's OK, I already test this case in my test suite

Ah, tell me more about this test suite - can we integrate it into Cassandane?

> > - In mboxlist_changesub(), shouldn't you be calling > > mboxevent_extract_mailbox() instead of manually filling out the > > event->mailboxid field? > I don't have a struct mailbox at this point

Sure, but you should really be calling an API function rather than reaching into the internals of the event structure.

[brong]

From: Sébastien Michel

> > I understand that we should use the 'uri' parameter for MessageNew and > > MessageXXXX for single message (and eventually MailboxID that seems redundant). > > Using mailboxID and uidsed for MessageXXXX in case of several messages (and > > optionnaly uri parameter that seems redundant) > > I think the RFC says that for any of the events MessageExpires, > MessageExpunges, MessageRead, MessageTrash, FlagsSet, or FlagsClear > you should include the fields > > mailboxID: imap URL of the mailbox > > uri: also the imap URL of the mailbox > > uidset: list of uids affected > > and for any other events which mention a message, you should include > > mailboxID: imap URL of the mailbox > > uri: imap URL of the message > > In other words, the form of the 'uri' parameter should depend only on the > event type and not on the number of messages referenced. Also, the 'uri' > parameter should always be present.

I just have a doubt about mailboxID : > Included in events that affect mailboxes.

I understand only to mailbox related events : MailboxCreate, MailboxDelete, MailboxRename, MailboxSubscribe and MailboxUnsubscribe

I've nearly finish the rewrite in accordance with the first and second tranche of reviews. I'm working on the third.

[brong]

From: Sébastien Michel

I've rewritten the code in accordance with all review comments of Greg. I've also broken up the commits into more readable smaller chunks and pushed in the branch ticket/3605. Full history of the rewrite is also available in our github repo : https://github.com/worldline-messaging/cyrus-imapd/commits/temp/ticket/3605/

I made some implementation choices that can be discussed, however :

• I removed "event_timestamp_format" option
• option "event_exclude_folders" is renamed to "event_exclude_specialuse". It uses SPECIAL-USE to exclude folders instead of using folder names
• I choose libjson that seems to be the "default" in some Linux distro. However this library will only support 64bit integer in the next 0.10 release. I've added a requirement on this next release.
• I choose to write a basic json formatter in xjson.[ch] as fallback. But I can change to another format if libjson library is not found (ie. properties format with key/value pairs)

I'm still discussing with Dilyan about where we should define xjson in Makefile.am

[brong]

From: Sébastien Michel

Changed the use of json-c by jansson to format JSON object, because current version of json-c doesn't support 64bit integer

JSON library is the only external dependency used by the event notification piece of code.

Regarding this dependency I have two possibilities in configure.ac :

• keep the internal xjson.[ch] as fallback when jansson is not available or in an older version. And print a warning to highlight this fallback.
• add an option to the configure script such as --enable-event-notification or --enable-mboxevent and raise an error if requirement on jansson failed (no more need to have internal xjson as fallback)

any opinion ?

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

(In reply to comment #14) > - add an option to the configure script such as --enable-event-notification or > --enable-mboxevent and raise an error if requirement on jansson failed (no more > need to have internal xjson as fallback) >

This option has my vote.

[brong]

From: Sébastien Michel

(In reply to comment #15) > (In reply to comment #14) > > - add an option to the configure script such as --enable-event-notification or > > --enable-mboxevent and raise an error if requirement on jansson failed (no more > > need to have internal xjson as fallback) > > > > This option has my vote.

I've added the option --disable-event-notification. So mailbox event notification is enabled by default and requires that jansson library is installed with its pkg-config file. I've also removed files xjson.[ch] that were used when jansson was missing. I did a rebase and pushed to the git repo (branch ticket/3605)

[brong]

From: Bron Gondwana

Is there anything blocking merging this? I'd like to get it in soon so we can start nailing down 2.5.0.

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

(In reply to comment #17) > Is there anything blocking merging this? I'd like to get it in soon so we can > start nailing down 2.5.0.

As I recall, Sebastien Michel wanted to review and clean up some commits.

Sebastien can make the merge himself, actually - he's got the git account.sebastien.michel@atos.net.

[brong]

From: Sébastien Michel

> As I recall, Sebastien Michel wanted to review and clean up some commits. > > Sebastien can make the merge himself, actually - he's got the git > account.sebastien.michel@atos.net.

All the work is done for a long time. I just made a last rebase of the branch ticket/3605 onto master and fixed some conflicts

I am ready to do the final merge in master if OK for you.

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

(In reply to comment #19) > All the work is done for a long time. I just made a last rebase of the branch > ticket/3605 onto master and fixed some conflicts > > I am ready to do the final merge in master if OK for you.

OK, +1.

[brong]

From: Sébastien Michel

pushed to master

[brong]

Closing: was RESOLVED in import