Conversation
…y response This is as per the suggestion in aws#74.
normj
left a comment
normj
left a comment
There was a problem hiding this comment.
Good feature add. Just want to make a slight change.
|
|
||
| /// <summary> | ||
| /// An optional set of Content-Types that will trigger encoding the response content in Base64. | ||
| /// </summary> |
There was a problem hiding this comment.
What if we switch this around. Maintain a collection of common content types that we don't need to be base 64 encoded like text/html. Don't expose the data collection but add an AddTextContentType operation. Then when processing the response if the content type is not in the collection then base 64 encode it.
This way we can make it so that must users don't have to anything special to handle binary type.
There was a problem hiding this comment.
I like the works by default strategy. :-)
With this change, do you also want to do away with the XXXContentType property and only have the one method AddXXXContentType() as the sole interface point to this feature? If yes, should there also be a way to inspect the list and remove from the list (i.e. enumerator and RemoveXXXContentType())?
There was a problem hiding this comment.
Was thinking about it a bit more -- does it make sense to support it "both ways"? For example, you can establish a default behavior (to encode or not to encode), and then define a list of exceptions -- the content types that alter the default behavior. And then of course, the default configuration is as you described which would accommodate most users. Something like:
enum ResponseEncoding
{
String = 0, // Or UTF8 or something...
Base64 = 1,
}
class APIGatewayProxyFunction
{
protected Dictionary<string, ResponseEncoding> _responseEncodingForContentType =
new Dictionary<string, ResponseEncoding>
{
// The defaults, such as from https://en.wikipedia.org/wiki/Media_type#Common_examples
["text/plain"] = ResponseEncoding.String,
["text/html"] = ResponseEncoding.String,
["text/css"] = ResponseEncoding.String,
["application/xml"] = ResponseEncoding.String,
["application/json"] = ResponseEncoding.String,
// etc...
};
public ResponseEncoding DefaultResponseEncoding
{ get; set; } = ResponseEncoding.Base64;
public void AddResponseEncodingForContentType(string contentType, ResponseEncoding encoding)
{
// ...
}
}Don't want to explode this feature change, but just thinking about the possible usage scenarios and what would be most stable for the long-term.
There was a problem hiding this comment.
Just occurred to me there is one more dependency to consider with whatever interface changes are made to support this, namely that you also have to update (and keep in sync) the Binary Support settings of the API Gateway by enumerating the binary content-types that your app will support.
There was a problem hiding this comment.
I like the code changes except I would keep _responseEncodingForContentType private at least for now. I'd rather make it protected later if we find we really need to.
Hopefully the API Gateway setting will be made available in the serverless.template at some point which is where I would expect that to be maintain. Right now that seems beyond the scope of this change.
|
@normj, I've incorporated all the changes we discussed. One thing to note, because the default disposition is to treat all responses as binary unless otherwise specified through registrations and/or default encoding property, many of the existing unit tests were failing because they were comparing content that was now being returned Base64-encoded. For that reason you'll see that most of tests were revised to use a slightly different pattern for looking at the response content body with the help of a new extension method on the response object. For example, what used to be something like: Assert.True(response.Body.Length > 0);is now Assert.True(response.BodyAsString().Length > 0);The extension method now looks at the new I suppose this is OK, since the AspNetCoreServer package is still considered experimental? |
* Forgot to remove unnecessary usings * Forgot to rename file after renaming enumeration
|
I was testing this feature in my dev branch. I didn't realize that for proxy requests, which is what we use for the ASP.NET Core stuff, the client has to send an Accept header for the binary content type. Seeing as this is likely to be called from a browser which isn't going to send the Accept header is this feature still useful? |
|
I agree the API Gateway (and maybe Lambda?) teams are placing some unusual, and seemingly unnecessary obstacles on making the most of their platforms, but perhaps they'll relax those restrictions at some point. If they simply allowed for a "default" mapping of binary content type, that would go a long way with many apps, perhaps I'll submit that request... Regardless, this support for binary content is still quite accessible when dealing with non-browser clients, such as mobile apps or custom clients. Also, even with conventional browsers, the request |
|
Actually just discovered something very useful -- it turns out that you can force APIGW + Lambda to always respond with binary content if you simply specify a Binary media type of I've confirmed, with this setting in place, the behavior of ASP.NET Core on Lambda now seems compatible with the expected behavior otherwise. I original opened this PR to clean up a similar hack I had to introduce in my project and with this latest change, I've confirmed the behavior is now compatible with millions of clients deployed on Windows. |
|
@normj, any other suggestions? |
|
@ebekker thanks for implementing this feature! This would be especially useful for integrating Swashbuckle to a net core web API lambda project. We were able to integrate Swashbuckle successfully and can access the swagger documentation through API gateway, the only bottleneck now is getting the swagger ui image files through API gateway and I hope that can be solved by having |
|
Sorry I have been off for a bit and coming back to this. Initially when I suggested we default to Base64 I assumed everything would be transparent to the user but since we found out it requires additional configuration on the API Gateway side I think we should change I don't need another pull request for the change. I have things merged into a local branch. I just wanted your thoughts before making that change. |
|
So you're saying that 'DefaultResponseContentEncoding' will be changed, but you would still including the base set of content-type specific mappings (the initial set in I'm OK with that, we would still need to update documentation for folks to make that work (i.e. provide guidance on adding the wildcard Do you have any insight on to the timeline of updating the SAM support for adding the ability to define Binary mappings? It looks like it stems from a limitation that Binary mappings can't even be defined by CloudFormation at this point, so I guess that team needs to bring that support up to speed first. |
|
Yes, I just changed DefaultResponseContentEncoding and left _responseContentEncodingForContentType alone. So images will be base64 encoded by default. Sorry I don't have any information about the SAM support for adding the ability to define binary mappings. |
|
Sounds good to me. |
|
|
||
| ## Supporting Binary Response Content | ||
|
|
||
| The interface between the API Gateway and Lambda provides for and assumes repsonse content to be returned as a UTF-8 string. |
There was a problem hiding this comment.
typo: repsonse -> response
|
|
||
| In order to facilitate this mechanism, the `APIGatewayProxyFunction` base class maintains a registry of MIME content types | ||
| and how they should be transformed before being returned to the calling API Gateway. For any binary content types that are | ||
| returned by your applicaiton, you should register them for Base64 tranformation and then the framework will take care of |
There was a problem hiding this comment.
applicaiton -> application
| using (var ms = new MemoryStream()) | ||
| { | ||
| responseFeatures.Body.CopyTo(ms); | ||
| bodyBytes = ms.ToArray(); |
There was a problem hiding this comment.
Binary responses (images etc) can often be >80KB. Should we be worried about LOH fragmentation in the event of the function being re-used? If so, there's a netstandard version of this package https://github.com/Microsoft/Microsoft.IO.RecyclableMemoryStream coming (or just copy the files) that might help.
There was a problem hiding this comment.
Interesting, curious when that package will support .NET Core
There was a problem hiding this comment.
Shipped earlier today microsoft/Microsoft.IO.RecyclableMemoryStream#43 (comment)
|
I pushed out version 0.10.0-preview1 of |
|
Cool, I'll keep an eye out on progress in CloudFormation and SAM support for the binary mapping, and we can hopefully simplify the interface to this feature in the future. |
|
Regarding the MS pooling, @damianh -- do want to take a crack at incorporating that in? Let me know if not, I think it could be a valuable addition. |
|
@normj -- can I ask, how did you pull in the changes from this PR? I'm trying to pull in all upstream changes into my local fork so that I get all caught up, but git is complaining about merge conflicts with all of the commits associated with this PR, even though there are only about a dozen or so lines that differ. |
|
I don't remember doing anything special. But admittedly I did the merge a while ago and then went on a 2 week vacation:) I did have a quite a few merge conflicts I had to deal with because your pull request came in while I was converting all the projects to the new MSBuild format. Probably something with that messed up the commit history compared your fork. Sorry about that. |
|
I can take a crack. However And compilation |
|
Looks like you are missing the official NuGet feed if you can't find AWSSDK.Kinesis. I am curious how this package improves performance. I'm always leary of adding dependencies so if I have the data showing it improves performance then it makes it easier to decide to take the pull request. |
Fully agreed. Am dependency allergic also. |
I have the official feed registered. Looks like it's choking on the the non-existent path defined in the Anyway, will open a separate PR for this, if it turns out to be desirable. |
| // Figure out how we should treat the response content | ||
| var rcEncoding = DefaultResponseContentEncoding; | ||
| if (contentType != null | ||
| && _responseContentEncodingForContentType.ContainsKey(contentType)) |
There was a problem hiding this comment.
This is problematic for content types that specify a charset. i.e. text/plain; charset=utf8.
Either specify all variations in _responseContentEncodingForContentType dictionary or perhaps ignore the charset altogether _responseContentEncodingForContentType.ContainsKey(contentType.Split(';')[0]) ?
4 participants
As per #74