Allow for case insensitve header names #398
Conversation
There was a problem hiding this comment.
Thanks for submitting a patch! Generally things look great, just a couple comments on the CaseInsensitiveDict and some tests we should add with this.
Have you run make integ-test and make func-test? We haven't yet enabled those in Travis.
Also, we should be adding at least some functional tests to cover this case. Could you add functional tests to cover thess cases?
| @@ -14,6 +14,18 @@ | |||
| LOG = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| class CaseInsensitiveDict(dict): | |||
There was a problem hiding this comment.
Please add tests for this class. It will help me (and others) understanding the application of this.
There was a problem hiding this comment.
I've added a test in tests/functional/local/apigw/test_service.py
There was a problem hiding this comment.
We should have both unit and functional. We need to make sure we are unit testing everything. The CodeCoverage show branch 22-23 missing and we are not really testing the other paths, it just so happens we execute those paths by creating the object. Ideally, we would mock out this class in the other tests, and then add explicit tests for this class.
There was a problem hiding this comment.
Added unit tests to tests/functional/local/apigw/test_service.py you should now have full code coverage of this class.
samcli/local/apigw/service.py
Outdated
|
|
||
| def __getitem__(self, key): | ||
| try: | ||
| return [v for k, v in self.items() if k.lower() == key.lower()][0] |
There was a problem hiding this comment.
I can't really wrap my brain around what is going on here and seems complex for what it should be doing.
Why not have a __setitem__ where you .lower on insert?
There was a problem hiding this comment.
I originally did that (a __setitem__ method that lowercases). That ended up generating a response with only lowercase headers, which after some thought I decided was a "bigger" change than a minimal change necessary. Since while headers are case-insensitive for comparisons us humans typically look at the case and think that a returned header of "content-type" looks wrong.
So, after some careful consideration realizing that this is only used for local testing, went with find the header as slightly more complicated (it's used only once in the server). While preserving the case of headers.
There was a problem hiding this comment.
That makes sense. I hadn't considered that. I do agree it would be a strange experience if the Header names magically changed in the request.
Can you refactor this to not use a list comprehension though? I am ok with simple ones but this is fairly complex and hard to parse/understand. We favor readability where we can.
There was a problem hiding this comment.
Sorry adding one more thing here that I forgot to mention in the comment above.
Could you also add a comment about why we are only implementing the ‘getitem’? This will help future people not be like, what is this madness. You should totally just use ‘setitem’ with this as well. :)
| @@ -450,6 +502,33 @@ def test_post_binary_with_incorrect_content_type(self): | |||
| self.assertEquals(response.headers.get('Content-Type'), "application/json") | |||
|
|
|||
|
|
|||
| class TestService_CaseInsensiveDict(TestCase): | |||
There was a problem hiding this comment.
Unit tests should go in the tests/unit/local/apigw/test_service.py not in the tests/functional. Can you move this to the correct folder/file?
| self.assertTrue('Content-Type' in data) | ||
| self.assertTrue('CONTENT-TYPE' in data) | ||
| self.assertTrue('Browser' in data) | ||
| self.assertTrue('Dog-Food' not in data) |
There was a problem hiding this comment.
We try and keep unit tests to only one assert as much as possible. There are always exceptions but generally if you are testing two different cases, even if they are related, you should split them out into different tests.
Unit tests are cheep and fast. While this seems very much copy-paste, it leads to better isolated tests in overall. The main advantage here is when you make code changes that impact multiple asserts. When chaining asserts like this in one test, the test will fail when the first assert fails. This gives no indication of the bigger picture and state of your system. Devs following TDD constantly run unit test and rely heavily on this idea. I should be able to run the tests and see/understand everything that went wrong.
You can very easily parameterize this and the following function to achieve this. Here is an example how we are doing this already: https://github.com/awslabs/aws-sam-cli/blob/develop/tests/unit/local/apigw/test_service.py#L520
|
@jfuss Moved the CaseInsensitiveDict test to the unit test directory (didn't see it before). |
|
@koblas No worries. :) |
In testing with a local served files, I noted that Content-Type headers were being compared in a case-sensitive way. Which means that for an express application which is serving headers up in lowercase, sam local was adding a second 'Content-Type: application/json header.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.