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

Added content-type check as per issue #193 #660

Merged
merged 2 commits into from Mar 24, 2014
Merged

Conversation

@rubenvereecken
Copy link
Contributor

@rubenvereecken rubenvereecken commented Mar 19, 2014

I wanted to get my hands dirty and carefully read #193.

Httpmiddleware is now changed in that it does not attempt to decode anything with application/x-gzip or application/gzip as Content-Type. These response are just passed through and both their Content-Type and Content-Encoding are left untouched.

@kmike
Copy link
Member

@kmike kmike commented Mar 20, 2014

Thanks, the fix makes sense!
Could you please add a test for it?

self.assertEqual(response.headers['Content-Type'], 'application/gzip')

newresponse = self.mw.process_response(request, response, self.spider)
self.assertIs(newresponse, response)

This comment has been minimized.

@rubenvereecken

rubenvereecken Mar 20, 2014
Author Contributor

The response should be left untouched with Content-Type set to 'application/gzip'


newresponse = self.mw.process_response(request, response, self.spider)
self.assertIs(newresponse, response)
self.assertIn('Content-Encoding', newresponse.headers)

This comment has been minimized.

@rubenvereecken

rubenvereecken Mar 20, 2014
Author Contributor

Content-Encoding should be left untouched.

@rubenvereecken
Copy link
Contributor Author

@rubenvereecken rubenvereecken commented Mar 20, 2014

Alright I wasn't sure whether this deserved its own test, I like the thoroughness!
The test is explained in the file comments

All tests pass.

request = response.request

self.assertEqual(response.headers['Content-Encoding'], 'gzip')
self.assertEqual(response.headers['Content-Type'], 'application/gzip')

This comment has been minimized.

@dangra

dangra Mar 21, 2014
Member

I think Content-Encoding and Content-Type assertions must happen after the response is processed by process_response hook.

@rubenvereecken
Copy link
Contributor Author

@rubenvereecken rubenvereecken commented Mar 21, 2014

It took a while for the new commit to arrive, but it has! @dangra was right about having the Content-Type and Content-Encoding checks happen after the processing. I tried following the working of other tests as closely as possible though and all should be good now.

dangra added a commit that referenced this pull request Mar 24, 2014
Added content-type check as per issue #193
@dangra dangra merged commit f687455 into scrapy:master Mar 24, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@dangra
Copy link
Member

@dangra dangra commented Mar 24, 2014

thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.