-
Notifications
You must be signed in to change notification settings - Fork 478
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
feat: add api gateway emulator skeleton #1902
feat: add api gateway emulator skeleton #1902
Conversation
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Commands/RunCommand.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Commands/CancellableAsyncCommand.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/IApiGatewayRouteConfigService.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Show resolved
Hide resolved
can you double check the line file endings. i was getting warnings in my ide that they were not set as CRLF |
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Constants.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Outdated
Show resolved
Hide resolved
Since we are working on different platforms we're bound to run into some line ending issues. I think what we did with the deploy tool was update our git config to retrieve files as LF. (something like that) |
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Show resolved
Hide resolved
foreach (var routeConfig in _routeConfigs) | ||
{ | ||
_logger.LogDebug("Checking if '{Path}' matches '{Template}'.", path, routeConfig.Path); | ||
var template = TemplateParser.Parse(routeConfig.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.
I don't see any tests for handling API Gateway's style of using {proxy+}
to indicate wild card character. I really want to know if Microsoft's TemplateParser
is handling that or if we need to roll our own solution. Have you done any side by side comparisons with API Gateway and this library to see if they resolve the same.
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.
TemplateParser supports a catch-all variable similar to the greedy variable in API gateway. I have updated the implementation to account for it as well as add sorting based on the route priority logic from API Gateway. I have added comments to explain everything in the code.
return null; | ||
} | ||
|
||
private RouteValueDictionary GetDefaults(RouteTemplate parsedTemplate) |
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.
Are defaults even a think in API Gateway routing? I thought with API Gateway routing you can just have the name of the variable. I could be wrong but be sure we are not just implementing a feature of the Microsoft library and not 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.
You are right. The function was not doing anything for our use case. I removed it.
|
||
namespace Amazon.Lambda.TestTool.UnitTests.Services; | ||
|
||
public class ApiGatewayRouteConfigServiceTests |
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.
You need tests with {proxy+}
especially when there are other routes. And you need tests with some overlap in the routes. For example a test with routeA -> /foo/{value}
and routeB -> /foo/bar
and you test with requests to /foo
, /foo/bar
and /foo/fred
. Similar idea with {proxy+}
and make sure we mimic API Gateway's behavior when choosing to use the proxy endpoint versus a more direct path. Another example routeA /{proxy+}
and routeB /phil
. Requests going to /norm
and /phil
and /phil/dotnet
. Don't expect the Microsoft template library to match API Gateway's behavior.
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 have updated the implementation to account for it as well as add sorting based on the route priority logic from API Gateway. I have added comments to explain everything in the code.
I also added 1 big test that lists a bunch of different routes with overlap and tests which function was matched. This should cover everything.
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Processes/ApiGatewayEmulatorProcess.cs
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
[Fact] | ||
public void ProperlySortRouteConfigs() |
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.
Do you verify with an API Gateway deployed with routes like this that returns behaves the same?
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.
Yes the values I have in the test have been verified in API Gateway
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Models/ApiGatewayRouteType.cs
Outdated
Show resolved
Hide resolved
Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Services/ApiGatewayRouteConfigService.cs
Show resolved
Hide resolved
} | ||
} | ||
|
||
// If there are leftover route segments |
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 explain what would be an example route and request path that would go into this section. It would be good to add examples in the comment block to help readers. I'm struggling to read this and understand in what scenario would the route ever be a matched if there are more route segments then were in the request 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.
I simplified this code paths as it was redundant. This was a leftover from my tweaking when I was working on it. I also added comments and examples explaining the code paths as much as possible.
} | ||
|
||
// If request has leftover segments that aren't matched | ||
if (matched && requestPathIndex < requestSegments.Length) |
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.
If the variableCount
doesn't match the variables found in the request path should that be an false 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.
If we enter these code paths, then matched will be false and variableCount won't matter.
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 ask that you change some LogDebug
to LogError
because they represent invalid user data. Other then that I'm good. I don't need to rereview for the LogError
change.
Issue #, if available:
DOTNET-7837
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.