-
Notifications
You must be signed in to change notification settings - Fork 481
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 happy path for lambda test tool v2 #1934
Conversation
/// </summary> | ||
/// <param name="response">The <see cref="HttpResponse"/> to set the error on.</param> | ||
/// <param name="emulatorMode">The <see cref="ApiGatewayEmulatorMode"/> determining the error format.</param> | ||
private static async Task SetErrorResponse(HttpResponse response, ApiGatewayEmulatorMode emulatorMode) |
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.
this is just refactoring
/// <param name="headers">The single-value headers.</param> | ||
/// <param name="multiValueHeaders">The multi-value headers.</param> | ||
/// <param name="emulatorMode">The <see cref="ApiGatewayEmulatorMode"/> determining the default content type.</param> | ||
private static void SetContentType(HttpResponse response, IDictionary<string, string>? headers, IDictionary<string, IList<string>>? multiValueHeaders, ApiGatewayEmulatorMode emulatorMode) |
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.
refactoring, not anything new
SetResponseHeaders(response, apiResponse.Headers, emulatorMode, apiResponse.MultiValueHeaders); | ||
SetResponseBody(response, apiResponse.Body, apiResponse.IsBase64Encoded); | ||
SetContentTypeAndStatusCodeV1(response, apiResponse.Headers, apiResponse.MultiValueHeaders, apiResponse.StatusCode, emulatorMode); | ||
response.StatusCode = apiResponse.StatusCode; |
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 had to move setting response headers before setting the body in order to fix some errors. see pr description for more details
else if (multiValueHeaders != null && multiValueHeaders.TryGetValue("Content-Type", out var multiValueContentType)) | ||
{ | ||
contentType = multiValueContentType.FirstOrDefault(); | ||
return; |
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.
this is a bug fix. a new integration test discovered an issue where if we had an empty string, the content length was not being set. so to fix it we do write empty string to the body, which will automatically set the content length
return Results.Ok(); | ||
if (response.FunctionError != null) | ||
{ | ||
// TODO: Mimic API Gateway's behavior when Lambda function has an exception during invocation. |
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 will make a backlog sim for this
|
||
private static IAmazonLambda CreateLambdaServiceClient(ApiGatewayRouteConfig routeConfig) | ||
{ | ||
// TODO: Handle routeConfig.Endpoint to null and use the settings versions of runtime. |
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 will make a backlog sim for this
@@ -181,12 +192,50 @@ private bool IsRouteConfigValid(ApiGatewayRouteConfig routeConfig) | |||
/// <returns>An <see cref="ApiGatewayRouteConfig"/> corresponding to Lambda function with an API Gateway HTTP Method and Path.</returns> | |||
public ApiGatewayRouteConfig? GetRouteConfig(string httpMethod, string path) |
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.
@normj what was the reason you had to make changes in this file in ef143c3#diff-e1cef17d258d2f0adc55f27ed60cc0763ee1acd18b66cf11cf77d765c1199003R189? i think i had your change and then removed it and ended up updating this file anyway when i was playing around with things. but i actually dont think changes are needed here. was there a specific scenario that wasn't working for you? all of the test cases pass in the previous version (in feature/lambdatesttool-v2 branch) and my new branch
i can remove the changes from the file then if we want to reduce the amount of code change in this PR, or maybe i can just leave the comments? What do we think?
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 had to have that because I could configure a API Gateway request path to just the root (/
). Without the StringSplitOptions.RemoveEmptyEntries
the return segments would be an array of 1 and that being an empty string. That would fail the match.
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.
gotcha. i will try out that scenario and see what happens
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.
looks like that also fails with my new logic. i will update that to fix this case and also add a test case
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 updated the logic to add a special case for handling the root path. i initially tried your way but that broke some other test cases, so it was easier to handle it with a special case.
I also added a unit test to verify that the root path works and tested it manually. Before my change, the unit test would fail and after my change, the unit test passes
// return "test", it actually comes as "\"test\"" to response. So we need to get the raw string which is what api gateway does. | ||
if (jsonElement.ValueKind == JsonValueKind.String) | ||
{ | ||
response = jsonElement.GetString(); |
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.
this is also a bug fix
@@ -22,26 +22,6 @@ public ApiGatewayResponseExtensionsAdditionalTests(ApiGatewayIntegrationTestFixt | |||
_httpClient = new HttpClient(); | |||
} | |||
|
|||
//[Fact] | |||
//public async Task V2_SetsContentTypeApplicationJsonWhenNoStatusProvided() |
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 forgot to remove this in a previous pr. this is covered by #1927
Issue #, if available: DOTNET-7840
Description of changes:
Headers are read-only, response has already started.
error, because we were setting the content type header after the body was set, so i updated the logic to set the headers first and refactored the logic a bit to make it look better\"stringhere\"
. Previously we were then setting thebody
to the encoded form"\test\"
, but in reality api gateways decodes the json string to juststringhere
Testing Details
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.