Added typo data collection (/submitData)

Added typo data collection (/submitData)

[Wladimir Palant, 2012-10-09 11:51:14]

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README.md
File sitescripts/urlfixer/README.md (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:21: A domain transfered to the server can have
one of the following status:
"Status" doesn't seem to be the right word, should be plural anyway - "states"?

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:23: * 1: the domain was entered
Better: "was entered without any further action" - all these domains have been
entered.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:24: * 2: there was a typo
Where and what does it have to do with the domain? Better: "the domain was
recognized as a typo"

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:25: * 3: the domain has been corrected
No, this domain *is* the correction. Meaning: "the domain is a correction that
the user accepted"

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:26: * 4: false positive (domain was corrected
even though the typed domain was no typo)
Better explanation: "user declined our correction"

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema.sql
File sitescripts/urlfixer/schema.sql (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema....
sitescripts/urlfixer/schema.sql:8: correction_correct FLOAT(10,10),
Aren't these two fields just derivatives of the data in corrections table? Then
we probably shouldn't cache them here for sake of database normalization -
better calculate them in the script used to extract the data later.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema....
sitescripts/urlfixer/schema.sql:25: CREATE UNIQUE INDEX idx_domain_status ON
corrections(domain, status);
Use UNIQUE constraint instead of these two indexes? Meaning: UNIQUE (domains)
and UNIQUE (domain, status)

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
File sitescripts/urlfixer/web/submitData.py (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:9: # from sitescripts.web import
url_handler, basic_auth
Any reason why the lines above are required? ;)

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:13: from math import sqrt
I would prefer to just import math and use the function as math.sqrt() so that
it is obvious where the function came from. I've deviated from that rule quite a
bit myself in this repository - but I was learning the language as well.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:40: data = json.loads(data, ["{}"])
Not sure I understand the significance of the second parameter - is that
supposed to be the encoding?

Also, this line should be outside the |if| block - if you fall back to the
default value then you should parse it as well.

Finally, you probably want to catch JSONDecodeError error and show an error
message - that's a user input error (not that a user should ever see it).

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:47: return showError("Error while reading
config.", start_response)
That's a server configuration issue, not something that the user needs to know.
But we need to know about it which is why swallowing the error isn't a good
idea. I would rather suggest that you leave that error alone - it will get
written into the logs along with a precise description of what's wrong.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:54: use_unicode=True, charset="utf8")
I suggest caching the database connection similarly to
sitescripts.crawler.web.crawler - we will get lots of connections here, not
opening a new connection each time is useful.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:56: for domain in data:
I suggest:

  for domain, status in data.iteritems():

You can get both keys and values from the iterator.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:60: db.close()
Database shouldn't be closed if you share the database connection as I suggested
above.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:63: ('Content-Length', str(len(output)))]
Output is empty and this won't change (nobody can see it). Meaning: get rid of
the output variable, don't set "Content-Length" header (it will be set
automatically anyway), return an empty list.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:115: cursor.execute("SELECT id FROM
domains WHERE domain='%s'" % (domain))
Please don't ever generate queries dynamically, use prepared statements (keyword
if you want to know the reason: SQL injection). This should be:

    cursor.execute("SELECT id FROM domains WHERE domain = %s", domain)

And similarly for all the other SQL statements.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:119: return _get_domain_id(db, domain)
Should be:

  return db.insert_id()

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:137: cursor.execute("UPDATE corrections
SET curr_month=curr_month+1, curr_year=curr_year+1 WHERE domain=%d AND
status=%d" % (domain_id, status))
http://dev.mysql.com/doc/refman/5.0/en/insert-on-duplicate.html (see saveReport
in sitescripts.reports.utils for an example)

[Thomas Greiner, 2012-10-09 13:47:01]

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README.md
File sitescripts/urlfixer/README.md (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/README....
sitescripts/urlfixer/README.md:21: A domain transfered to the server can have
one of the following status:
On 2012/10/09 11:51:14, Wladimir Palant wrote:
> "Status" doesn't seem to be the right word, should be plural anyway -
"states"?

I tried to avoid calling them "types" due to possible problems with reserved
keywords.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema.sql
File sitescripts/urlfixer/schema.sql (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema....
sitescripts/urlfixer/schema.sql:25: CREATE UNIQUE INDEX idx_domain_status ON
corrections(domain, status);
On 2012/10/09 11:51:14, Wladimir Palant wrote:
> Use UNIQUE constraint instead of these two indexes? Meaning: UNIQUE (domains)
> and UNIQUE (domain, status)

I prefer having indexes because SELECTs on this table are made by using
domain+status and therefore it would speed up the process for the score
calculation and others.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
File sitescripts/urlfixer/web/submitData.py (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:115: cursor.execute("SELECT id FROM
domains WHERE domain='%s'" % (domain))
On 2012/10/09 11:51:14, Wladimir Palant wrote:
> Please don't ever generate queries dynamically, use prepared statements
(keyword
> if you want to know the reason: SQL injection). This should be:
> 
>     cursor.execute("SELECT id FROM domains WHERE domain = %s", domain)
> 
> And similarly for all the other SQL statements.

Actually, I thought that this was the syntax for prepared statements. But I
guess I was wrong.

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/web/sub...
sitescripts/urlfixer/web/submitData.py:137: cursor.execute("UPDATE corrections
SET curr_month=curr_month+1, curr_year=curr_year+1 WHERE domain=%d AND
status=%d" % (domain_id, status))
On 2012/10/09 11:51:14, Wladimir Palant wrote:
> http://dev.mysql.com/doc/refman/5.0/en/insert-on-duplicate.html (see
saveReport
> in sitescripts.reports.utils for an example)

Thanks for the tip!

[Wladimir Palant, 2012-10-09 14:17:08]

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema.sql
File sitescripts/urlfixer/schema.sql (right):

http://codereview.adblockplus.org/8483155/diff/1/sitescripts/urlfixer/schema....
sitescripts/urlfixer/schema.sql:25: CREATE UNIQUE INDEX idx_domain_status ON
corrections(domain, status);
On 2012/10/09 13:47:02, Thomas Greiner wrote:
> I prefer having indexes because SELECTs on this table are made by using
> domain+status and therefore it would speed up the process for the score
> calculation and others.

A UNIQUE constraint will always create an index implicitly.

http://codereview.adblockplus.org/8483155/diff/7001/sitescripts/urlfixer/web/...
File sitescripts/urlfixer/web/submitData.py (right):

http://codereview.adblockplus.org/8483155/diff/7001/sitescripts/urlfixer/web/...
sitescripts/urlfixer/web/submitData.py:76: cursor.execute("INSERT INTO
corrections(domain, status, curr_month, prev_month, curr_year, prev_year) VALUES
(%s, %s, %s, %s, %s, %s) ON DUPLICATE KEY UPDATE curr_month=curr_month+1,
curr_year=curr_year+1", (domain_id, status, 1, 0, 1, 0))
Style nit: spaces around binary operators

[Wladimir Palant, 2012-10-09 14:29:37]

LGTM