-
Notifications
You must be signed in to change notification settings - Fork 475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ApiGatewayHttpApiV2ProxyRequestTranslator and ApiGatewayProxyRequestTranslator #1901
base: asmarp/api-gateway-emulator-skeleton
Are you sure you want to change the base?
Add ApiGatewayHttpApiV2ProxyRequestTranslator and ApiGatewayProxyRequestTranslator #1901
Conversation
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayHttpApiV2ProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayProxyRequestTranslator.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/ApiGatewayRequestTranslatorFactory.cs
Outdated
Show resolved
Hide resolved
471264d
to
2e94f8b
Compare
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/IRouteConfigurationParser.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/HttpContextExtensions.cs
Outdated
Show resolved
Hide resolved
f996e5b
to
77f7679
Compare
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Extensions/HttpContextExtensions.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Extensions/ExceptionExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any tests for special characters in headers, resource paths or query strings. Have we done a comparison yet with API Gateway? We should have tests for those cases. That includes the RawPath
property as well.
so i did some tests of checking the existing functionality of api gateway in production, with special characters in the header by
|
if (string.IsNullOrEmpty(contentType)) | ||
return false; | ||
|
||
return contentType.StartsWith("image/", StringComparison.OrdinalIgnoreCase) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this list of content types we use for response in the ASP.NET Core bridge library. https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.AspNetCoreServer/AbstractAspNetCoreFunction.cs#L59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also think about how this can be configurable. Probably via environment variables that we can have users configure in Aspire.
Headers = headers, | ||
QueryStringParameters = queryStringParameters, | ||
PathParameters = pathParameters ?? new Dictionary<string, string>(), | ||
Body = HttpRequestUtility.ReadRequestBody(request), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take note that when we get these ToAPIGatewayXXX() methods hooked up to the main code path that is asking for the APIGatewayHttpApiV2ProxyRequest
. When we convert the APIGatewayHttpApiV2ProxyRequest
to the MemoryStream to Lambda we should check the size of the stream is no more then 6MB. If so throw back the same error that users would get from API Gateway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i will connect with phil on where he wants to do this logic at
MultiValueHeaders = multiValueHeaders, | ||
QueryStringParameters = queryStringParameters, | ||
MultiValueQueryStringParameters = multiValueQueryStringParameters, | ||
PathParameters = pathParameters ?? new Dictionary<string, string>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we initializing this to an empty collection if we don't have any. Is this what API Gateway does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. i just did a test
Invoke-WebRequest -Uri https://myapi-gateway-url.amazonaws.com/testfunction -Method GET
where it echos the api gateway request object and i see the value is null
when there are no path parameters.
pathParameters: null,
stageVariables: null,
body: null,
isBase64Encoded: false
}
i will update the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill also double check the query string and header default values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive updated the logic to handle this now
|
||
foreach (var header in headers) | ||
{ | ||
singleValueHeaders[header.Key] = header.Value.Last() ?? ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memory for API Gateway is that in HTTP V2 where there is just a single headers collection the headers are comma delimited. But we need to verify that. I'm sure it isn't just get the last value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah you are right on this https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html
Format 2.0 doesn't have multiValueHeaders or multiValueQueryStringParameters fields. Duplicate headers are combined with commas and included in the headers field. Duplicate query strings are combined with commas and included in the queryStringParameters field.
i think i got confused when reading the other docs for v1 https://aws.amazon.com/blogs/compute/support-for-multi-value-parameters-in-amazon-api-gateway/
Before this change, API Gateway used to retain only the last value and drop everything else for a multi-valued parameter. You can see the original behavior in the queryStringParameters parameter in the above input, where only the “fish” value is retained.
I will update accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive updated the logic to handle this now. this logic will still just return the dicts as-is but then i have the caller perform the comma separated merging.
|
||
foreach (var param in query) | ||
{ | ||
singleValueParams[param.Key] = param.Value.Last() ?? ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above for ExtractHeaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive updated the logic to handle this now
@gcbeattyAWS Also check more characters then aws-lambda-dotnet/Libraries/src/Amazon.Lambda.AspNetCoreServer/Internal/Utilities.cs Line 148 in 3596a58
|
so i have updated the logic based on my findings:1
i have added test cases to check all of these scenarios |
|
||
if (request.Cookies.Any()) | ||
{ | ||
httpApiV2ProxyRequest.Cookies = request.Cookies.Select(c => $"{c.Key}={c.Value}").ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headers and cookies are just sent as-is (from my testing)
DOTNET-7838
Description of changes:
Testing details
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.