Crawler backend

I've made various changes and improvements to the crawler backend while working on the crawler.

[Wladimir Palant, 2012-09-14 14:42:13]

http://codereview.adblockplus.org/8327353/diff/1/sitescripts/crawler/bin/extr...
File sitescripts/crawler/bin/extract_crawler_sites.py (right):

http://codereview.adblockplus.org/8327353/diff/1/sitescripts/crawler/bin/extr...
sitescripts/crawler/bin/extract_crawler_sites.py:23: matches =
re.match(r"[A-Z]:.*(https?://.*)", line)
What if the URL is followed by some additional info? Maybe at least \S* and not
.*?

http://codereview.adblockplus.org/8327353/diff/1/sitescripts/crawler/bin/extr...
sitescripts/crawler/bin/extract_crawler_sites.py:34: print "INSERT INTO
crawler_sites (url) VALUES ('" + url + "');"
SQL injection? :-(

[Felix Dahlke, 2012-09-14 14:42:44]

Uploaded a new patch set with a quick README fix.

[Wladimir Palant, 2012-09-14 17:24:18]

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
File sitescripts/crawler/bin/extract_crawler_sites.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:20: if line == "":
Is this really a good break condition? An empty commit line is unlikely but
possible... Anyway, how about "for line in process.stdout:" instead of a while
loop?

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:23: matches =
re.match(r"[A-Z]:.*(https?://.*)", line)
What if we have some additional text following the URL? Or an inexperienced
EasyList author deviates from the "A:" format? I would prefer something more
generic like r"\bhttps?://\S+", maybe even with an additional .strip(",.!?")
below.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:34: print "INSERT INTO
crawler_sites (url) VALUES ('" + url + "');"
While this might not be a big issue here, failing to escape values in SQL
statements is a bad habit. Please add escaping or - better - eliminate the SQL
file as intermediate step. Just add the URLs to the database immediately using
prepared statements.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/schem...
File sitescripts/crawler/schema.sql (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/schem...
sitescripts/crawler/schema.sql:8: url VARCHAR(512) NOT NULL
How about UNIQUE(url) constraint? This will make sure we don't add the same URL
twice due to some bug. And it will create an index (which is necessary given
that you frequently query this table by URL).

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:35: return cursor.fetchall()[0]["id"]
a) Why use fetchall() if we are only interested in one result? See fetchone().
b) What if there is no result (e.g. data consistency issue)? Please make sure
you don't fail completely then.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:42: line = data_file.readline().strip()
Same as earlier, please use:

  for line in environ["wsgi.input"]:

Note how your loop will run forever if the boundary is not found in the input.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:49: if current_line < 5 or not line:
No, that's not how you parse multipart/form-data (classical magic number).
Please extract the boundary from environ.get('HTTP_CONTENT_DISPOSITION', '')
(should be following "; boundary=") and look for the line "--" + boundary
starting a field. It is followed by a few headers (number unknown) and an empty
line - and then the contents. "--" + boundary + "--" ends it then - assuming
that we only get one field is fine here IMHO, not the assumptions about how the
browser will format multipart/form-data.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/bi...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/bi...
sitescripts/extensions/bin/createNightlies.py:1: # coding: utf-8
This looks like Andrey's patch, not yours...

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/te...
File sitescripts/extensions/template/androidupdates.xml (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/te...
sitescripts/extensions/template/androidupdates.xml:1: <?xml version='1.0'
encoding='UTF-8'?>
This looks like Andrey's patch, not yours...

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/ut...
File sitescripts/extensions/utils.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/extensions/ut...
sitescripts/extensions/utils.py:1: # coding: utf-8
This looks like Andrey's patch, not yours...

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/web.py
File sitescripts/web.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/web.py#newcode33
sitescripts/web.py:33: expected_username = get_config().get("DEFAULT",
"basic_auth_username")
I don't think that these settings belong into the DEFAULT section, what if we
decide to use different credentials for different scripts? How about passing the
config section to basic_auth()?

[Felix Dahlke, 2012-09-15 02:40:19]

Wow, a lot of helpful suggestions, thanks :) Particularly happy about "for line
in process.stdout" if that works out. The examples I've found do this empty
string checking but it's making me a bit nervous.

As for Andrey's patch: I fear I've included all commits since the ID I
specified, including those not from me. If we work with branches or bookmarks
from now on, that shouldn't happen anymore. I didn't touch any files in
sitescripts/extensions.

[Felix Dahlke, 2012-09-26 15:20:30]

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
File sitescripts/crawler/bin/extract_crawler_sites.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:20: if line == "":
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> Is this really a good break condition? An empty commit line is unlikely but
> possible... Anyway, how about "for line in process.stdout:" instead of a while
> loop?

Done. An empty commit line would be "\n", but you're definitely right that for
... in is nicer.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:23: matches =
re.match(r"[A-Z]:.*(https?://.*)", line)
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> What if we have some additional text following the URL? Or an inexperienced
> EasyList author deviates from the "A:" format? I would prefer something more
> generic like r"\bhttps?://\S+", maybe even with an additional .strip(",.!?")
> below.

Done.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/bin/e...
sitescripts/crawler/bin/extract_crawler_sites.py:34: print "INSERT INTO
crawler_sites (url) VALUES ('" + url + "');"
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> While this might not be a big issue here, failing to escape values in SQL
> statements is a bad habit. Please add escaping or - better - eliminate the SQL
> file as intermediate step. Just add the URLs to the database immediately using
> prepared statements.

Done. I'd rather stick to generating a file with statements. More convenient to
test/verify, and we can keep old files for regression testing.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/schem...
File sitescripts/crawler/schema.sql (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/schem...
sitescripts/crawler/schema.sql:8: url VARCHAR(512) NOT NULL
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> How about UNIQUE(url) constraint? This will make sure we don't add the same
URL
> twice due to some bug. And it will create an index (which is necessary given
> that you frequently query this table by URL).

Done.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:35: return cursor.fetchall()[0]["id"]
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> a) Why use fetchall() if we are only interested in one result? See fetchone().
> b) What if there is no result (e.g. data consistency issue)? Please make sure
> you don't fail completely then.

Done.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:42: line = data_file.readline().strip()
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> Same as earlier, please use:
> 
>   for line in environ["wsgi.input"]:
> 
> Note how your loop will run forever if the boundary is not found in the input.

Done.

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:49: if current_line < 5 or not line:
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> No, that's not how you parse multipart/form-data (classical magic number).
> Please extract the boundary from environ.get('HTTP_CONTENT_DISPOSITION', '')
> (should be following "; boundary=") and look for the line "--" + boundary
> starting a field. It is followed by a few headers (number unknown) and an
empty
> line - and then the contents. "--" + boundary + "--" ends it then - assuming
> that we only get one field is fine here IMHO, not the assumptions about how
the
> browser will format multipart/form-data.

Done. Wow, that was one mean hack I did there :)

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/web.py
File sitescripts/web.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/web.py#newcode33
sitescripts/web.py:33: expected_username = get_config().get("DEFAULT",
"basic_auth_username")
On 2012/09/14 17:24:18, Wladimir Palant wrote:
> I don't think that these settings belong into the DEFAULT section, what if we
> decide to use different credentials for different scripts? How about passing
the
> config section to basic_auth()?

Done.

[Wladimir Palant, 2012-09-27 06:13:24]

Sorry, I cannot really review that without interdiff - too many changes to
compare them manually. Please either upload a patch against the same base
revision as patchset 2 or at least upload a diff against the current tip (--rev
b499161baebe).

[Felix Dahlke, 2012-09-27 06:23:58]

On 2012/09/27 06:13:24, Wladimir Palant wrote:
> Sorry, I cannot really review that without interdiff - too many changes to
> compare them manually. Please either upload a patch against the same base
> revision as patchset 2 or at least upload a diff against the current tip
(--rev
> b499161baebe).

Re-uploaded the patch set, interdiffs looking OK now.

[Wladimir Palant, 2012-09-27 07:34:17]

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/3002/sitescripts/crawler/web/c...
sitescripts/crawler/web/crawler.py:49: if current_line < 5 or not line:
On 2012/09/26 15:20:30, Felix H. Dahlke wrote:
> Done. Wow, that was one mean hack I did there :)

Much better now :)

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/bin/...
File sitescripts/crawler/bin/extract_crawler_sites.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/bin/...
sitescripts/crawler/bin/extract_crawler_sites.py:19: matches =
re.match(r".*\b(https?://\S*)", line)
Please use re.search() rather than re.match() here (and then without .* at the
start). re.match() will always match the complete string - we are actually
interested in a substring.

Generally, I consider re.match() in Python's standard library a bug - it's
confusing and typically useless.

Side-note: I would probably name the result variable match and not matches - it
is really only one match unlike when calling re.findall().

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:42: boundary = re.search(r"boundary=(.*)",
environ["CONTENT_TYPE"]).group(1)
Assumptions here:

1) There is a CONTENT_TYPE header.
2) It starts with "multipart/form-data".
3) It also contains "boundary=" substring.

These are probably valid assumptions to make but I would normally make sure to
produce a proper error message if any of these is violated.

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:75: 
Thinking about that... So we give the client a list of URLs from crawler_sites
table but ignore IDs, then we get the URL back from the client and have to find
the corresponding ID? Seems not very logical. Maybe send both the URL and the ID
to the client and have it use the site ID in the data it sends back?

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/hg/bin/ircho...
File sitescripts/hg/bin/irchook.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/hg/bin/ircho...
sitescripts/hg/bin/irchook.py:1: import os, subprocess, pipes
That's my change, not yours :)

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/web.py
File sitescripts/web.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/web.py#newco...
sitescripts/web.py:24: def basic_auth(f):
This should accept an optional config_section parameter and the crawler should
actually use it (config_section="crawler"). It will still fall back to DEFAULT
if there is no dedicated setting but at least it will be possible to specify a
different password for the crawler.

[Felix Dahlke, 2012-09-27 09:26:24]

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/bin/...
File sitescripts/crawler/bin/extract_crawler_sites.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/bin/...
sitescripts/crawler/bin/extract_crawler_sites.py:19: matches =
re.match(r".*\b(https?://\S*)", line)
On 2012/09/27 07:34:17, Wladimir Palant wrote:
> Please use re.search() rather than re.match() here (and then without .* at the
> start). re.match() will always match the complete string - we are actually
> interested in a substring.
> 
> Generally, I consider re.match() in Python's standard library a bug - it's
> confusing and typically useless.
> 
> Side-note: I would probably name the result variable match and not matches -
it
> is really only one match unlike when calling re.findall().

Done. You're right, I'm used to calling it "matches" because it's an array with
the matched string and all groups in JS. It's really just one though.

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:42: boundary = re.search(r"boundary=(.*)",
environ["CONTENT_TYPE"]).group(1)
On 2012/09/27 07:34:17, Wladimir Palant wrote:
> Assumptions here:
> 
> 1) There is a CONTENT_TYPE header.
> 2) It starts with "multipart/form-data".
> 3) It also contains "boundary=" substring.
> 
> These are probably valid assumptions to make but I would normally make sure to
> produce a proper error message if any of these is violated.

Done. You're finding a lot of those because my mindset has always been to write
the simplest code possible, see what exceptions occur during testing and handle
errors properly where necessary. But that was in environments with lots of
testing before deployment, so I guess I'll better get used to handling things
like that properly form the start.

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:75: 
On 2012/09/27 07:34:17, Wladimir Palant wrote:
> Thinking about that... So we give the client a list of URLs from crawler_sites
> table but ignore IDs, then we get the URL back from the client and have to
find
> the corresponding ID? Seems not very logical. Maybe send both the URL and the
ID
> to the client and have it use the site ID in the data it sends back?

I did that for two reasons:
1. The site urls are definitely unique (now even guaranteed by the database)
2. The ID is a database detail that could actually change for some reason, e.g.
denormalization

The problem is that this is blowing the size of the crawlerData request up
somewhat. I was planning to wait and see if that's going to be a problem.

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/web.py
File sitescripts/web.py (right):

http://codereview.adblockplus.org/8327353/diff/18002/sitescripts/web.py#newco...
sitescripts/web.py:24: def basic_auth(f):
On 2012/09/27 07:34:17, Wladimir Palant wrote:
> This should accept an optional config_section parameter and the crawler should
> actually use it (config_section="crawler"). It will still fall back to DEFAULT
> if there is no dedicated setting but at least it will be possible to specify a
> different password for the crawler.

Done. Somehow thought authenticate was used directly.

[Wladimir Palant, 2012-09-27 13:44:51]

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:28: @basic_auth
Shouldn't this be @basic_auth(config_section="crawler")?

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:44: raise ValueError("Content-Type missing
from header")
Python doesn't work like that - accessing a non-existing key will cause an
exception. So you better change that condition into "if not 'CONTENT_TYPE' in
environ:" and only access the key after the check. Alternatively you can use
environ.get("CONTENT_TYPE", "") and leave out the check here (an empty string
will trigger an error below).

And: yes, I am used to looking at the edge conditions when I write code rather
than leaving it up to testing.

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:73: return True
That return value is unused, left-over from earlier testing?

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:93: @basic_auth
Shouldn't this be @basic_auth(config_section="crawler")?

[Felix Dahlke, 2012-09-27 14:15:33]

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
File sitescripts/crawler/web/crawler.py (right):

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:28: @basic_auth
On 2012/09/27 13:44:51, Wladimir Palant wrote:
> Shouldn't this be @basic_auth(config_section="crawler")?

Done.

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:44: raise ValueError("Content-Type missing
from header")
On 2012/09/27 13:44:51, Wladimir Palant wrote:
> Python doesn't work like that - accessing a non-existing key will cause an
> exception. So you better change that condition into "if not 'CONTENT_TYPE' in
> environ:" and only access the key after the check. Alternatively you can use
> environ.get("CONTENT_TYPE", "") and leave out the check here (an empty string
> will trigger an error below).

Done.

> And: yes, I am used to looking at the edge conditions when I write code rather
> than leaving it up to testing.

I'll work on it.

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:73: return True
On 2012/09/27 13:44:51, Wladimir Palant wrote:
> That return value is unused, left-over from earlier testing?

Done.

http://codereview.adblockplus.org/8327353/diff/23003/sitescripts/crawler/web/...
sitescripts/crawler/web/crawler.py:93: @basic_auth
On 2012/09/27 13:44:51, Wladimir Palant wrote:
> Shouldn't this be @basic_auth(config_section="crawler")?

Done.

[Wladimir Palant, 2012-09-27 14:24:13]

LGTM

http://codereview.adblockplus.org/8327353/diff/29002/sitescripts/web.py
File sitescripts/web.py (right):

http://codereview.adblockplus.org/8327353/diff/29002/sitescripts/web.py#newco...
sitescripts/web.py:29: return decorator
I already suspected that the old version didn't work :)

[Felix Dahlke, 2012-09-27 14:25:23]

On 2012/09/27 14:24:13, Wladimir Palant wrote:
> LGTM

Hurray!
 
> http://codereview.adblockplus.org/8327353/diff/29002/sitescripts/web.py
> File sitescripts/web.py (right):
> 
>
http://codereview.adblockplus.org/8327353/diff/29002/sitescripts/web.py#newco...
> sitescripts/web.py:29: return decorator
> I already suspected that the old version didn't work :)

It did work until I added the parameter - and actually used it :)