Update add attachment interface #522

shortstuffsushi · 2017-09-16T16:51:20Z

Support attachment adding via stream. I have a couple of question's I'd like to get feedback on. This ultimately addresses #511, though it doesn't add an overload for plain content being base64'd internally.

Anyway, questions:

I chose to make the method async, because it's reading an unknown stream. Would a non-async method be desirable as well?
The function currently just returns in several failure scenarios. It could instead return a result type value indicating status and error messages (think IdentityResult)
Unit tests don't currently seem to cover this, should I add them?
The example usage projects don't seem to show attachment handling, should I update them projects?
[Edited] I'm truncating the stream at int.MaxValue which will be 2gb. As discussed below, there's potential to check message length. Is that desired?

* Support adding a single Attachment object * Clean up if else, just "if doesn't exist, create" then add item * Same for multiple, make if not exists, then add range * There likely would have been bug in the multiple because it was assigning a list passed in, which could have been modified from outside * Remove return at end of function in void functions

This caused confusion internally, so a little more detail would help.

Read a stream's bytes, base64 encode, attach. Arrange usings.

SendGridDX · 2017-09-16T16:51:37Z

All committers have signed the CLA.

Jericho · 2017-09-17T20:38:12Z

To your point regarding the size restriction: the way I understand SendGrid's documentation, the restriction applies to the total size of an email not just the size of its attachments which begs the question: how do you calculate the size of an email?

shortstuffsushi · 2017-09-17T20:58:26Z

@Jericho the size of the email would be, as I understand it, the sum of the content size and attachment size. It could be calculated, but I didn't do it the first time around because I'm sure that'll all be double checked server side anyway.

To be honest, the reason I added any size check was to avoid the bizarre discrepancies in the Stream API, where Stream.length is a long but read is Stream.read(byte[] buffer, int offset, int count). If there are better thoughts around how to handle that, I'd love the hear them. I didn't really come across any good ideas for it yesterday, and took the easy route -- never handling anything bigger than int.MaxValue.

Jericho · 2017-09-17T21:07:54Z

Are you sure it's just the sum of content + sum of attachment size? What about recipients names and addresses? What about subject line? (these are just some example off the top of my head) Should they be included in "total" size? Also, is the size calculated using the JSON string including attributes? I'm just trying to point out that it's not as trivial as it may sound.

Also, in my opinion, the right place to check if an email exceeds the size limit is when it's about to be sent.

shortstuffsushi · 2017-09-17T21:10:06Z

I guess I can't speak to what fields are all included in the SendGrid idea of "total size," and indeed -- it should (and likely will be) calculated by their servers before sending. As I mentioned, this was my way of avoiding the Stream API issue, if you have thoughts on that, I'd be interested in hearing them.

Jericho · 2017-09-17T21:11:14Z

I'm mentioning all this because I went through the exact same thought process when I was writing the StrongGrid library as you just did. In the end I had too many unanswered questions and I decided not to do any size checking. I am hoping SendGrid returns a meaningful error message if someone attempts to send an email that exceeds the limit (although I have to admit I have never actually tested this scenario).

shortstuffsushi · 2017-09-17T21:18:30Z

Ok, so I decided to just take a look at your implementation to see if you had come up with a solution, but it appears that you chose to instead just truncate the stream which is what I was hoping to avoid. Obviously this won't matter in real use cases for a couple reason:

Loading an attachment > int.MaxValue into memory would be unrealistic in most applications (int.MaxValue I believe correlates to 2GB)
Uploading would take a while
SendGrid would ultimately just reject it anyway

So, perhaps the naive implementation is alright because it'll "mostly work" and fail in the extreme case for a number of reasons anyway. Again, I'd be interested in more graceful ways of handling this, as I've never run into the situation of trying to copy a stream in this manner, and I'm not sure what the usual implementation is.

Jericho · 2017-09-17T21:37:02Z

I don't think overcoming the "long to int conversion causing the stream to be truncated if file exceeds 2GB" issue is worthwhile since we know that SendGrid would reject an email with a 3GB or 5GB or 10GB attachment anyway.

You are making me realize that checking the size before attaching a file makes sense in order to prevent a developer from attempting to add a file so large that it would take so much memory, be very slow and be rejected by SendGrid anyway. I'll add this attachment size check to my implementation (and give you credit!) but I still would like to know how SendGrid calculates an email "total" size in order to implement this check before attempting to send an email.

Instead, just truncate long to int.

More changes around stream truncation and not handling size.

thinkingserious · 2017-09-18T18:36:45Z

@Jericho,

Thanks for jumping in with feedback!

The total size is calculated from the entire JSON payload. Note that you will need to handle the case where UTF-8 characters are included.

@shortstuffsushi,

Your PR is now queued up on our backlog for review. Thanks!

With Best Regards,

Elmer

mbernier · 2017-10-28T20:50:00Z

@Jericho can you please give this a look and let us know your thoughts? Should we modify anything before merge? Will it cause any issues with any of the suggested work we have in #530

Jericho · 2017-10-28T22:29:21Z

@shortstuffsushi you decided against checking size of each attachment and the total size of a message? The discussion you and I had about this topic inspired me to implement the size check in StrongGrid and I thought you were doing the same for SendGrid's library.

shortstuffsushi · 2017-10-29T00:03:32Z

I did decide against the per file check. The total size would still be relevant, but I didn't choose to do it here. It was indicated above that I would need to check the entire json payload size, which seemed out of scope for my method add. Still probably worth doing, but not here imo.

thinkingserious · 2017-10-31T03:48:20Z

Hi @shortstuffsushi,

My apologies for taking forever to review this :(

To merge this, I just need 2 more things:

Please do add unit test(s)
Please add an example of how to use this feature to USE_CASES.md

No worries if we don't merge this in October, you have earned your hacktoberfest swag :)

With Best Regards,

Elmer

shortstuffsushi · 2017-11-02T21:17:30Z

Thanks @thinkingserious -- I'll try to get to this over the weekend.

Make class interface more generic.

shortstuffsushi · 2017-11-06T18:01:42Z

@thinkingserious I added some tests and some notes to the markdown file. I wasn't able to get the integration tests working, and I didn't really look into why. Are those relevant for what I'm doing? Can you re-review this now?

thinkingserious · 2017-11-09T06:22:29Z

Thanks @shortstuffsushi!

I am currently working through the flood of PRs we received from Hacktoberfest. There were over 1,000!

Thanks in advance for your patience.

With Best Regards,

Elmer

thinkingserious

Nice work!

thinkingserious · 2017-11-28T21:49:04Z

src/SendGrid/Helpers/Mail/SendGridMessage.cs

        }

        /// <summary>
        /// Add attachments to the email.
        /// </summary>
        /// <param name="attachments">A list of Attachments.</param>
-        public void AddAttachments(List<Attachment> attachments)
+        public void AddAttachments(IEnumerable<Attachment> attachments)


I like using the IEnumerable here, but I'm concerned about breaking changes by removing the old function signature. How about keeping the old function signature and adding this one as a function overload?

Because List inherits from IEnumerable, this shouldn't be a breaking change. Anyone using lists should see things continue to work as though nothing changed.

Awesome! Thanks for the super quick response :)

thinkingserious · 2017-11-28T23:31:19Z

Hello @shortstuffsushi,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

shortstuffsushi · 2017-12-06T15:05:58Z

Thanks @thinkingserious. I'm hoping to help out more in the future, but I've been a bit busy lately as you can probably tell by the slow responses. Hopefully I'll be talking to you again soon.

thinkingserious · 2017-12-14T01:06:26Z

That sounds excellent @shortstuffsushi! Thank you for the follow up :)

shortstuffsushi added 3 commits September 16, 2017 10:24

Update param description.

43b1b1f

This caused confusion internally, so a little more detail would help.

Add method for adding attachment streams.

869af03

Read a stream's bytes, base64 encode, attach. Arrange usings.

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Sep 16, 2017

Jericho mentioned this pull request Sep 17, 2017

Check attachment size Jericho/StrongGrid#191

Closed

shortstuffsushi added 2 commits September 18, 2017 09:09

Remove extra logic around length.

2709194

Instead, just truncate long to int.

Whoops, this didn't go through on the last commit.

0540d7c

More changes around stream truncation and not handling size.

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Sep 18, 2017

thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Sep 30, 2017

This was referenced Sep 30, 2017

Create an Attachment Helper #248

Closed

SendGridMessage.AddAttachment now requires Base64 encoded content #511

Closed

shortstuffsushi added 4 commits November 6, 2017 11:55

Null / empty check.

89f89af

Make class interface more generic.

Clean up usings so I can compile.

d266816

Add a bunch of tests around the AddAttachment methods.

6525eaa

Add example file stream usage in USE_CASES.md

babd710

thinkingserious requested changes Nov 28, 2017

View reviewed changes

thinkingserious approved these changes Nov 28, 2017

View reviewed changes

thinkingserious merged commit a6fcf87 into sendgrid:master Nov 28, 2017

shortstuffsushi deleted the update-add-attachment-interface branch December 6, 2017 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update add attachment interface #522

Update add attachment interface #522

shortstuffsushi commented Sep 16, 2017 •

edited

Loading

SendGridDX commented Sep 16, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading

shortstuffsushi commented Sep 17, 2017

Jericho commented Sep 17, 2017 •

edited

Loading

shortstuffsushi commented Sep 17, 2017

Jericho commented Sep 17, 2017 •

edited

Loading

shortstuffsushi commented Sep 17, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading

thinkingserious commented Sep 18, 2017

mbernier commented Oct 28, 2017

Jericho commented Oct 28, 2017

shortstuffsushi commented Oct 29, 2017

thinkingserious commented Oct 31, 2017

shortstuffsushi commented Nov 2, 2017

shortstuffsushi commented Nov 6, 2017

thinkingserious commented Nov 9, 2017

thinkingserious left a comment

thinkingserious Nov 28, 2017

shortstuffsushi Nov 28, 2017

thinkingserious Nov 28, 2017

thinkingserious commented Nov 28, 2017

shortstuffsushi commented Dec 6, 2017

thinkingserious commented Dec 14, 2017

Update add attachment interface #522

Update add attachment interface #522

Conversation

shortstuffsushi commented Sep 16, 2017 • edited Loading

SendGridDX commented Sep 16, 2017 • edited Loading

Jericho commented Sep 17, 2017 • edited Loading

shortstuffsushi commented Sep 17, 2017

Jericho commented Sep 17, 2017 • edited Loading

shortstuffsushi commented Sep 17, 2017

Jericho commented Sep 17, 2017 • edited Loading

shortstuffsushi commented Sep 17, 2017 • edited Loading

Jericho commented Sep 17, 2017 • edited Loading

thinkingserious commented Sep 18, 2017

mbernier commented Oct 28, 2017

Jericho commented Oct 28, 2017

shortstuffsushi commented Oct 29, 2017

thinkingserious commented Oct 31, 2017

shortstuffsushi commented Nov 2, 2017

shortstuffsushi commented Nov 6, 2017

thinkingserious commented Nov 9, 2017

thinkingserious left a comment

Choose a reason for hiding this comment

thinkingserious Nov 28, 2017

Choose a reason for hiding this comment

shortstuffsushi Nov 28, 2017

Choose a reason for hiding this comment

thinkingserious Nov 28, 2017

Choose a reason for hiding this comment

thinkingserious commented Nov 28, 2017

shortstuffsushi commented Dec 6, 2017

thinkingserious commented Dec 14, 2017

shortstuffsushi commented Sep 16, 2017 •

edited

Loading

SendGridDX commented Sep 16, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading

shortstuffsushi commented Sep 17, 2017 •

edited

Loading

Jericho commented Sep 17, 2017 •

edited

Loading