Auto-decode the HTTP request body based on the Content-Encoding HTTP header

[Lucas-C]

Which issue(s) does this change fix?

This was not reported so far, based on a quick research among this repository issues.

Why is this change necessary?

Currently AWS SAM does not replicate API Gateway behaviour of auto-decoding a gzip-ed payload.

How does it address the issue?

This PR ensures that the Flask request.stream is decoded with the gzip standard Python module if the request HTTP header Content-Encoding is gzip

What side effects does this change have?

None

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests

Note : I have only added unit tests so far. I can add integration tests upon request.

• Write/update functional tests if needed
• make pr passes

Note : I'm not sure how best to fix this Pylint check:

$ pylint --rcfile .pylintrc samcli/local/apigw/local_apigw_service.py
************* Module samcli.local.apigw.local_apigw_service
samcli/local/apigw/local_apigw_service.py:275:4: R0912: Too many branches (13/12) (too-many-branches)

• make update-reproducible-reqs if dependencies were changed
• Write documentation

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

[Lucas-C]

A small note about a minor refactoring included this PR: I removed the get_request_methods_endpoints static method.

Why? Because I had to make use of request.headers, and hence had a few options:

• add a new get_request_headers static method (which IMHO would have slightly increased the code complexity)
• rename & alter get_request_methods_endpoints to also extract the headers
• get rid of get_request_methods_endpoints and directly access Flask global request object, but make so that the existing unit tests still pass

I picked this last option, as it was easy to mock Flask global request in the tests through the use of @patch("samcli.local.apigw.local_apigw_service.request")

[hawflau]

Thanks for the contribution!. Seems there's a pylint violation causing one of the check failures. Could you please fix that first?

pylint --rcfile .pylintrc samcli
************* Module samcli.local.apigw.local_apigw_service
samcli/local/apigw/local_apigw_service.py:275:4: R0912: Too many branches (13/12) (too-many-branches)

I'll continue review the remaining of the PR.

[Lucas-C]

Hi @hawflau

Thank you for your comment.
I had already mentioned that failing Pylint check in the issue description,
but I'm not sure how to be best fix it: shoud I split the function in 2 parts?

[hawflau]

Hi @Lucas-C yeah, splitting the function into two parts makes sense. The simplest you can do is to just factor out the part you added into a new method

[Lucas-C]

@hawflau: thank you for your answer.
OK, I've just done that.

[Lucas-C]

The make-pr job does not pass, but I do not understand why...
The following error gets repeated, but that does not happen in other jobs nor on my computer:

name = 'request'

    def _lookup_req_object(name):
        top = _request_ctx_stack.top
        if top is None:
>           raise RuntimeError(_request_ctx_err_msg)
E           RuntimeError: Working outside of request context.
E           
E           This typically means that you attempted to use functionality that needed
E           an active HTTP request.  Consult the documentation on testing for
E           information about how to avoid this problem.

On my computer make pr runs fine, it just produces FAIL Required test coverage of 95% not reached. Total coverage: 94.99%.

Could you provide some advice on this please?

[Lucas-C]

I see that the maintainer/need-followup label has been added,
so I'm going to wait for an answer from maintainers 😊

[jfuss]

@Lucas-C I checked on of the jobs runs and looks like it just formatting (at least for that run). Could you update the PR with the head of develop and ran make black? That should get everything up to date and get rid of the formatting issue.

I am going through our PR backlog and adding ones to review to my list. Will add this one to my list and targeting to have reviews out by end of the week for ones I assign to myself.

[Lucas-C]

@Lucas-C I checked on of the jobs runs and looks like it just formatting (at least for that run). Could you update the PR with the head of develop and ran make black? That should get everything up to date and get rid of the formatting issue.

I did jut that yesterday :)

[jfuss]

@Lucas-C I see the failures you posted above now. I remember having some troubles with this a while back and think that is why we mocked out get_request_methods_endpoints in the way we did. Looks like you changed that.

Doing some quick googling, https://stackoverflow.com/questions/17375340/testing-code-that-requires-a-flask-app-or-request-context might be helpful. Basically what is happening there is some context that flask (or WSGI) needs and it's not setup correctly within the test causing the failures. We could try to dig deeper into the "right" way to do this in the test or just mock out similar to how we did previous. It is weird that this only happens on the CI system though.

[Lucas-C]

Basically what is happening there is some context that flask (or WSGI) needs and it's not setup correctly within the test causing the failures. We could try to dig deeper into the "right" way to do this in the test or just mock out similar to how we did previous. It is weird that this only happens on the CI system though.

Yeah, it's weird, tests pass fine on my computer.
I think it may be due to multi-threading or multi-processing being used at some point (pytest-forked maybe?)

I found a way to solve the problem in the CI pipeline by using Flask-Testing, which I added as a dependency in dev.txt.
With this additional fix, do you think this PR could be merged?

[mndeveci]

Thanks for fixing the issues @Lucas-C. The only worry that I have is the library that you've added. It seems it is not maintained anymore (last commit from 2020). Can we somehow resolve this issue by overriding TestCase or is there another library that we can use?

[Lucas-C]

Thanks for fixing the issues @Lucas-C. The only worry that I have is the library that you've added. It seems it is not maintained anymore (last commit from 2020). Can we somehow resolve this issue by overriding TestCase or is there another library that we can use?

OK, I understand if you don't want to introduce this library as a dependency.

Using it solved an issue that only appears in the CI pipeline, so it is quite annoying to debug...
I'm sorry but I'm getting a bit tired of fighting with the tests, whereas my initial patch was relatively simple.

I have already spent quite a few hours on this PR, and asked for help on how to handle those failing tests 2 months ago, without any practical solution being suggested so far... And the solution I came with is apparently not satisfying.

Could the maintainers of this project please give some assistance here?
Should we look into FlaskTesting to understand why it solved the issue, and import their workaround into aws-sam-cli unit tests? I do not want to go into this direction (or another) and then be told that's not an acceptable solution...
What do you recommend, and could you provide code snippets to help please?

I'm a bit sad if this PR does not get merged after the time I spent on it, but I don't to fall for the sunk cost fallacy 😊

[mndeveci]

@Lucas-C thank you very much for your efforts, we really appreciate the community contributions for this project! I feel your pain and I am doing my best to help you fix the issues and merge this PR.

I've fixed unit test issues and removed that library but I can't push to your fork. Can you give me write access to your fork so that I can push my updates?

Thanks!

[Lucas-C]

Sure, I gave you write permission on it @mndeveci

[mndeveci]

LGTM! Thanks a lot for your contribution 🥳

[moelasmar]

Could you please include an integration test cases to cover your changes!!

[hawflau]

The make-pr run at Appveyor (https://ci.appveyor.com/project/AWSSAMCLI/aws-sam-cli/builds/45835621) failed for Py38 and Py39. Only succeeded for Py37.

Error logs:

=================================== FAILURES ===================================
_______ TestApiGatewayService.test_request_handler_with_gzipped_body_ok ________
self = <tests.unit.local.apigw.test_local_apigw_service.TestApiGatewayService testMethod=test_request_handler_with_gzipped_body_ok>
request_mock = <AsyncMock name='request' id='139925277621792'>
    @patch("samcli.local.apigw.local_apigw_service.request")
    def test_request_handler_with_gzipped_body_ok(self, request_mock):
        self.api_service._get_current_route = MagicMock()
        self.api_service._get_current_route.return_value = self.api_gateway_route
    
        parse_output_mock = Mock()
        parse_output_mock.return_value = (200, Headers({}), "body")
        self.api_service._parse_v1_payload_format_lambda_output = parse_output_mock
    
        request_mock.endpoint = "test"
        request_mock.host = request_mock.remote_addr = "test"
        request_mock.method = "POST"
        request_mock.path = "/"
        request_mock.scheme = "http"
        request_mock.environ = request_mock.view_args = {}
        request_mock.headers = Headers({"Content-Encoding": "gzip"})
        request_mock.stream = io.BytesIO(gzip.compress(b"Hello world!"))
        request_mock.get_data = lambda: request_mock.stream.read()  # replicates werkzeug.wrappers.Request.get_data
    
>       result = self.api_service._request_handler()
tests/unit/local/apigw/test_local_apigw_service.py:468: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
samcli/local/apigw/local_apigw_service.py:337: in _request_handler
    event = self._construct_v_1_0_event(
samcli/local/apigw/local_apigw_service.py:703: in _construct_v_1_0_event
    query_string_dict, multi_value_query_string_dict = LocalApigwService._query_string_params(flask_request)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
flask_request = <AsyncMock name='request' id='139925277621792'>
    @staticmethod
    def _query_string_params(flask_request):
        """
        Constructs an APIGW equivalent query string dictionary
    
        Parameters
        ----------
        flask_request request
            Request from Flask
    
        Returns dict (str: str), dict (str: list of str)
        -------
            Empty dict if no query params where in the request otherwise returns a dictionary of key to value
    
        """
        query_string_dict = {}
        multi_value_query_string_dict = {}
    
        # Flask returns an ImmutableMultiDict so convert to a dictionary that becomes
        # a dict(str: list) then iterate over
>       for query_string_key, query_string_list in flask_request.args.lists():
E       TypeError: 'coroutine' object is not iterable
samcli/local/apigw/local_apigw_service.py:829: TypeError
----------------------------- Captured stderr call -----------------------------
Exception ignored in: <coroutine object AsyncMockMixin._execute_mock_call at 0x7f42e0eb4640>
Traceback (most recent call last):
  File "/home/appveyor/.localpython3.9.15/lib/python3.9/warnings.py", line 506, in _warn_unawaited_coroutine
    warn(msg, category=RuntimeWarning, stacklevel=2, source=coro)
RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited

@Lucas-C do you know what the reason could be?

[jfuss]

Closing as this looks stale. If you are still interested in contributing, feel free to re-open or create a new PR.