implement the paging/sorting in data

+ make all test pass
+ simplify code linked to the buildbod_www package
+ create more tests for paging/sorting
+ more docs on js tests

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>

master/buildbot/data/base.py

@@ -13,7 +13,7 @@
 #
 # Copyright Buildbot Team Members
 from buildbot.data import exceptions
-
+from twisted.internet import defer
 class ResourceType(object):
     type = None
     endpoints = []
@@ -54,7 +54,30 @@ def control(self, action, args, kwargs):
     def startConsuming(self, callback, options, kwargs):
         raise NotImplementedError
 
+class GetParamsCheckMixin(object):
+    """ a mixin for making generic paramater checks for the get data api"""
+    maximumCount = 0
+    def get(self, options, kwargs):
+        """generic tests for get options
+           currently only test the count is not too large to avoid dos
+        """
+        if "count" in options:
+            try:
+                options["count"] = int(options["count"])
+            except:
+                return defer.fail(exceptions.InvalidOptionException(
+                        "count need to be integer %s"%(
+                            options["count"])))
+            if self.maximumCount > 0 and self.maximumCount < options["count"]:
+                return defer.fail(exceptions.InvalidOptionException(
+                        "to many element requested: %d > %d"%(
+                            options["count"], self.maximumCount)))
+        return self.safeGet(options, kwargs)
+    def safeGet(self, options, kwargs):
+        raise NotImplementedError
+
 class ControlParamsCheckMixin(object):
+    """ a mixin for making generic paramater checks for the control data api"""
     action_specs = {}
     def control(self, action, args, kwargs):
         if not action in self.action_specs:

master/buildbot/data/changes.py

@@ -16,7 +16,7 @@
 from twisted.internet import defer
 from twisted.python import log
 import datetime
-from buildbot.data import base, exceptions
+from buildbot.data import base
 from buildbot.process import metrics
 from buildbot.process.users import users
 from buildbot.util import datetime2epoch, epoch2datetime
@@ -31,23 +31,16 @@ def get(self, options, kwargs):
         return d
 
 
-class ChangesEndpoint(base.Endpoint):
+class ChangesEndpoint(base.GetParamsCheckMixin,base.Endpoint,):
 
     pathPattern = ( 'change', )
-    rootLinkName = 'changes'
-
-    def get(self, options, kwargs):
-        try:
-            count = min(int(options.get('count', '50')), 50)
-        except:
-            return defer.fail(
-                    exceptions.InvalidOptionException('invalid count option'))
-        d = self.master.db.changes.getRecentChanges(count)
-        @d.addCallback
-        def sort(changes):
-            changes.sort(key=lambda chdict : chdict['changeid'])
-            return map(_fixChange, changes)
-        return d
+    rootLinkName = 'change'
+    maximumCount = 50
+    @defer.inlineCallbacks
+    def safeGet(self, options, kwargs):
+        options['total'] = yield self.master.db.changes.getChangesCount(options)
+        changes = yield self.master.db.changes.getChanges(options)
+        defer.returnValue(map(_fixChange, changes))
 
     def startConsuming(self, callback, options, kwargs):
         return self.master.mq.startConsuming(callback,

master/buildbot/data/masters.py

@@ -47,15 +47,15 @@ def get(self, options, kwargs):
         yield defer.returnValue(_db2data(m) if m else None)
 
 
-class MastersEndpoint(base.Endpoint):
+class MastersEndpoint(base.GetParamsCheckMixin, base.Endpoint):
 
     pathPatterns = [ ( 'master', ),
                      ( 'builder', 'i:builderid', 'master', ) ]
     rootLinkName = 'masters'
 
     @defer.inlineCallbacks
-    def get(self, options, kwargs):
-        masterlist = yield self.master.db.masters.getMasters()
+    def safeGet(self, options, kwargs):
+        masterlist = yield self.master.db.masters.getMasters(options)
         # filter by builder if requested
         if 'builderid' in kwargs:
             builder = yield self.master.db.builders.getBuilder(

master/buildbot/db/base.py

@@ -23,7 +23,7 @@ class DBConnectorComponent(object):
     # of the necessary backlinks and other housekeeping.
 
     connector = None
-
+    data2db = {}
     def __init__(self, connector):
         self.db = connector
 
@@ -84,7 +84,21 @@ def thd(conn, no_recurse=False):
                 return thd(conn, no_recurse=True)
         return self.db.pool.do(thd)
 
-
+    def dataOptionsToSelectOptions(self, opts):
+        r = {}
+        if 'start' in opts and opts['start'] != 0:
+            r["offset"] = int(opts['start'])
+        if 'count' in opts and opts['count'] != 0:
+            r["limit"] = int(opts['count'])
+        if 'sort' in opts and opts['sort'] != "":
+            def convert((k,r)):
+                if k in self.data2db:
+                    k = self.data2db[k]
+                if r:
+                    return sa.desc(k)
+                return sa.asc(k)
+            r["order_by"] = map(convert,opts['sort'])
+        return r
 class CachedMethod(object):
     def __init__(self, cache_name, method):
         self.cache_name = cache_name

master/buildbot/db/changes.py

@@ -158,6 +158,38 @@ def get_changes(changeids):
         d.addCallback(get_changes)
         return d
 
+    def getChanges(self, opts={}):
+        def thd(conn):
+            # get the changeids from the 'changes' table
+            changes_tbl = self.db.model.changes
+            q = sa.select([changes_tbl.c.changeid],
+                          **self.dataOptionsToSelectOptions(opts))
+            rp = conn.execute(q)
+            changeids = [ row.changeid for row in rp ]
+            rp.close()
+            return list(changeids)
+        d = self.db.pool.do(thd)
+
+        # then turn those into changes, using the cache
+        def get_changes(changeids):
+            return defer.gatherResults([ self.getChange(changeid)
+                                         for changeid in changeids ])
+        d.addCallback(get_changes)
+        return d
+
+    def getChangesCount(self, opts):
+        def thd(conn):
+            changes_tbl = self.db.model.changes
+            q = sa.select([changes_tbl.c.changeid]).count()
+            rp = conn.execute(q)
+            r = 0
+            for row in rp:
+                r = row[0]
+            rp.close()
+            return int(r)
+        d = self.db.pool.do(thd)
+        return d
+
     def getLatestChangeid(self):
         def thd(conn):
             changes_tbl = self.db.model.changes

master/buildbot/db/masters.py

@@ -23,6 +23,7 @@ class MasterDict(dict):
     pass
 
 class MastersConnectorComponent(base.DBConnectorComponent):
+    data2db = { "masterid":"id", "link":"id"}
 
     def findMasterId(self, name, _reactor=reactor):
         tbl=self.db.model.masters
@@ -80,12 +81,12 @@ def thd(conn):
             return rv
         return self.db.pool.do(thd)
 
-    def getMasters(self):
+    def getMasters(self, opts={}):
         def thd(conn):
             tbl = self.db.model.masters
             return [
                 self._masterdictFromRow(row)
-                for row in conn.execute(tbl.select()).fetchall() ]
+                for row in conn.execute(tbl.select(**self.dataOptionsToSelectOptions(opts))).fetchall() ]
         return self.db.pool.do(thd)
 
     def _masterdictFromRow(self, row):

master/buildbot/scripts/uitestserver.py

@@ -15,23 +15,14 @@
 
 
 import webbrowser,os
-from buildbot.scripts import base
 from twisted.internet import reactor, defer
 from buildbot.test.fake import fakemaster
 
 @defer.inlineCallbacks
 def _uitestserver(config):
-    if not base.isBuildmasterDir(config['basedir']):
-        print "not a buildmaster directory"
-        reactor.stop()
-        raise defer.returnValue(1)
-    public_html = os.path.join(config['basedir'],"public_html")
-    if not os.path.isdir(public_html):
-        print "buildmaster directory, must contain configured public_html directory"
-        reactor.stop()
-        raise defer.returnValue(1)
-    master = yield fakemaster.make_master_for_uitest(int(config['port']), public_html)
-    webbrowser.open(master.config.www['url']+"app/base/bb/tests/runner.html")
+    master = yield fakemaster.make_master_for_uitest(int(config['port']))
+    if "DISPLAY" in os.environ:
+        webbrowser.open(master.config.www['url']+"app/base/bb/tests/runner.html")
 
 def uitestserver(config):
     def async():

master/buildbot/test/fake/fakedb.py

@@ -23,6 +23,7 @@
 import random
 import base64
 import hashlib
+from operator import itemgetter
 from buildbot.util import json, epoch2datetime, datetime2epoch
 from twisted.python import failure
 from twisted.internet import defer, reactor
@@ -394,12 +395,24 @@ class BuilderMaster(Row):
 # Fake DB Components
 
 class FakeDBComponent(object):
+    data2db = { }
 
     def __init__(self, db, testcase):
         self.db = db
         self.t = testcase
         self.setUp()
 
+    def applyDataOptions(self, l, opts):
+        if 'sort' in opts:
+            for k,r in reversed(opts['sort']):
+                if k in self.data2db:
+                    k = self.data2db[k]
+                l.sort(key=itemgetter(k), reverse = r)
+        if 'start' in opts and opts['start'] != 0:
+            l = l[int(opts['start']):]
+        if 'count' in opts and opts['count'] != 0:
+            l = l[:int(opts['count'])]
+        return l
 
 class FakeChangesComponent(FakeDBComponent):
 
@@ -485,9 +498,14 @@ def getChangeUids(self, changeid):
     def getRecentChanges(self, count):
         ids = sorted(self.changes.keys())
         chdicts = [ self._chdict(self.changes[id]) for id in ids[-count:] ]
-        random.shuffle(chdicts) # since order is not documented
         return defer.succeed(chdicts)
 
+    def getChanges(self, opts={}):
+        chdicts = [ self._chdict(v) for v in self.changes.values() ]
+        return defer.succeed(self.applyDataOptions(chdicts,opts))
+    def getChangesCount(self, opts={}):
+        return len(self.changes)
+
     def _chdict(self, row):
         chdict = row.copy()
         del chdict['uids']
@@ -1311,6 +1329,7 @@ def identifierToUid(self, identifier):
 
 class FakeMastersComponent(FakeDBComponent):
 
+    data2db = { "masterid":"id", "link":"id"}
     def setUp(self):
         self.masters = {}
 
@@ -1351,8 +1370,8 @@ def getMaster(self, masterid):
             return defer.succeed(self.masters[masterid])
         return defer.succeed(None)
 
-    def getMasters(self):
-        return defer.succeed(self.masters.values())
+    def getMasters(self, opts = {}):
+        return defer.succeed(self.applyDataOptions(self.masters.values(), opts))
 
     # test helpers
 

master/buildbot/test/fake/fakemaster.py

@@ -137,7 +137,7 @@ def make_master(wantMq=False, wantDb=False, wantData=False,
 
 # this config has real mq, real data, real www, but fakedb, and no build engine for ui test
 @defer.inlineCallbacks
-def make_master_for_uitest(port, public_html):
+def make_master_for_uitest(port):
     tcp = reactor.listenTCP(port, ServerFactory())
     port = tcp._realPortNumber
     yield tcp.stopListening()
@@ -151,7 +151,7 @@ class testHookedDataConnector(dataconnector.DataConnector):
         submodules = dataconnector.DataConnector.submodules + ['buildbot.data.testhooks']
 
     master.data = testHookedDataConnector(master)
-    master.config.www = dict(url=url, port=port, public_html=public_html)
+    master.config.www = dict(url=url, port=port)
     master.www = service.WWWService(master)
     master.data.updates.playTestScenario("buildbot.test.scenarios.base.BaseScenario.populateBaseDb")
     yield master.www.startService()

master/buildbot/test/scenarios/base.py

@@ -42,7 +42,7 @@ def stopMaster(self):
         return self.master.data.updates.masterStopped(name=u'master', masterid=14)
     @defer.inlineCallbacks
     def addChanges(self):
-        for rev in xrange(1,2000):
+        for rev in xrange(1,200):
             yield self.master.data.updates.addChange(
                 author=u'warner', branch=u'warnerdb',
                 category=u'devel', comments=u'fix whitespace',

master/buildbot/test/unit/test_data_changes.py

@@ -91,12 +91,36 @@ def test_get_fewer(self):
         def check(changes):
             self.assertEqual(len(changes), 1)
             types.verifyData(self, 'change', {}, changes[0])
+            self.assertEqual(changes[0]['changeid'], 13)
+        return d
+    def test_get_paging(self):
+        d = self.callGet(dict(count='1',start=1), dict())
+        @d.addCallback
+        def check(changes):
+            self.assertEqual(len(changes), 1)
+            types.verifyData(self, 'change', {}, changes[0])
+            self.assertEqual(changes[0]['changeid'], 14)
         return d
 
     def test_get_invalid_count(self):
         d = self.callGet(dict(count='ten'), dict())
         self.assertFailure(d, exceptions.InvalidOptionException)
 
+    def test_get_invalid_count2(self):
+        d = self.callGet(dict(count=50000), dict())
+        self.assertFailure(d, exceptions.InvalidOptionException)
+
+    def test_get_sorted(self):
+        # invert the default order
+        d = self.callGet(dict(sort=[('changeid',1)]), dict())
+        @d.addCallback
+        def check(changes):
+            types.verifyData(self, 'change', {}, changes[1])
+            self.assertEqual(changes[1]['changeid'], 13)
+            types.verifyData(self, 'change', {}, changes[0])
+            self.assertEqual(changes[0]['changeid'], 14)
+        return d
+
     def test_startConsuming(self):
         self.callStartConsuming({}, {},
                 expected_filter=('change', None, 'new'))