fix: parse attachments from streaming request

[joshgummersall]

Fixes #3887

[coveralls]

Pull Request Test Coverage Report for Build 1082891443

• 14 of 25 (56.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 84.455%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
libraries/botbuilder/src/botFrameworkAdapter.ts	4	8	50.0%
libraries/botbuilder/src/cloudAdapter.ts	0	7	0.0%

Totals
Change from base Build 1082884211:	-0.03%
Covered Lines:	19574
Relevant Lines:	21949

---

💛 - Coveralls

[carlosscastro]

You probably are aware of this, but adding this note just in case -- attachments have a content type, and depending on that content type they could be json, cards, audio, text, etc. Another dimension is whether the attachment is inlined or has a url. I'd suggest to try this out with a few different contents and mechanism combinations to make sure we are covering the scenario families.

[carlosscastro]

If you know for a fact that this code is independent of any corner case combination then that's fine. In C# there are 2 families of attachments to test to cover pretty much all cases, I'm just not sure what the minimum test base is for this code without spending a bit more time on it.

[compulim]

I am not expert on this topic, you can refer to how C# SDK works.

FYI, someone told me, stream could be never-ending, e.g. a speech stream. That means await Promise.all() could (almost) never resolve.

Maybe we didn't go as far as this yet even in C#.

[joshgummersall]

@compulim, the IReceiveRequest, in this context, is a discrete set of bytes from a socket. The socket, itself, is a stream that is never-ending.

@carlosscastro working on verifying now, thanks for the pointers! I think I have found another hiccup when adding contentType-aware stream reading.

[carlosscastro]

@compulim good point! the endless stream could exist in the highest protocol layer, which assembles streams. However, this is a requirement that was part of the original design but it's not completely implemented, and we don't really use it for any practical scenario and many parts of the code don't really support that.

[joshgummersall]

@carlosscastro I've fixed the content type issue in the latest commit, please have a look and see if you agree with the changes. I verified that attachments coming in via DL ASE now appear well-formed (i.e. I'm getting a content type and either a JSON-parsed object for application/json or a string of chars otherwise).

[stevengum]

Minor nits around unused imports, otherwise it looks good!