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

solves an issue when using path parameters and policy based resource access limits #1633

Closed
wants to merge 0 commits into from

Conversation

zoellner
Copy link
Contributor

@zoellner zoellner commented Dec 21, 2022

Description

This is a reintroduction of #1283 to fix bug introduced in 5ff81fb

Now based on latest master with tests passing.

It solves an issue when using path parameters and policy based resource access limits.

Motivation and Context

Fixes #1043
Fixes #1586

How Has This Been Tested?

used in our own application. working again as before the bug was introduced

Screenshots (if appropriate):

@zoellner zoellner changed the title This solves an issue when using path parameters and policy based resource access limits Dec 21, 2022
@zoellner
Copy link
Contributor Author

@dherault there are two tests failing but I am not sure what the tests are supposed to test and have a feeling they might have been put in place based on previous responses without any real requirement for that outcome.
e.g. this one:

    {
      description: 'event.resource should not contain wildcards',
      expected: '/{id}/test-resource-variable-handler',
      path: '/1/test-resource-variable-handler',
      status: 200,
    }

it's not clear to me why {id} would be expected in the path

@dnalborczyk
Copy link
Collaborator

thank you for the PR @zoellner

there are two tests failing but I am not sure what the tests are supposed to test and have a feeling they might have been put in place based on previous responses without any real requirement for that outcome. e.g. this one:

I looked at those failing tests briefly, incl. deployment, and it seems they behave as expected. only thing is that those don't belong in the handler tests section, as those are solely testing the different handler implementations (e.g. callback, context.done, Promises/async functions).

that said, I might be mistaken, what do you expect the outcome of those tests to be?

@zoellner
Copy link
Contributor Author

I would expect the same as I'm getting when logging the event in AWS Lambda and there I see methodArn and path having path parameters replaced but resource not
and in the requestContext: resroucePath without replacement, path with replacement

a logged request from AWS Lambda, adjusted to this example and sanitized

{
    "type": "REQUEST",
    "methodArn": "arn:aws:execute-api:region:accountId:gatewayid/STAGE/GET/1/test-resource-variable-handler",
    "resource": "/{id}/test-resource-variable-handler",
    "path": "/prefix/1/test-resource-variable-handler",
  ...
     "requestContext": {
        
        "resourcePath": "/{id}/test-resource-variable-handler",
        "httpMethod": "GET",
        "path": "/prefix/1/test-resource-variable-handler",
        ...
    }
}

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Dec 29, 2022

I looked at this failing test as a start: https://github.com/dherault/serverless-offline/actions/runs/3785736032/jobs/6436014680

1) star routes
       when a catch all route is used in a rest api
         it should return a payload:

      AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

{
  path: '/hello',
  resource: '/hello'
}

should loosely deep-equal

{
  path: '/hello',
  resource: '/{proxy+}'
}
      + expected - actual

       {
         "path": "/hello"
      -  "resource": "/hello"
      +  "resource": "/{proxy+}"
       }
      
      at Context.<anonymous> (file:///home/runner/work/serverless-offline/serverless-offline/tests/end-to-end/starRoutesRestApi/starRoutesRestApi.test.js:24:14)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

I went to: /tests/end-to-end/starRoutesRestApi/src

and deployed with npx serverless deploy

then fetched the endpoint: https://xxxxxxx.execute-api.us-east-1.amazonaws.com/dev/foo

the response:

{
  "path": "/foo",
  "resource": "/{proxy+}"
}

then I started the local process with npx serverless offline start

fetched http://localhost:3000/dev/foo

the response:

{
  "path": "/foo",
  "resource": "/{proxy+}"
}

there are two tests failing but I am not sure what the tests are supposed to test

in this case it seems just the path and the resource of the event object:

export const hello = async (event) => {

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