clean db interface and comment model

isso/__init__.py

@@ -29,8 +29,6 @@ import sys
 import io
 import os
 import json
-import locale
-import traceback
 
 from os.path import dirname, join
 from argparse import ArgumentParser
@@ -48,7 +46,7 @@ from werkzeug.serving import run_simple
 
 from jinja2 import Environment, FileSystemLoader
 
-from isso import db, utils, migrate
+from isso import db, utils, migrate, views
 from isso.views import comment, admin
 
 url_map = Map([

isso/db.py

@@ -122,29 +122,29 @@ class SQLite(Abstract):
             votes=query[10]
         )
 
-    def add(self, path, c, remote_addr):
+    def add(self, uri, c, remote_addr):
         voters = buffer(Bloomfilter(iterable=[remote_addr]).array)
         with sqlite3.connect(self.dbpath) as con:
             keys = ','.join(self.fields)
             values = ','.join('?' * len(self.fields))
             con.execute('INSERT INTO comments (%s) VALUES (%s);' % (keys, values), (
-                path, 0, c.created, c.modified, c.text, c.author, c.hash, c.website,
-                c.parent, self.mode, voters)
+                uri, 0, time.time(), None, c["text"], c["author"], c["hash"], c["website"],
+                c["parent"], self.mode, voters)
             )
 
         with sqlite3.connect(self.dbpath) as con:
             return self.query2comment(
-                con.execute('SELECT *, MAX(id) FROM comments WHERE path=?;', (path, )).fetchone())
+                con.execute('SELECT *, MAX(id) FROM comments WHERE path=?;', (uri, )).fetchone())
 
     def activate(self, path, id):
         with sqlite3.connect(self.dbpath) as con:
             con.execute("UPDATE comments SET mode=1 WHERE path=? AND id=? AND mode=2", (path, id))
         return self.get(path, id)
 
-    def update(self, path, id, comment):
+    def update(self, path, id, values):
         with sqlite3.connect(self.dbpath) as con:
-            for field, value in comment.iteritems(False):
-                con.execute('UPDATE comments SET %s=? WHERE path=? AND id=?;' % field,
+            for key, value in values.iteritems():
+                con.execute('UPDATE comments SET %s=? WHERE path=? AND id=?;' % key,
                     (value, path, id))
 
         with sqlite3.connect(self.dbpath) as con:
@@ -167,7 +167,7 @@ class SQLite(Abstract):
 
             con.execute('UPDATE comments SET text=? WHERE path=? AND id=?', ('', path, id))
             con.execute('UPDATE comments SET mode=? WHERE path=? AND id=?', (4, path, id))
-            for field in set(Comment.fields) - set(['text', 'parent']):
+            for field in ('text', 'author', 'website'):
                 con.execute('UPDATE comments SET %s=? WHERE path=? AND id=?' % field,
                     (None, path, id))
         return self.get(path, id)

isso/migrate.py

@@ -13,7 +13,7 @@ from __future__ import division
 import sys
 import os
 
-from time import mktime, strptime, sleep
+from time import mktime, strptime
 from urlparse import urlparse
 from collections import defaultdict
 

isso/models.py

@@ -3,11 +3,9 @@
 # Copyright 2012, Martin Zimmermann <info@posativ.org>. All rights reserved.
 # License: BSD Style, 2 clauses. see isso/__init__.py
 
-import json
-import time
-import hashlib
+from __future__ import unicode_literals
 
-aluhut = lambda ip: hashlib.sha1(ip + '\x082@t9*\x17\xad\xc1\x1c\xa5\x98').hexdigest()
+import hashlib
 
 
 class Comment(object):
@@ -25,50 +23,41 @@ class Comment(object):
     normal and queued using MODE=3.
     """
 
-    protected = ['path', 'id', 'mode', 'created', 'modified', 'hash', 'votes']
-    fields = ['text', 'author', 'website', 'parent']
-
-    def __init__(self, **kw):
+    fields = ["text", "author", "website", "votes", "hash", "parent", "mode", "id",
+              "created", "modified"]
 
-        for field in self.protected + self.fields:
-            setattr(self, field, kw.get(field))
+    def __init__(self, **kwargs):
 
-    def iteritems(self, protected=True):
-        for field in self.fields:
-            yield field, getattr(self, field)
-        if protected:
-            for field in self.protected:
-                yield field, getattr(self, field)
+        self.values = {}
 
-    @classmethod
-    def fromjson(self, data, ip='127.0.0.1'):
+        for key in Comment.fields:
+            self.values[key] = kwargs.get(key, None)
 
-        if '.' in ip:
-            ip = ip.rsplit('.', 1)[0] + '.0'
+    def __getitem__(self, key):
+        return self.values[key]
 
-        data = json.loads(data)
-        comment = Comment(
-            created=time.time(),
-            hash=hashlib.md5(data.get('email', aluhut(ip))).hexdigest())
+    def __setitem__(self, key, value):
+        self.values[key] = value
 
-        for field in self.fields:
-            if field == 'text' and field not in data:
-                raise ValueError('Comment needs at least text, but no text was provided.')
-            setattr(comment, field, data.get(field))
-
-        return comment
+    def iteritems(self):
+        for key in Comment.fields:
+            yield key, self.values[key]
 
     @property
     def pending(self):
-        return self.mode == 2
+        return self.values["mode"] == 2
 
     @property
     def deleted(self):
-        return self.mode == 4
+        return self.values["mode"] == 4
 
     @property
     def md5(self):
         hv = hashlib.md5()
-        for key in set(self.fields) - set(['parent', ]):
-            hv.update((getattr(self, key) or u"").encode('utf-8', errors="replace"))
+
+        for key, value in self.iteritems():
+            if key == "parent" or value is None:
+                continue
+            hv.update(unicode(self.values.get(key, "")).encode("utf-8", "replace")) # XXX
+
         return hv.hexdigest()

isso/utils.py

@@ -9,8 +9,8 @@ import json
 
 import socket
 import httplib
+import ipaddress
 
-import math
 import random
 import hashlib
 import contextlib
@@ -48,6 +48,17 @@ def normalize(host):
     return (rv.netloc + ':443') if rv.scheme == 'https' else rv.netloc
 
 
+def anonymize(remote_addr):
+    ip = ipaddress.ip_address(remote_addr)
+    if ip.version == "4":
+        return ''.join(ip.exploded.rsplit('.', 1)[0]) + '.' + '0'
+    return ip.exploded.rsplit(':', 4)[0]
+
+
+def salt(value, s=u'\x082@t9*\x17\xad\xc1\x1c\xa5\x98'):
+    return hashlib.sha1((value + s).encode('utf-8')).hexdigest()
+
+
 def mksecret(length):
     return ''.join(random.choice(ascii_letters + digits) for x in range(length))
 

isso/views/comment.py

@@ -4,7 +4,10 @@
 # License: BSD Style, 2 clauses. see isso/__init__.py
 
 import cgi
-import urllib
+import json
+import hashlib
+import sqlite3
+import logging
 
 from itsdangerous import SignatureExpired, BadSignature
 
@@ -43,28 +46,40 @@ def create(app, environ, request, uri):
         return Response('URI does not exist', 400)
 
     try:
-        comment = models.Comment.fromjson(request.data, ip=request.remote_addr)
-    except ValueError as e:
-        return Response(unicode(e), 400)
+        data = json.loads(request.data)
+    except ValueError:
+        return Response("No JSON object could be decoded", 400)
 
-    for attr in 'author', 'website':
-        if getattr(comment, attr) is not None:
-            try:
-                setattr(comment, attr, cgi.escape(getattr(comment, attr)))
-            except AttributeError:
-                Response('', 400)
+    if not data.get("text"):
+        return Response("No text given.", 400)
+
+    if "id" in data and not isinstance(data["id"], int):
+        return Response("Parent ID must be an integer.")
+
+    if "email" in data:
+        hash = data["email"]
+    else:
+        hash = utils.salt(utils.anonymize(request.remote_addr))
+
+    comment = models.Comment(
+        text=data["text"], parent=data.get("parent"),
+
+        author=data.get("author") and cgi.escape(data.get("author")),
+        website=data.get("website") and cgi.escape(data.get("website")),
+
+        hash=hashlib.md5(hash).hexdigest())
 
     try:
-        rv = app.db.add(uri, comment, request.remote_addr)
-    except ValueError:
-        abort(400)  # FIXME: custom exception class, error descr
+        rv = app.db.add(uri, comment, utils.anonymize(request.remote_addr))
+    except sqlite3.Error:
+        logging.exception('uncaught SQLite3 exception')
+        abort(400)
 
-    md5 = rv.md5
-    rv.text = app.markdown(rv.text)
+    rv["text"] = app.markdown(rv["text"])
 
     resp = Response(app.dumps(rv), 202 if rv.pending else 201,
         content_type='application/json')
-    resp.set_cookie('%s-%s' % (uri, rv.id), app.sign([uri, rv.id, md5]),
+    resp.set_cookie('%s-%s' % (uri, rv["id"]), app.sign([uri, rv["id"], rv.md5]),
        max_age=app.MAX_AGE, path='/')
     return resp
 
@@ -94,7 +109,7 @@ def modify(app, environ, request, uri, id):
 
     try:
         rv = app.unsign(request.cookies.get('%s-%s' % (uri, id), ''))
-    except (SignatureExpired, BadSignature) as e:
+    except (SignatureExpired, BadSignature):
         try:
             rv = app.unsign(request.cookies.get('admin', ''))
         except (SignatureExpired, BadSignature):
@@ -106,11 +121,25 @@ def modify(app, environ, request, uri, id):
 
     if request.method == 'PUT':
         try:
-            rv = app.db.update(uri, id, models.Comment.fromjson(request.data))
-            rv.text = app.markdown(rv.text)
-            return Response(app.dumps(rv), 200, content_type='application/json')
-        except ValueError as e:
-            return Response(unicode(e), 400) # FIXME: custom exception and error descr
+            data = json.loads(request.data)
+        except ValueError:
+            return Response("No JSON object could be decoded", 400)
+
+        if not data.get("text"):
+            return Response("No text given.", 400)
+
+        for key in data.keys():
+            if key not in ("text", "author", "website"):
+                data.pop(key)
+
+        try:
+            rv = app.db.update(uri, id, data)
+        except sqlite3.Error:
+            logging.exception('uncaught SQLite3 exception')
+            abort(400)
+
+        rv["text"] = app.markdown(rv["text"])
+        return Response(app.dumps(rv), 200, content_type='application/json')
 
     if request.method == 'DELETE':
         rv = app.db.delete(uri, id)

specs/test_comment.py

@@ -1,4 +1,6 @@
 
+from __future__ import unicode_literals
+
 import os
 import json
 import urllib
@@ -57,11 +59,10 @@ class TestComments(unittest.TestCase):
         assert rv.status_code == 201
         assert len(filter(lambda header: header[0] == 'Set-Cookie', rv.headers)) == 1
 
-        c = Comment.fromjson(rv.data)
+        rv = json.loads(rv.data)
 
-        assert not c.pending
-        assert not c.deleted
-        assert c.text == '<p>Lorem ipsum ...</p>\n'
+        assert rv["mode"] == 1
+        assert rv["text"] == '<p>Lorem ipsum ...</p>\n'
 
     def testCreateAndGetMultiple(self):
 
@@ -154,3 +155,16 @@ class TestComments(unittest.TestCase):
         assert a['hash'] != '192.168.1.1'
         assert a['hash'] == b['hash']
         assert a['hash'] != c['hash']
+
+    def testVisibleFields(self):
+
+        rv = self.post('/new?uri=%2Fpath%2F', data=json.dumps({"text": "..."}))
+        assert rv.status_code == 201
+
+        rv = json.loads(rv.data)
+
+        for key in Comment.fields:
+            rv.pop(key)
+
+        assert rv.keys() == []
+

specs/test_db.py

@@ -1,19 +1,13 @@
 
 import os
-import json
 import time
 import tempfile
 import unittest
 
-import isso
 from isso.models import Comment
 from isso.db import SQLite
 
 
-def comment(**kw):
-    return Comment.fromjson(json.dumps(kw))
-
-
 class TestSQLite(unittest.TestCase):
 
     def setUp(self):
@@ -23,61 +17,59 @@ class TestSQLite(unittest.TestCase):
 
     def test_get(self):
 
-        rv = self.db.add('/', comment(text='Spam'), '')
-        c = self.db.get('/', rv.id)
+        rv = self.db.add('/', Comment(text='Spam'), '')
+        c = self.db.get('/', rv["id"])
 
-        assert c.id == 1
-        assert c.text == 'Spam'
+        assert c["id"] == 1
+        assert c["text"] == 'Spam'
 
     def test_add(self):
 
-        self.db.add('/', comment(text='Foo'), '')
-        self.db.add('/', comment(text='Bar'), '')
-        self.db.add('/path/', comment(text='Baz'), '')
+        self.db.add('/', Comment(text='Foo'), '')
+        self.db.add('/', Comment(text='Bar'), '')
+        self.db.add('/path/', Comment(text='Baz'), '')
 
         rv = list(self.db.retrieve('/'))
-        assert rv[0].id == 1
-        assert rv[0].path == '/'
-        assert rv[0].text == 'Foo'
+        assert rv[0]["id"] == 1
+        assert rv[0]["text"] == 'Foo'
 
-        assert rv[1].id == 2
-        assert rv[1].text == 'Bar'
+        assert rv[1]["id"] == 2
+        assert rv[1]["text"] == 'Bar'
 
         rv = list(self.db.retrieve('/path/'))
-        assert rv[0].id == 1
-        assert rv[0].path == '/path/'
-        assert rv[0].text == 'Baz'
+        assert rv[0]["id"] == 1
+        assert rv[0]["text"] == 'Baz'
 
     def test_add_return(self):
 
-        self.db.add('/', comment(text='1'), '')
-        self.db.add('/', comment(text='2'), '')
+        self.db.add('/', Comment(text='1'), '')
+        self.db.add('/', Comment(text='2'), '')
 
-        assert self.db.add('/path/', comment(text='1'), '').id == 1
+        assert self.db.add('/path/', Comment(text='1'), '')["id"] == 1
 
     def test_update(self):
 
-        rv = self.db.add('/', comment(text='Foo'), '')
+        rv = self.db.add('/', Comment(text='Foo'), '')
         time.sleep(0.1)
-        rv = self.db.update('/', rv.id, comment(text='Bla'))
-        c = self.db.get('/', rv.id)
+        rv = self.db.update('/', rv["id"], {"text": "Bla"})
+        c = self.db.get('/', rv["id"])
 
-        assert c.id == 1
-        assert c.text == 'Bla'
-        assert c.created < c.modified
+        assert c["id"] == 1
+        assert c["text"] == 'Bla'
+        assert c["created"] < c["modified"]
 
     def test_delete(self):
 
-        rv = self.db.add('/', comment(
+        rv = self.db.add('/', Comment(
             text='F**CK', author='P*NIS', website='http://somebadhost.org/'), '')
-        assert self.db.delete('/', rv.id) == None
+        assert self.db.delete('/', rv["id"]) == None
 
     def test_recent(self):
 
-        self.db.add('/path/', comment(text='2'), '')
+        self.db.add('/path/', Comment(text='2'), '')
 
         for x in range(5):
-            self.db.add('/', comment(text='%i' % (x+1)), '')
+            self.db.add('/', Comment(text='%i' % (x+1)), '')
 
         assert len(list(self.db.recent(mode=7))) == 6
         assert len(list(self.db.recent(mode=7, limit=5))) == 5
@@ -95,13 +87,13 @@ class TestSQLitePending(unittest.TestCase):
 
     def test_retrieve(self):
 
-        self.db.add('/', comment(text='Foo'), '')
+        self.db.add('/', Comment(text='Foo'), '')
         assert len(list(self.db.retrieve('/'))) == 0
 
     def test_activate(self):
 
-        self.db.add('/', comment(text='Foo'), '')
-        self.db.add('/', comment(text='Bar'), '')
+        self.db.add('/', Comment(text='Foo'), '')
+        self.db.add('/', Comment(text='Bar'), '')
         self.db.activate('/', 2)
 
         assert len(list(self.db.retrieve('/'))) == 1

specs/test_vote.py

@@ -1,4 +1,6 @@
 
+from __future__ import unicode_literals
+
 import os
 import json
 import tempfile