Merge pull request buildbot#7607 from tdesveaux/typing/db/buildrequests

typing: convert db.buildrequests.BrDict to dataclass

common/code_spelling_ignore_words.txt

@@ -204,6 +204,7 @@ buildrequestid
 buildrequestresults
 buildrequests
 buildrequestsconnectorcomponent
+buildrequestmodel
 buildresult
 buildroot
 build's

master/buildbot/data/buildrequests.py

@@ -13,6 +13,10 @@
 #
 # Copyright Buildbot Team Members
 
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
 from twisted.internet import defer
 
 from buildbot.data import base
@@ -21,51 +25,56 @@
 from buildbot.db.buildrequests import NotClaimedError
 from buildbot.process.results import RETRY
 
+if TYPE_CHECKING:
+    from typing import Sequence
+
+    from buildbot.data.resultspec import ResultSpec
+    from buildbot.db.buildrequests import BuildRequestModel
+
+
+def _db2data(dbmodel: BuildRequestModel, properties: dict | None):
+    return {
+        'buildrequestid': dbmodel.buildrequestid,
+        'buildsetid': dbmodel.buildsetid,
+        'builderid': dbmodel.builderid,
+        'priority': dbmodel.priority,
+        'claimed': dbmodel.claimed,
+        'claimed_at': dbmodel.claimed_at,
+        'claimed_by_masterid': dbmodel.claimed_by_masterid,
+        'complete': dbmodel.complete,
+        'results': dbmodel.results,
+        'submitted_at': dbmodel.submitted_at,
+        'complete_at': dbmodel.complete_at,
+        'waited_for': dbmodel.waited_for,
+        'properties': properties,
+    }
 
-class Db2DataMixin:
-    def _generate_filtered_properties(self, props, filters):
-        """
-        This method returns Build's properties according to property filters.
-
-        :param props: Properties as a dict (from db)
-        :param filters: Desired properties keys as a list (from API URI)
-        """
-        # by default no properties are returned
-        if props and filters:
-            return (
-                props
-                if '*' in filters
-                else dict(((k, v) for k, v in props.items() if k in filters))
-            )
+
+def _generate_filtered_properties(props: dict, filters: Sequence) -> dict | None:
+    """
+    This method returns Build's properties according to property filters.
+
+    :param props: Properties as a dict (from db)
+    :param filters: Desired properties keys as a list (from API URI)
+    """
+    # by default no properties are returned
+    if not props and not filters:
         return None
 
+    set_filters = set(filters)
+    if '*' in set_filters:
+        return props
+
+    return {k: v for k, v in props.items() if k in set_filters}
+
+
+class Db2DataMixin:
     @defer.inlineCallbacks
-    def addPropertiesToBuildRequest(self, buildrequest, filters):
+    def get_buildset_properties_filtered(self, buildsetid: int, filters: Sequence):
         if not filters:
             return None
-        props = yield self.master.db.buildsets.getBuildsetProperties(buildrequest['buildsetid'])
-        filtered_properties = self._generate_filtered_properties(props, filters)
-        if filtered_properties:
-            buildrequest['properties'] = filtered_properties
-        return None
-
-    def db2data(self, dbdict):
-        data = {
-            'buildrequestid': dbdict['buildrequestid'],
-            'buildsetid': dbdict['buildsetid'],
-            'builderid': dbdict['builderid'],
-            'priority': dbdict['priority'],
-            'claimed': dbdict['claimed'],
-            'claimed_at': dbdict['claimed_at'],
-            'claimed_by_masterid': dbdict['claimed_by_masterid'],
-            'complete': dbdict['complete'],
-            'results': dbdict['results'],
-            'submitted_at': dbdict['submitted_at'],
-            'complete_at': dbdict['complete_at'],
-            'waited_for': dbdict['waited_for'],
-            'properties': dbdict.get('properties'),
-        }
-        return defer.succeed(data)
+        props = yield self.master.db.buildsets.getBuildsetProperties(buildsetid)
+        return _generate_filtered_properties(props, filters)
 
     fieldMapping = {
         'buildrequestid': 'buildrequests.id',
@@ -90,14 +99,14 @@ class BuildRequestEndpoint(Db2DataMixin, base.Endpoint):
     """
 
     @defer.inlineCallbacks
-    def get(self, resultSpec, kwargs):
+    def get(self, resultSpec: ResultSpec, kwargs):
         buildrequest = yield self.master.db.buildrequests.getBuildRequest(kwargs['buildrequestid'])
+        if not buildrequest:
+            return None
 
-        if buildrequest:
-            filters = resultSpec.popProperties() if hasattr(resultSpec, 'popProperties') else []
-            yield self.addPropertiesToBuildRequest(buildrequest, filters)
-            return (yield self.db2data(buildrequest))
-        return None
+        filters = resultSpec.popProperties() if hasattr(resultSpec, 'popProperties') else []
+        properties = yield self.get_buildset_properties_filtered(buildrequest.buildsetid, filters)
+        return _db2data(buildrequest, properties)
 
     @defer.inlineCallbacks
     def set_request_priority(self, brid, args, kwargs):
@@ -153,8 +162,8 @@ def get(self, resultSpec, kwargs):
         results = []
         filters = resultSpec.popProperties() if hasattr(resultSpec, 'popProperties') else []
         for br in buildrequests:
-            yield self.addPropertiesToBuildRequest(br, filters)
-            results.append((yield self.db2data(br)))
+            properties = yield self.get_buildset_properties_filtered(br.buildsetid, filters)
+            results.append(_db2data(br, properties))
         return results
 
 
@@ -251,7 +260,7 @@ def completeBuildRequests(self, brids, results, complete_at=None):
             brdict = yield self.master.db.buildrequests.getBuildRequest(brid)
 
             if brdict:
-                bsid = brdict['buildsetid']
+                bsid = brdict.buildsetid
                 if bsid in seen_bsids:
                     continue
                 seen_bsids.add(bsid)

master/buildbot/data/buildsets.py

@@ -226,7 +226,7 @@ def maybeBuildsetComplete(self, bsid):
         # figure out the overall results of the buildset:
         cumulative_results = SUCCESS
         for brdict in brdicts:
-            cumulative_results = worst_status(cumulative_results, brdict['results'])
+            cumulative_results = worst_status(cumulative_results, brdict.results)
 
         # get a copy of the buildset
         bsdict = yield self.master.db.buildsets.getBuildset(bsid)

master/buildbot/data/masters.py

@@ -170,7 +170,7 @@ def _masterDeactivatedHousekeeping(self, masterid, name):
             complete=False, claimed=masterid
         )
         yield self.master.db.buildrequests.unclaimBuildRequests(
-            brids=[br['buildrequestid'] for br in buildrequests]
+            brids=[br.buildrequestid for br in buildrequests]
         )
 
     @defer.inlineCallbacks

master/buildbot/data/resultspec.py

@@ -13,6 +13,8 @@
 #
 # Copyright Buildbot Team Members
 
+from __future__ import annotations
+
 import sqlalchemy as sa
 from twisted.python import log
 
@@ -71,10 +73,12 @@ class FieldBase:
         # only support string values, because currently there are no queries against lists in SQL
     }
 
-    def __init__(self, field, op, values):
+    # can't type `values` as `Sequence` as `str` is one as well...
+    def __init__(self, field: bytes, op: str, values: list | tuple):
         self.field = field
         self.op = op
         self.values = values
+        assert isinstance(values, (list, tuple))
 
     def getOperator(self, sqlMode=False):
         v = self.values
@@ -419,8 +423,7 @@ def keyFunc(elem, order=self.order):
                     Do a multi-level sort by passing in the keys
                     to sort by.
 
-                    @param elem: each item in the list to sort.  It must be
-                              a C{dict}
+                    @param elem: each item in the list to sort.
                     @param order: a list of keys to sort by, such as:
                                 ('lastName', 'firstName', 'age')
                     @return: a key used by sorted(). This will be a
@@ -436,7 +439,8 @@ def keyFunc(elem, order=self.order):
                             # it means sort by 'lastName' in reverse.
                             k = k[1:]
                             doReverse = True
-                        val = NoneComparator(elem[k])
+                        val = elem[k] if isinstance(elem, dict) else getattr(elem, k)
+                        val = NoneComparator(val)
                         if doReverse:
                             val = ReverseComparator(val)
                         compareKey.append(val)

master/buildbot/db/buildrequests.py

@@ -13,18 +13,27 @@
 #
 # Copyright Buildbot Team Members
 
+from __future__ import annotations
 
 import itertools
+from dataclasses import dataclass
+from typing import TYPE_CHECKING
 
 import sqlalchemy as sa
 from twisted.internet import defer
+from twisted.python import deprecate
 from twisted.python import log
+from twisted.python import versions
 
 from buildbot.db import NULL
 from buildbot.db import base
 from buildbot.process.results import RETRY
 from buildbot.util import datetime2epoch
 from buildbot.util import epoch2datetime
+from buildbot.warnings import warn_deprecated
+
+if TYPE_CHECKING:
+    import datetime
 
 
 class AlreadyClaimedError(Exception):
@@ -35,7 +44,46 @@ class NotClaimedError(Exception):
     pass
 
 
-class BrDict(dict):
+@dataclass
+class BuildRequestModel:
+    buildrequestid: int
+    buildsetid: int
+    builderid: int
+    buildername: str
+    submitted_at: datetime.datetime
+    complete_at: datetime.datetime | None = None
+    complete: bool = False
+    results: int | None = None
+    waited_for: bool = False
+    priority: int = 0
+
+    claimed_at: datetime.datetime | None = None
+    claimed_by_masterid: int | None = None
+
+    @property
+    def claimed(self) -> bool:
+        return self.claimed_at is not None
+
+    # For backward compatibility from when SsDict inherited from Dict
+    def __getitem__(self, key: str):
+        warn_deprecated(
+            '3.12.0',
+            (
+                'BuildRequestsConnectorComponent '
+                'getBuildRequest, and getBuildRequests '
+                'no longer return BuildRequest as dictionnaries. '
+                'Usage of [] accessor is deprecated: please access the member directly'
+            ),
+        )
+
+        if hasattr(self, key):
+            return getattr(self, key)
+
+        raise KeyError(key)
+
+
+@deprecate.deprecated(versions.Version("buildbot", 3, 12, 0), BuildRequestModel)
+class BrDict(BuildRequestModel):
     pass
 
 
@@ -63,17 +111,16 @@ def _saSelectQuery(self):
             builder_tbl.c.name.label('buildername'),
         ).select_from(from_clause)
 
-    # returns a Deferred that returns a value
-    def getBuildRequest(self, brid):
-        def thd(conn):
+    def getBuildRequest(self, brid) -> defer.Deferred[BuildRequestModel | None]:
+        def thd(conn) -> BuildRequestModel | None:
             reqs_tbl = self.db.model.buildrequests
             q = self._saSelectQuery()
             q = q.where(reqs_tbl.c.id == brid)
             res = conn.execute(q)
             row = res.fetchone()
             rv = None
             if row:
-                rv = self._brdictFromRow(row, self.db.master.masterid)
+                rv = self._modelFromRow(row)
             res.close()
             return rv
 
@@ -90,10 +137,10 @@ def getBuildRequests(
         repository=None,
         resultSpec=None,
     ):
-        def deduplicateBrdict(brdicts):
-            return list(({b['buildrequestid']: b for b in brdicts}).values())
+        def deduplicateBrdict(brdicts: list[BuildRequestModel]) -> list[BuildRequestModel]:
+            return list(({b.buildrequestid: b for b in brdicts}).values())
 
-        def thd(conn):
+        def thd(conn) -> list[BuildRequestModel]:
             reqs_tbl = self.db.model.buildrequests
             claims_tbl = self.db.model.buildrequest_claims
             sstamps_tbl = self.db.model.sourcestamps
@@ -122,17 +169,11 @@ def thd(conn):
                 q = q.where(sstamps_tbl.c.repository == repository)
 
             if resultSpec is not None:
-                return deduplicateBrdict(
-                    resultSpec.thd_execute(
-                        conn, q, lambda r: self._brdictFromRow(r, self.db.master.masterid)
-                    )
-                )
+                return deduplicateBrdict(resultSpec.thd_execute(conn, q, self._modelFromRow))
 
             res = conn.execute(q)
 
-            return deduplicateBrdict([
-                self._brdictFromRow(row, self.db.master.masterid) for row in res.fetchall()
-            ])
+            return deduplicateBrdict([self._modelFromRow(row) for row in res.fetchall()])
 
         res = yield self.db.pool.do(thd)
         return res
@@ -165,8 +206,7 @@ def thd(conn):
 
         yield self.db.pool.do(thd)
 
-    # returns a Deferred that returns None
-    def unclaimBuildRequests(self, brids):
+    def unclaimBuildRequests(self, brids) -> defer.Deferred[None]:
         def thd(conn):
             transaction = conn.begin()
             claims_tbl = self.db.model.buildrequest_claims
@@ -264,31 +304,18 @@ def thd(conn):
         return self.db.pool.do(thd)
 
     @staticmethod
-    def _brdictFromRow(row, master_masterid):
-        claimed = False
-        claimed_by_masterid = None
-        claimed_at = None
-        if row.claimed_at is not None:
-            claimed_at = row.claimed_at
-            claimed = True
-            claimed_by_masterid = row.masterid
-
-        submitted_at = epoch2datetime(row.submitted_at)
-        complete_at = epoch2datetime(row.complete_at)
-        claimed_at = epoch2datetime(claimed_at)
-
-        return BrDict(
+    def _modelFromRow(row):
+        return BuildRequestModel(
             buildrequestid=row.id,
             buildsetid=row.buildsetid,
             builderid=row.builderid,
             buildername=row.buildername,
-            priority=row.priority,
-            claimed=claimed,
-            claimed_at=claimed_at,
-            claimed_by_masterid=claimed_by_masterid,
+            submitted_at=epoch2datetime(row.submitted_at),
+            complete_at=epoch2datetime(row.complete_at),
             complete=bool(row.complete),
             results=row.results,
-            submitted_at=submitted_at,
-            complete_at=complete_at,
             waited_for=bool(row.waited_for),
+            priority=row.priority,
+            claimed_at=epoch2datetime(row.claimed_at),
+            claimed_by_masterid=row.masterid,
         )