Duplicate Dictionary Key Exception During Processing of Certain Messages with Multiple Attachments

[gitrkenn]

Hi Dijji:

First, thank you for a very useful app!

I ran across what may appear to be a very minor (or not) bug. The exception was thrown during the processing of a message with multiple attachments. The message contained two attachments (from the List<Attachment> Attachments collection). However, for both attachments, although their HasContentId was true, the fields Content was null and ContentId was "" (empty string) for both instances (is this normal?). Therefore, when trying to add to the Dictionary via dict.Add(a.ContentId, a), that triggered the exception below:

System.ArgumentException: An item with the same key has already been added. at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource) at System.Collections.Generic.Dictionary'2.Insert(TKey key, TValue value, Boolean add) at XstReader.Message.EmbedAttachments(String body, XstFile xst)

I merely made the modification as shown below to prevent the exception. Both attachments showed up correctly in the message window (also was able to save both attachments successfully), but not sure that was the correct fix.

if (!dict.ContainsKey(a.ContentId))
{
    dict.Add(a.ContentId, a);
}

Reference in Message.cs where exception was thrown.

[Dijji]

Thank you for this. A very clear and helpful report.

I think that the deeper problem is probably that HasContentId is picking up too many attachments that aren't really intended as message content at all. As well as the problem you find, this will give false positives for MayHaveInlineAttachment

Instead of your change, could you try changing line 336 of view.cs to:

public bool HasContentId { get { return (ContentId != null && ContentId.Length > 0); } }

and try and reproduce the problem?

I think this should work, but please let me know. To be really thorough, if it does work, I will go both with my change and your change, the latter being to counter any spurious setting of ContentId to the same mystery non-empty value in multiple attachments. The alternative would be to defer the look up of the ContentId to within the match handling, where we know what were looking for, but that seems uglier.

Dijji

[gitrkenn]

Dijji: I implemented your suggested change and that also worked. But inline with your thoughts, I also left the change I made in place just in case there are other unexpected scenarios haven't encountered (where if for some reason, the ContenId is duplicated, either as empty or non-empty).

[Dijji]

I have made the changes, tested them locally, and committed them to master. They will form part of the next release.

Thanks again!