Skip to content
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

Adds non greedy parameter values into the path when {proxy+} #1403

Conversation

christostatitzikidis
Copy link
Contributor

@christostatitzikidis christostatitzikidis commented Dec 24, 2022

Issue #, if available:
#1233

A confirmed bug since July, that has not been fixed yet, and has taken me a whole day until I realised what the problem is.
Basically, when {proxy+}, the values of the non-greedy parameters are not picked up from the request path.

So the following scenario fails:

Given an API Gateway resource set up as /path/{foo}/bar/{proxy+}
When I make a request to path /path/afoo/bar/subpath
Then the foo variable in my controller action should be equal to afoo

What actually happens is that my controller's foo variable contains the value {foo}

Description of changes:
Added code that will actually replace the non-greedy parameters with their values in the path.
In order to keep changes as small as possible, I just added a loop, but I think it would be better optimised if the path was replaced straight away with the incoming request's path - that's already what happens with non-proxy requests ((line 176). I added a comment about that.

The pull request refers to lines 156-177 of ApiGatewayProxyFunction.cs

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for this bug fix. We also need some tests for this fix. Tests for this project are here and are relatively easy to add. Basically just create your sample event json and a corresponding endpoint in the test project.

@@ -154,12 +154,21 @@ protected override void MarshallRequest(InvokeFeatures features, APIGatewayProxy
requestFeatures.Method = apiGatewayRequest.HttpMethod;

string path = null;

// Replaces {proxy+} in path, if exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should always be doing this logic regardless over the being a {proxy+}. It is possible to use this package and not use {proxy+} and use path parameters..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point actually. Let me double check the case and I'll update the PR, if required.

As far as this PR goes, we have been using a custom build of the package across multiple APIs, with the changes in the PR since December 24th, and it all works as expected (cases are normal proxy resources without non-greedy parameters, non-proxy resources without non-greedy parameters and proxy resources with non-greedy parameters).

I have found that replacing the logic in lines 156-171 with string path = apiGatewayRequest.Path; at line 156 causes some tests to fail, so for now, it's easier/safer to use the logic in this PR, in my opinion, as all tests succeed, and it's been tested on (our) production for a good while, without issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked and we are actually also using non-greedy parameters in non-proxy resource paths in our custom deployments that contain this PR.
This works just fine because the path does never get in the {proxy+} logic (line 157 on) and gets replaced properly by line 175

if (string.IsNullOrEmpty(path))
{
    path = apiGatewayRequest.Path;
}

I have added an additional test to cover this case, and it passes without any modifications to the PR

@normj normj changed the base branch from master to dev February 12, 2023 02:43
@normj normj merged commit b0951c1 into aws:dev Feb 12, 2023
@normj
Copy link
Member

normj commented Feb 13, 2023

Thanks for the PR. The PR has been released as part of version 8.0.0 of Amazon.Lambda.AspNetCoreServer. This was a major version bump to PR #899 shipped in this release which is a slight breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants