From e667ca76820a53ac3abf34604fc284761f936bb9 Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Fri, 24 May 2019 21:45:53 +0900 Subject: [PATCH 1/8] Account for mangling when serializing requests with private callbacks --- scrapy/utils/reqser.py | 6 +++++- tests/test_utils_reqser.py | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index 959dddbd5..8c99763cf 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -75,7 +75,11 @@ def _find_method(obj, func): pass else: if func_self is obj: - return six.get_method_function(func).__name__ + name = six.get_method_function(func).__name__ + if name.startswith('__'): + classname = obj.__class__.__name__.lstrip('_') + name = '_%s%s' % (classname, name) + return name raise ValueError("Function %s is not a method of: %s" % (func, obj)) diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index dcc070b8f..f7191fcef 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -68,6 +68,12 @@ class RequestSerializationTest(unittest.TestCase): errback=self.spider.handle_error) self._assert_serializes_ok(r, spider=self.spider) + def test_private_callback_serialization(self): + r = Request("http://www.example.com", + callback=self.spider._TestSpider__parse_item_private, + errback=self.spider.handle_error) + self._assert_serializes_ok(r, spider=self.spider) + def test_unserializable_callback1(self): r = Request("http://www.example.com", callback=lambda x: x) self.assertRaises(ValueError, request_to_dict, r) @@ -87,6 +93,9 @@ class TestSpider(Spider): def handle_error(self, failure): pass + def __parse_item_private(self, response): + pass + class CustomRequest(Request): pass From 144afcee7973ab97d6c8d89fec007046cc878e3d Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Sat, 25 May 2019 00:52:00 +0900 Subject: [PATCH 2/8] Use regex to check for private methods --- scrapy/utils/reqser.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index 8c99763cf..07c51aaff 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -2,12 +2,16 @@ Helper functions for serializing (and deserializing) requests. """ import six +import re from scrapy.http import Request from scrapy.utils.python import to_unicode, to_native_str from scrapy.utils.misc import load_object +private_name_regex = re.compile('^__[^_](.*[^_])?_?$') + + def request_to_dict(request, spider=None): """Convert Request object to a dict. @@ -76,7 +80,7 @@ def _find_method(obj, func): else: if func_self is obj: name = six.get_method_function(func).__name__ - if name.startswith('__'): + if private_name_regex.search(name): classname = obj.__class__.__name__.lstrip('_') name = '_%s%s' % (classname, name) return name From 72b7d3e90ac2d21ffdd0c44878ec1a5a5d0fa5ce Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Mon, 27 May 2019 23:30:23 +0900 Subject: [PATCH 3/8] Make the regex align to the spec better; add unit tests for name variations --- scrapy/utils/reqser.py | 2 +- tests/test_utils_reqser.py | 24 +++++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index 07c51aaff..04665a2d4 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -9,7 +9,7 @@ from scrapy.utils.python import to_unicode, to_native_str from scrapy.utils.misc import load_object -private_name_regex = re.compile('^__[^_](.*[^_])?_?$') +private_name_regex = re.compile('^__.*[^_]_?$') def request_to_dict(request, spider=None): diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index f7191fcef..b49450ac5 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -3,7 +3,7 @@ import unittest from scrapy.http import Request, FormRequest from scrapy.spiders import Spider -from scrapy.utils.reqser import request_to_dict, request_from_dict +from scrapy.utils.reqser import request_to_dict, request_from_dict, private_name_regex class RequestSerializationTest(unittest.TestCase): @@ -74,6 +74,28 @@ class RequestSerializationTest(unittest.TestCase): errback=self.spider.handle_error) self._assert_serializes_ok(r, spider=self.spider) + def test_private_callback_name_matching(self): + self.assertTrue(private_name_regex.search('__a')) + self.assertTrue(private_name_regex.search('__a_')) + self.assertTrue(private_name_regex.search('__a_a')) + self.assertTrue(private_name_regex.search('__a_a_')) + self.assertTrue(private_name_regex.search('__a__a')) + self.assertTrue(private_name_regex.search('__a__a_')) + self.assertTrue(private_name_regex.search('__a___a')) + self.assertTrue(private_name_regex.search('__a___a_')) + self.assertTrue(private_name_regex.search('___a')) + self.assertTrue(private_name_regex.search('___a_')) + self.assertTrue(private_name_regex.search('___a_a')) + self.assertTrue(private_name_regex.search('___a_a_')) + self.assertTrue(private_name_regex.search('____a_a_')) + + self.assertFalse(private_name_regex.search('_a')) + self.assertFalse(private_name_regex.search('_a_')) + self.assertFalse(private_name_regex.search('__a__')) + self.assertFalse(private_name_regex.search('__')) + self.assertFalse(private_name_regex.search('___')) + self.assertFalse(private_name_regex.search('____')) + def test_unserializable_callback1(self): r = Request("http://www.example.com", callback=lambda x: x) self.assertRaises(ValueError, request_to_dict, r) From 9af91a26b035a10e9303227ad9ddd5e043725514 Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Tue, 28 May 2019 01:40:26 +0900 Subject: [PATCH 4/8] Replace regex usage --- scrapy/utils/reqser.py | 10 +++++----- tests/test_utils_reqser.py | 40 +++++++++++++++++++------------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index 04665a2d4..40223661f 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -2,16 +2,12 @@ Helper functions for serializing (and deserializing) requests. """ import six -import re from scrapy.http import Request from scrapy.utils.python import to_unicode, to_native_str from scrapy.utils.misc import load_object -private_name_regex = re.compile('^__.*[^_]_?$') - - def request_to_dict(request, spider=None): """Convert Request object to a dict. @@ -71,6 +67,10 @@ def request_from_dict(d, spider=None): flags=d.get('flags')) +def _is_private_method(name): + return name.startswith('__') and not name.endswith('__') + + def _find_method(obj, func): if obj: try: @@ -80,7 +80,7 @@ def _find_method(obj, func): else: if func_self is obj: name = six.get_method_function(func).__name__ - if private_name_regex.search(name): + if _is_private_method(name): classname = obj.__class__.__name__.lstrip('_') name = '_%s%s' % (classname, name) return name diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index b49450ac5..fad5b6003 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -3,7 +3,7 @@ import unittest from scrapy.http import Request, FormRequest from scrapy.spiders import Spider -from scrapy.utils.reqser import request_to_dict, request_from_dict, private_name_regex +from scrapy.utils.reqser import request_to_dict, request_from_dict, _is_private_method class RequestSerializationTest(unittest.TestCase): @@ -75,26 +75,26 @@ class RequestSerializationTest(unittest.TestCase): self._assert_serializes_ok(r, spider=self.spider) def test_private_callback_name_matching(self): - self.assertTrue(private_name_regex.search('__a')) - self.assertTrue(private_name_regex.search('__a_')) - self.assertTrue(private_name_regex.search('__a_a')) - self.assertTrue(private_name_regex.search('__a_a_')) - self.assertTrue(private_name_regex.search('__a__a')) - self.assertTrue(private_name_regex.search('__a__a_')) - self.assertTrue(private_name_regex.search('__a___a')) - self.assertTrue(private_name_regex.search('__a___a_')) - self.assertTrue(private_name_regex.search('___a')) - self.assertTrue(private_name_regex.search('___a_')) - self.assertTrue(private_name_regex.search('___a_a')) - self.assertTrue(private_name_regex.search('___a_a_')) - self.assertTrue(private_name_regex.search('____a_a_')) + self.assertTrue(_is_private_method('__a')) + self.assertTrue(_is_private_method('__a_')) + self.assertTrue(_is_private_method('__a_a')) + self.assertTrue(_is_private_method('__a_a_')) + self.assertTrue(_is_private_method('__a__a')) + self.assertTrue(_is_private_method('__a__a_')) + self.assertTrue(_is_private_method('__a___a')) + self.assertTrue(_is_private_method('__a___a_')) + self.assertTrue(_is_private_method('___a')) + self.assertTrue(_is_private_method('___a_')) + self.assertTrue(_is_private_method('___a_a')) + self.assertTrue(_is_private_method('___a_a_')) + self.assertTrue(_is_private_method('____a_a_')) - self.assertFalse(private_name_regex.search('_a')) - self.assertFalse(private_name_regex.search('_a_')) - self.assertFalse(private_name_regex.search('__a__')) - self.assertFalse(private_name_regex.search('__')) - self.assertFalse(private_name_regex.search('___')) - self.assertFalse(private_name_regex.search('____')) + self.assertFalse(_is_private_method('_a')) + self.assertFalse(_is_private_method('_a_')) + self.assertFalse(_is_private_method('__a__')) + self.assertFalse(_is_private_method('__')) + self.assertFalse(_is_private_method('___')) + self.assertFalse(_is_private_method('____')) def test_unserializable_callback1(self): r = Request("http://www.example.com", callback=lambda x: x) From bcad8947e8192448ab3bd59489444efb567f8793 Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Mon, 3 Jun 2019 20:41:02 +0900 Subject: [PATCH 5/8] Support inherited private method names --- scrapy/utils/reqser.py | 9 +++++++-- tests/test_utils_reqser.py | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index 40223661f..d1f472e6e 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -81,8 +81,13 @@ def _find_method(obj, func): if func_self is obj: name = six.get_method_function(func).__name__ if _is_private_method(name): - classname = obj.__class__.__name__.lstrip('_') - name = '_%s%s' % (classname, name) + qualname = getattr(func, '__qualname__', None) + if qualname is None: + classname = obj.__class__.__name__.lstrip('_') + name = '_%s%s' % (classname, name) + else: + splits = qualname.split('.') + name = '_%s%s' % (splits[-2], splits[-1]) return name raise ValueError("Function %s is not a method of: %s" % (func, obj)) diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index fad5b6003..31577bc8c 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import unittest +import sys from scrapy.http import Request, FormRequest from scrapy.spiders import Spider @@ -74,6 +75,14 @@ class RequestSerializationTest(unittest.TestCase): errback=self.spider.handle_error) self._assert_serializes_ok(r, spider=self.spider) + def test_mixin_private_callback_serialization(self): + if sys.version_info[0] < 3: + return + r = Request("http://www.example.com", + callback=self.spider._TestSpiderMixin__mixin_callback, + errback=self.spider.handle_error) + self._assert_serializes_ok(r, spider=self.spider) + def test_private_callback_name_matching(self): self.assertTrue(_is_private_method('__a')) self.assertTrue(_is_private_method('__a_')) @@ -106,7 +115,12 @@ class RequestSerializationTest(unittest.TestCase): self.assertRaises(ValueError, request_to_dict, r) -class TestSpider(Spider): +class TestSpiderMixin(object): + def __mixin_callback(self, response): + pass + + +class TestSpider(Spider, TestSpiderMixin): name = 'test' def parse_item(self, response): From 9c81721c407ff41ef9dce2c33e26ac477355cf1f Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Wed, 5 Jun 2019 23:43:56 +0900 Subject: [PATCH 6/8] Add tests for private method name mangling --- scrapy/utils/reqser.py | 18 +++++++++++------- tests/test_utils_reqser.py | 16 +++++++++++++++- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py index d1f472e6e..3c463cfed 100644 --- a/scrapy/utils/reqser.py +++ b/scrapy/utils/reqser.py @@ -71,6 +71,16 @@ def _is_private_method(name): return name.startswith('__') and not name.endswith('__') +def _mangle_private_name(obj, func, name): + qualname = getattr(func, '__qualname__', None) + if qualname is None: + classname = obj.__class__.__name__.lstrip('_') + return '_%s%s' % (classname, name) + else: + splits = qualname.split('.') + return '_%s%s' % (splits[-2], splits[-1]) + + def _find_method(obj, func): if obj: try: @@ -81,13 +91,7 @@ def _find_method(obj, func): if func_self is obj: name = six.get_method_function(func).__name__ if _is_private_method(name): - qualname = getattr(func, '__qualname__', None) - if qualname is None: - classname = obj.__class__.__name__.lstrip('_') - name = '_%s%s' % (classname, name) - else: - splits = qualname.split('.') - name = '_%s%s' % (splits[-2], splits[-1]) + return _mangle_private_name(obj, func, name) return name raise ValueError("Function %s is not a method of: %s" % (func, obj)) diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index 31577bc8c..7f9e31daa 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -2,9 +2,11 @@ import unittest import sys +import six + from scrapy.http import Request, FormRequest from scrapy.spiders import Spider -from scrapy.utils.reqser import request_to_dict, request_from_dict, _is_private_method +from scrapy.utils.reqser import request_to_dict, request_from_dict, _is_private_method, _mangle_private_name class RequestSerializationTest(unittest.TestCase): @@ -105,6 +107,18 @@ class RequestSerializationTest(unittest.TestCase): self.assertFalse(_is_private_method('___')) self.assertFalse(_is_private_method('____')) + def _assert_mangles_to(self, obj, name): + self.assertEqual( + _mangle_private_name(obj, getattr(obj, name), name), + name + ) + + def test_private_name_mangling(self): + self._assert_mangles_to( + self.spider, '_TestSpider__parse_item_private') + self._assert_mangles_to( + self.spider, '_TestSpiderMixin__mixin_callback') + def test_unserializable_callback1(self): r = Request("http://www.example.com", callback=lambda x: x) self.assertRaises(ValueError, request_to_dict, r) From 3dd3e8c29863683d60f9c4f74aacac3103703061 Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Wed, 5 Jun 2019 23:49:54 +0900 Subject: [PATCH 7/8] Restrict different class mangling tests to Py 3+ --- tests/test_utils_reqser.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index 7f9e31daa..57dc5db53 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -116,8 +116,9 @@ class RequestSerializationTest(unittest.TestCase): def test_private_name_mangling(self): self._assert_mangles_to( self.spider, '_TestSpider__parse_item_private') - self._assert_mangles_to( - self.spider, '_TestSpiderMixin__mixin_callback') + if sys.version_info[0] >= 3: + self._assert_mangles_to( + self.spider, '_TestSpiderMixin__mixin_callback') def test_unserializable_callback1(self): r = Request("http://www.example.com", callback=lambda x: x) From 6af1dc89aa5988ebbfbef90afdafa84736f3993c Mon Sep 17 00:00:00 2001 From: Andrew Baxter Date: Thu, 6 Jun 2019 04:25:19 +0900 Subject: [PATCH 8/8] Fix mangling test --- tests/test_utils_reqser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py index 57dc5db53..e5a09dcf1 100644 --- a/tests/test_utils_reqser.py +++ b/tests/test_utils_reqser.py @@ -108,8 +108,9 @@ class RequestSerializationTest(unittest.TestCase): self.assertFalse(_is_private_method('____')) def _assert_mangles_to(self, obj, name): + func = getattr(obj, name) self.assertEqual( - _mangle_private_name(obj, getattr(obj, name), name), + _mangle_private_name(obj, func, func.__name__), name )