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

[MRG] Do not set Referer by default when its value is None #475

Merged
merged 1 commit into from Dec 24, 2013

Conversation

@dangra
Copy link
Member

@dangra dangra commented Nov 25, 2013

Another implementation for #473, alternative #474.

closes #473.

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Dec 2, 2013

I like more this approach than #474

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Dec 2, 2013

Me too. How about a test? :)

@redapple
Copy link
Contributor

@redapple redapple commented Dec 16, 2013

Well, @dangra knows where to throw in his magic ;-) I had no clue in #474

@dangra
Copy link
Member Author

@dangra dangra commented Dec 17, 2013

please review.

headers={'X-Set': '1', 'X-False': '', 'X-Unset': None})
spider = SingleRequestSpider(seed=req)
yield docrawl(spider)
self.assertTrue('response' in spider.meta)

This comment has been minimized.

@kmike

kmike Dec 17, 2013
Member

We can use self.assertIn because we dropped 2.6 support. I believe the advantage is a nicer failure message.

This comment has been minimized.

@dangra

dangra Dec 17, 2013
Author Member

addressed by dangra@3e84c70

@defer.inlineCallbacks
def test_request_headers(self):
req = Request('http://localhost:8998/echo?headers=1&body=0',
headers={'X-Set': '1', 'X-False': '', 'X-Unset': None})

This comment has been minimized.

@kmike

kmike Dec 17, 2013
Member

This test doesn't check that passing None unsets a header (main motivation for #473) because X-Unset header is not set.

This comment has been minimized.

@dangra

dangra Dec 17, 2013
Author Member

fair point, although the real issue with #473 is that Referrer is set using setdefault()

This comment has been minimized.

@dangra

dangra Dec 17, 2013
Author Member

addressed by dangra@f7aa0dd

@dangra
Copy link
Member Author

@dangra dangra commented Dec 17, 2013

I appreciate if someone else can provide some documentation wording for this feature before it's merged.

@dangra
Copy link
Member Author

@dangra dangra commented Dec 18, 2013

Looks good to anyone? I'll merge if noone chime in :)

@redapple
Copy link
Contributor

@redapple redapple commented Dec 18, 2013

Checked network traffic with Wireshark.
It works as expected for the "Referer" header for example.

@kmike
Copy link
Member

@kmike kmike commented Dec 18, 2013

I'd prefer an even more "functional-style" test - e.g. check that Referer header is not set when None is used and is set if None is not used (no explicit Referer header and explicit empty Referer header). Checking for setdefault looks like checking against implementation details.

As for the docs, it seems that "unset" would be a bit misleading - we can't unset a header in Request constructor because the header can be added later by a middleware, and the feature should still work. It is more like "preventing header from being sent".

@dangra
Copy link
Member Author

@dangra dangra commented Dec 19, 2013

@kmike: you're right, I'll add a test for Referer header and move the unittest to scrapy.tests.test_http_headers.

@dangra
Copy link
Member Author

@dangra dangra commented Dec 24, 2013

@kmike : review latest changes to tests cases.

# start requests doesn't set Referer header
echo0 = json.loads(spider.meta['responses'][2].body)
self.assertNotIn('Referer', echo0['headers'])
# following request set Referer to start request url

This comment has been minimized.

@kmike

kmike Dec 24, 2013
Member

sets

# next request avoids Referer header
echo2 = json.loads(spider.meta['responses'][2].body)
self.assertNotIn('Referer', echo2['headers'])
# last request explicitly set a Referer header

This comment has been minimized.

@kmike

kmike Dec 24, 2013
Member

sets

@kmike
Copy link
Member

@kmike kmike commented Dec 24, 2013

Looks good!

dangra added a commit that referenced this pull request Dec 24, 2013
[MRG] Do not set Referer by default when its value is None
@dangra dangra merged commit e910255 into scrapy:master Dec 24, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@dangra dangra deleted the dangra:473-do-not-send-header branch Dec 24, 2013
pablohoffman added a commit that referenced this pull request Dec 24, 2013
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.

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