From 627b8232c857cc85a45f0698d084eeba854e1aca Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 13 May 2024 20:58:39 +0200 Subject: [PATCH 1/4] data: make ResultSpec ordering work with non-dict elements --- master/buildbot/data/resultspec.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/master/buildbot/data/resultspec.py b/master/buildbot/data/resultspec.py index 85761daa7f7b..832f5b5f64f4 100644 --- a/master/buildbot/data/resultspec.py +++ b/master/buildbot/data/resultspec.py @@ -419,8 +419,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 +435,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) From f7206c38ab0d75c4847637140bb3c6bfba89e769 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 13 May 2024 23:11:00 +0200 Subject: [PATCH 2/4] data: Fix resultspec.Property intantiated with a str values in tests Added an assert to resultspec.FieldBase.__init__ to prevent this. --- master/buildbot/data/resultspec.py | 6 +++++- master/buildbot/test/unit/data/test_buildrequests.py | 6 +++--- master/buildbot/test/unit/data/test_builds.py | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/master/buildbot/data/resultspec.py b/master/buildbot/data/resultspec.py index 832f5b5f64f4..447e2984025c 100644 --- a/master/buildbot/data/resultspec.py +++ b/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 diff --git a/master/buildbot/test/unit/data/test_buildrequests.py b/master/buildbot/test/unit/data/test_buildrequests.py index a9eaf7182598..fdf574bbaad9 100644 --- a/master/buildbot/test/unit/data/test_buildrequests.py +++ b/master/buildbot/test/unit/data/test_buildrequests.py @@ -98,7 +98,7 @@ def testGetMissing(self): @defer.inlineCallbacks def testGetProperty(self): - prop = resultspec.Property(b'property', 'eq', 'prop1') + prop = resultspec.Property(b'property', 'eq', ['prop1']) buildrequest = yield self.callGet( ('buildrequests', 44), resultSpec=resultspec.ResultSpec(properties=[prop]) ) @@ -107,7 +107,7 @@ def testGetProperty(self): @defer.inlineCallbacks def testGetProperties(self): - prop = resultspec.Property(b'property', 'eq', '*') + prop = resultspec.Property(b'property', 'eq', ['*']) buildrequest = yield self.callGet( ('buildrequests', 44), resultSpec=resultspec.ResultSpec(properties=[prop]) ) @@ -190,7 +190,7 @@ def testGetProperties(self): buildsetid=8822, property_name='prop2', property_value='["two", "fake2"]' ), ]) - prop = resultspec.Property(b'property', 'eq', '*') + prop = resultspec.Property(b'property', 'eq', ['*']) buildrequests = yield self.callGet( ('builders', 78, 'buildrequests'), resultSpec=resultspec.ResultSpec(properties=[prop]) ) diff --git a/master/buildbot/test/unit/data/test_builds.py b/master/buildbot/test/unit/data/test_builds.py index 7d5eea8c0fe8..61dfc44f5c21 100644 --- a/master/buildbot/test/unit/data/test_builds.py +++ b/master/buildbot/test/unit/data/test_builds.py @@ -99,7 +99,7 @@ def test_get_buildername_not_existing_number(self): @defer.inlineCallbacks def test_properties_injection(self): resultSpec = resultspec.OptimisedResultSpec( - properties=[resultspec.Property(b'property', 'eq', 'reason')] + properties=[resultspec.Property(b'property', 'eq', ['reason'])] ) build = yield self.callGet(('builders', 77, 'builds', 3), resultSpec=resultSpec) self.validateData(build) @@ -285,7 +285,7 @@ def test_get_complete_at(self): @defer.inlineCallbacks def test_properties_injection(self): resultSpec = resultspec.OptimisedResultSpec( - properties=[resultspec.Property(b'property', 'eq', 'reason')] + properties=[resultspec.Property(b'property', 'eq', ['reason'])] ) builds = yield self.callGet(('builds',), resultSpec=resultSpec) From da5b5c62d67a419df4055cacad12d8e5c803d424 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 13 May 2024 23:12:57 +0200 Subject: [PATCH 3/4] data: minor clean to buildrequests.Db2DataMixin --- master/buildbot/data/buildrequests.py | 104 ++++++++++++++------------ 1 file changed, 57 insertions(+), 47 deletions(-) diff --git a/master/buildbot/data/buildrequests.py b/master/buildbot/data/buildrequests.py index 92ee4c9ca397..d745e2534801 100644 --- a/master/buildbot/data/buildrequests.py +++ b/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,55 @@ 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 + + +def _db2data(dbdict: dict, properties: dict | None): + return { + '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': 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 +98,16 @@ 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 +163,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 From 8e9df31ddee62b77997e6d771a60dcda68f59b10 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 13 May 2024 20:39:33 +0200 Subject: [PATCH 4/4] db: Add BuildRequestsModel dataclass --- common/code_spelling_ignore_words.txt | 1 + master/buildbot/data/buildrequests.py | 35 +++--- master/buildbot/data/buildsets.py | 2 +- master/buildbot/data/masters.py | 2 +- master/buildbot/db/buildrequests.py | 103 +++++++++++------- master/buildbot/process/buildrequest.py | 37 ++++--- .../process/buildrequestdistributor.py | 35 +++++- .../reporters/generators/buildrequest.py | 2 +- master/buildbot/reporters/utils.py | 19 +++- master/buildbot/steps/trigger.py | 2 +- master/buildbot/test/fakedb/buildrequests.py | 36 +++--- master/buildbot/test/fakedb/builds.py | 4 +- master/buildbot/test/fakedb/sourcestamps.py | 2 +- .../test/unit/db/test_buildrequests.py | 57 ++++------ .../test/unit/process/test_buildrequest.py | 13 +-- .../test/unit/schedulers/test_triggerable.py | 30 ++--- .../docs/developer/database/buildrequests.rst | 9 +- master/docs/spelling_wordlist.txt | 1 + newsfragments/db-brdict.removal | 1 + .../db-introduce-BuildRequestModel.change | 1 + 20 files changed, 225 insertions(+), 167 deletions(-) create mode 100644 newsfragments/db-brdict.removal create mode 100644 newsfragments/db-introduce-BuildRequestModel.change diff --git a/common/code_spelling_ignore_words.txt b/common/code_spelling_ignore_words.txt index c40a4882f439..291635eb6053 100644 --- a/common/code_spelling_ignore_words.txt +++ b/common/code_spelling_ignore_words.txt @@ -204,6 +204,7 @@ buildrequestid buildrequestresults buildrequests buildrequestsconnectorcomponent +buildrequestmodel buildresult buildroot build's diff --git a/master/buildbot/data/buildrequests.py b/master/buildbot/data/buildrequests.py index d745e2534801..c66894cdb166 100644 --- a/master/buildbot/data/buildrequests.py +++ b/master/buildbot/data/buildrequests.py @@ -29,22 +29,23 @@ from typing import Sequence from buildbot.data.resultspec import ResultSpec + from buildbot.db.buildrequests import BuildRequestModel -def _db2data(dbdict: dict, properties: dict | None): +def _db2data(dbmodel: BuildRequestModel, properties: dict | None): return { - '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'], + '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, } @@ -104,9 +105,7 @@ def get(self, resultSpec: ResultSpec, kwargs): return None filters = resultSpec.popProperties() if hasattr(resultSpec, 'popProperties') else [] - properties = yield self.get_buildset_properties_filtered( - buildrequest['buildsetid'], filters - ) + properties = yield self.get_buildset_properties_filtered(buildrequest.buildsetid, filters) return _db2data(buildrequest, properties) @defer.inlineCallbacks @@ -163,7 +162,7 @@ def get(self, resultSpec, kwargs): results = [] filters = resultSpec.popProperties() if hasattr(resultSpec, 'popProperties') else [] for br in buildrequests: - properties = yield self.get_buildset_properties_filtered(br['buildsetid'], filters) + properties = yield self.get_buildset_properties_filtered(br.buildsetid, filters) results.append(_db2data(br, properties)) return results @@ -261,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) diff --git a/master/buildbot/data/buildsets.py b/master/buildbot/data/buildsets.py index dcc19801f928..b114bef66081 100644 --- a/master/buildbot/data/buildsets.py +++ b/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) diff --git a/master/buildbot/data/masters.py b/master/buildbot/data/masters.py index 5e034e7ec61a..b4c27de2fec7 100644 --- a/master/buildbot/data/masters.py +++ b/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 diff --git a/master/buildbot/db/buildrequests.py b/master/buildbot/db/buildrequests.py index f5064b1ef2db..906f4340b5c3 100644 --- a/master/buildbot/db/buildrequests.py +++ b/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,9 +111,8 @@ 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) @@ -73,7 +120,7 @@ def thd(conn): 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, ) diff --git a/master/buildbot/process/buildrequest.py b/master/buildbot/process/buildrequest.py index 8df4a25a220a..3920a30764a2 100644 --- a/master/buildbot/process/buildrequest.py +++ b/master/buildbot/process/buildrequest.py @@ -13,7 +13,10 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + import calendar +from typing import TYPE_CHECKING from twisted.internet import defer @@ -21,6 +24,9 @@ from buildbot.process import properties from buildbot.process.results import SKIPPED +if TYPE_CHECKING: + from buildbot.db.buildrequests import BuildRequestModel + class BuildRequestCollapser: # brids is a list of the new added buildrequests id @@ -63,12 +69,14 @@ def collapse(self): bldrdict = yield self.master.data.get(('builders', builderid)) # Get the builder object bldr = self.master.botmaster.builders.get(bldrdict['name']) + if not bldr: + continue # Get the Collapse BuildRequest function (from the configuration) - collapseRequestsFn = bldr.getCollapseRequestsFn() if bldr else None + collapseRequestsFn = bldr.getCollapseRequestsFn() unclaim_brs = yield self._getUnclaimedBrs(builderid) # short circuit if there is no merging to do - if not collapseRequestsFn or not unclaim_brs: + if not unclaim_brs: continue for unclaim_br in unclaim_brs: @@ -193,9 +201,9 @@ class BuildRequest: sources = {} @classmethod - def fromBrdict(cls, master, brdict): + def fromBrdict(cls, master, brdict: BuildRequestModel): """ - Construct a new L{BuildRequest} from a dictionary as returned by + Construct a new L{BuildRequest} from a L{BuildRequestModel} as returned by L{BuildRequestsConnectorComponent.getBuildRequest}. This method uses a cache, which may result in return of stale objects; @@ -208,30 +216,29 @@ def fromBrdict(cls, master, brdict): @returns: L{BuildRequest}, via Deferred """ cache = master.caches.get_cache("BuildRequests", cls._make_br) - return cache.get(brdict['buildrequestid'], brdict=brdict, master=master) + return cache.get(brdict.buildrequestid, brdict=brdict, master=master) @classmethod @defer.inlineCallbacks - def _make_br(cls, brid, brdict, master): + def _make_br(cls, brid: int, brdict: BuildRequestModel, master): buildrequest = cls() buildrequest.id = brid - buildrequest.bsid = brdict['buildsetid'] - builder = yield master.db.builders.getBuilder(brdict['builderid']) - buildrequest.buildername = builder.name - buildrequest.builderid = brdict['builderid'] - buildrequest.priority = brdict['priority'] - dt = brdict['submitted_at'] + buildrequest.bsid = brdict.buildsetid + buildrequest.buildername = brdict.buildername + buildrequest.builderid = brdict.builderid + buildrequest.priority = brdict.priority + dt = brdict.submitted_at buildrequest.submittedAt = dt and calendar.timegm(dt.utctimetuple()) buildrequest.master = master - buildrequest.waitedFor = brdict['waited_for'] + buildrequest.waitedFor = brdict.waited_for # fetch the buildset to get the reason - buildset = yield master.db.buildsets.getBuildset(brdict['buildsetid']) + buildset = yield master.db.buildsets.getBuildset(brdict.buildsetid) assert buildset # schema should guarantee this buildrequest.reason = buildset['reason'] # fetch the buildset properties, and convert to Properties - buildset_properties = yield master.db.buildsets.getBuildsetProperties(brdict['buildsetid']) + buildset_properties = yield master.db.buildsets.getBuildsetProperties(brdict.buildsetid) buildrequest.properties = properties.Properties.fromDict(buildset_properties) diff --git a/master/buildbot/process/buildrequestdistributor.py b/master/buildbot/process/buildrequestdistributor.py index 7f9650f5f57a..859c808a62ff 100644 --- a/master/buildbot/process/buildrequestdistributor.py +++ b/master/buildbot/process/buildrequestdistributor.py @@ -24,6 +24,7 @@ from twisted.python.failure import Failure from buildbot.data import resultspec +from buildbot.db.buildrequests import BuildRequestModel from buildbot.process import metrics from buildbot.process.buildrequest import BuildRequest from buildbot.util import deferwaiter @@ -91,15 +92,43 @@ def _fetchUnclaimedBrdicts(self): return self.unclaimedBrdicts @defer.inlineCallbacks - def _getBuildRequestForBrdict(self, brdict): + def _getBuildRequestForBrdict(self, brdict: dict): # Turn a brdict into a BuildRequest into a brdict. This is useful # for API like 'nextBuild', which operate on BuildRequest objects. breq = self.breqCache.get(brdict['buildrequestid']) if not breq: - breq = yield BuildRequest.fromBrdict(self.master, brdict) + builder = yield self.master.data.get( + ('builders', brdict['builderid']), [resultspec.ResultSpec(fields=['name'])] + ) + if not builder: + return None + + model = BuildRequestModel( + buildrequestid=brdict['buildrequestid'], + buildsetid=brdict['buildsetid'], + builderid=brdict['builderid'], + buildername=builder['name'], + submitted_at=brdict['submitted_at'], + ) + if 'complete_at' in brdict: + model.complete_at = brdict['complete_at'] + if 'complete' in brdict: + model.complete = brdict['complete'] + if 'results' in brdict: + model.results = brdict['results'] + if 'waited_for' in brdict: + model.waited_for = brdict['waited_for'] + if 'priority' in brdict: + model.priority = brdict['priority'] + if 'claimed_at' in brdict: + model.claimed_at = brdict['claimed_at'] + if 'claimed_by_masterid' in brdict: + model.claimed_by_masterid = brdict['claimed_by_masterid'] + + breq = yield BuildRequest.fromBrdict(self.master, model) if breq: - self.breqCache[brdict['buildrequestid']] = breq + self.breqCache[model.buildrequestid] = breq return breq def _getBrdictForBuildRequest(self, breq): diff --git a/master/buildbot/reporters/generators/buildrequest.py b/master/buildbot/reporters/generators/buildrequest.py index 0afdd38d2b96..fde261f4c2ef 100644 --- a/master/buildbot/reporters/generators/buildrequest.py +++ b/master/buildbot/reporters/generators/buildrequest.py @@ -54,7 +54,7 @@ def partial_build_dict(self, master, buildrequest): props = Properties() buildrequest = yield BuildRequest.fromBrdict(master, brdict) - builder = yield master.botmaster.getBuilderById(brdict['builderid']) + builder = yield master.botmaster.getBuilderById(brdict.builderid) yield Build.setup_properties_known_before_build_starts(props, [buildrequest], builder) Build.setupBuildProperties(props, [buildrequest]) diff --git a/master/buildbot/reporters/utils.py b/master/buildbot/reporters/utils.py index d38e834da7bd..c70d21308b4c 100644 --- a/master/buildbot/reporters/utils.py +++ b/master/buildbot/reporters/utils.py @@ -13,7 +13,11 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + +import dataclasses from collections import UserList +from typing import TYPE_CHECKING from twisted.internet import defer from twisted.python import log @@ -23,6 +27,9 @@ from buildbot.process.results import RETRY from buildbot.util import flatten +if TYPE_CHECKING: + from buildbot.db.buildrequests import BuildRequestModel + @defer.inlineCallbacks def getPreviousBuild(master, build): @@ -119,15 +126,15 @@ def getDetailsForBuild( @defer.inlineCallbacks -def get_details_for_buildrequest(master, buildrequest, build): - buildset = yield master.data.get(("buildsets", buildrequest['buildsetid'])) - builder = yield master.data.get(("builders", buildrequest['builderid'])) +def get_details_for_buildrequest(master, buildrequest: BuildRequestModel, build): + buildset = yield master.data.get(("buildsets", buildrequest.buildsetid)) + builder = yield master.data.get(("builders", buildrequest.builderid)) - build['buildrequest'] = buildrequest + build['buildrequest'] = dataclasses.asdict(buildrequest) build['buildset'] = buildset - build['builderid'] = buildrequest['builderid'] + build['builderid'] = buildrequest.builderid build['builder'] = builder - build['url'] = getURLForBuildrequest(master, buildrequest['buildrequestid']) + build['url'] = getURLForBuildrequest(master, buildrequest.buildrequestid) build['results'] = None build['complete'] = False diff --git a/master/buildbot/steps/trigger.py b/master/buildbot/steps/trigger.py index ff3656844834..faea1afa5cd2 100644 --- a/master/buildbot/steps/trigger.py +++ b/master/buildbot/steps/trigger.py @@ -249,7 +249,7 @@ def _add_results(self, brid): @defer.inlineCallbacks def _is_buildrequest_complete(brid): buildrequest = yield self.master.db.buildrequests.getBuildRequest(brid) - return buildrequest['complete'] + return buildrequest.complete event = ('buildrequests', str(brid), 'complete') yield self.master.mq.waitUntilEvent(event, lambda: _is_buildrequest_complete(brid)) diff --git a/master/buildbot/test/fakedb/buildrequests.py b/master/buildbot/test/fakedb/buildrequests.py index 19b1ede10b8e..e92a817b1123 100644 --- a/master/buildbot/test/fakedb/buildrequests.py +++ b/master/buildbot/test/fakedb/buildrequests.py @@ -90,23 +90,24 @@ def insert_test_data(self, rows): # component methods @defer.inlineCallbacks - def getBuildRequest(self, brid): + def getBuildRequest(self, brid: int): row = self.reqs.get(brid) - if row: - claim_row = self.claims.get(brid, None) - if claim_row: - row.claimed_at = claim_row.claimed_at - row.claimed = True - row.masterid = claim_row.masterid - row.claimed_by_masterid = claim_row.masterid - else: - row.claimed_at = None - builder = yield self.db.builders.getBuilder(row.builderid) - row.buildername = builder.name - return self._brdictFromRow(row) - else: + if not row: return None + claim_row = self.claims.get(brid, None) + if claim_row: + row.claimed_at = claim_row.claimed_at + row.claimed = True + row.masterid = claim_row.masterid + row.claimed_by_masterid = claim_row.masterid + else: + row.claimed_at = None + row.masterid = None + builder = yield self.db.builders.getBuilder(row.builderid) + row.buildername = builder.name + return self._modelFromRow(row) + @defer.inlineCallbacks def getBuildRequests( self, @@ -135,6 +136,7 @@ def getBuildRequests( br.claimed_by_masterid = claim_row.masterid else: br.claimed_at = None + br.masterid = None if claimed is not None: if isinstance(claimed, bool): if claimed: @@ -162,7 +164,7 @@ def getBuildRequests( continue builder = yield self.db.builders.getBuilder(br.builderid) br.buildername = builder.name - rv.append(self._brdictFromRow(br)) + rv.append(self._modelFromRow(br)) if resultSpec is not None: rv = self.applyResultSpec(rv, resultSpec) return rv @@ -205,8 +207,8 @@ def completeBuildRequests(self, brids, results, complete_at=None): self.reqs[brid].complete_at = complete_at return defer.succeed(None) - def _brdictFromRow(self, row): - return buildrequests.BuildRequestsConnectorComponent._brdictFromRow(row, self.MASTER_ID) + def _modelFromRow(self, row): + return buildrequests.BuildRequestsConnectorComponent._modelFromRow(row) # noqa pylint: disable=protected-access # fake methods diff --git a/master/buildbot/test/fakedb/builds.py b/master/buildbot/test/fakedb/builds.py index b0bfd86751ca..2f41b32d6d24 100644 --- a/master/buildbot/test/fakedb/builds.py +++ b/master/buildbot/test/fakedb/builds.py @@ -207,8 +207,8 @@ def getBuildsForChange(self, changeid): for breq in breqs: for result in results: - if result['buildsetid'] == breq['buildsetid']: - result['buildrequestid'] = breq['buildrequestid'] + if result['buildsetid'] == breq.buildsetid: + result['buildrequestid'] = breq.buildrequestid for build in builds: for result in results: diff --git a/master/buildbot/test/fakedb/sourcestamps.py b/master/buildbot/test/fakedb/sourcestamps.py index ad40c3174bb1..bdac2d8be041 100644 --- a/master/buildbot/test/fakedb/sourcestamps.py +++ b/master/buildbot/test/fakedb/sourcestamps.py @@ -246,7 +246,7 @@ def get_sourcestamps_for_buildset(self, buildsetid): def getSourceStampsForBuild(self, buildid): build = yield self.db.builds.getBuild(buildid) breq = yield self.db.buildrequests.getBuildRequest(build['buildrequestid']) - bset = yield self.db.buildsets.getBuildset(breq['buildsetid']) + bset = yield self.db.buildsets.getBuildset(breq.buildsetid) results = [] for ssid in bset['sourcestamps']: diff --git a/master/buildbot/test/unit/db/test_buildrequests.py b/master/buildbot/test/unit/db/test_buildrequests.py index 1e7656e95a0b..74547ef11e87 100644 --- a/master/buildbot/test/unit/db/test_buildrequests.py +++ b/master/buildbot/test/unit/db/test_buildrequests.py @@ -84,21 +84,20 @@ def test_getBuildRequest(self): yield self.assertEqual( brdict, - { - "buildrequestid": 44, - "buildsetid": self.BSID, - "builderid": self.BLDRID1, - "buildername": "builder1", - "priority": 7, - "claimed": True, - "claimed_by_masterid": self.MASTER_ID, - "complete": True, - "results": 75, - "claimed_at": self.CLAIMED_AT, - "submitted_at": self.SUBMITTED_AT, - "complete_at": self.COMPLETE_AT, - "waited_for": False, - }, + buildrequests.BuildRequestModel( + buildrequestid=44, + buildsetid=self.BSID, + builderid=self.BLDRID1, + buildername="builder1", + priority=7, + claimed_by_masterid=self.MASTER_ID, + complete=True, + results=75, + claimed_at=self.CLAIMED_AT, + submitted_at=self.SUBMITTED_AT, + complete_at=self.COMPLETE_AT, + waited_for=False, + ), ) @defer.inlineCallbacks @@ -128,7 +127,7 @@ def do_test_getBuildRequests_claim_args(self, **kwargs): ]) brlist = yield self.db.buildrequests.getBuildRequests(**kwargs) - self.assertEqual(sorted([br['buildrequestid'] for br in brlist]), sorted(expected)) + self.assertEqual(sorted([br.buildrequestid for br in brlist]), sorted(expected)) def test_getBuildRequests_no_claimed_arg(self): return self.do_test_getBuildRequests_claim_args(expected=[50, 51, 52, 53]) @@ -155,7 +154,7 @@ def do_test_getBuildRequests_buildername_arg(self, **kwargs): ]) brlist = yield self.db.buildrequests.getBuildRequests(**kwargs) - self.assertEqual(sorted([br['buildrequestid'] for br in brlist]), sorted(expected)) + self.assertEqual(sorted([br.buildrequestid for br in brlist]), sorted(expected)) @defer.inlineCallbacks def do_test_getBuildRequests_complete_arg(self, **kwargs): @@ -188,7 +187,7 @@ def do_test_getBuildRequests_complete_arg(self, **kwargs): ]) brlist = yield self.db.buildrequests.getBuildRequests(**kwargs) - self.assertEqual(sorted([br['buildrequestid'] for br in brlist]), sorted(expected)) + self.assertEqual(sorted([br.buildrequestid for br in brlist]), sorted(expected)) def test_getBuildRequests_complete_none(self): return self.do_test_getBuildRequests_complete_arg(expected=[70, 80, 81, 82]) @@ -220,7 +219,7 @@ def test_getBuildRequests_bsid_arg(self): ]) brlist = yield self.db.buildrequests.getBuildRequests(bsid=self.BSID) - self.assertEqual(sorted([br['buildrequestid'] for br in brlist]), sorted([70, 72])) + self.assertEqual(sorted([br.buildrequestid for br in brlist]), sorted([70, 72])) @defer.inlineCallbacks def test_getBuildRequests_combo(self): @@ -299,7 +298,7 @@ def test_getBuildRequests_combo(self): builderid=self.BLDRID1, claimed=self.MASTER_ID, complete=True, bsid=self.BSID ) - self.assertEqual([br['buildrequestid'] for br in brlist], [44]) + self.assertEqual([br.buildrequestid for br in brlist], [44]) @defer.inlineCallbacks def do_test_getBuildRequests_branch_arg(self, **kwargs): @@ -323,7 +322,7 @@ def do_test_getBuildRequests_branch_arg(self, **kwargs): ]) brlist = yield self.db.buildrequests.getBuildRequests(**kwargs) - self.assertEqual(sorted([br['buildrequestid'] for br in brlist]), sorted(expected)) + self.assertEqual(sorted([br.buildrequestid for br in brlist]), sorted(expected)) def test_getBuildRequests_branch(self): return self.do_test_getBuildRequests_branch_arg(branch='branch_A', expected=[70, 90]) @@ -368,10 +367,7 @@ def do_test_claimBuildRequests( self.assertNotEqual(expected, None, "unexpected success from claimBuildRequests") self.assertEqual( - sorted([ - (r['buildrequestid'], r['claimed_at'], r['claimed_by_masterid']) - for r in results - ]), + sorted([(r.buildrequestid, r.claimed_at, r.claimed_by_masterid) for r in results]), sorted(expected), ) except Exception as e: @@ -461,9 +457,7 @@ def test_claimBuildRequests_other_master_claim_stress(self): # check that [1,1000) were not claimed, and 1000 is still claimed self.assertEqual( - [(r['buildrequestid'], r['claimed_by_masterid'], r['claimed_at']) for r in results][ - :10 - ], + [(r.buildrequestid, r.claimed_by_masterid, r.claimed_at) for r in results][:10], [(1000, self.OTHER_MASTER_ID, epoch2datetime(1300103810))], ) @@ -499,10 +493,7 @@ def do_test_completeBuildRequests( self.assertNotEqual(expected, None, "unexpected success from completeBuildRequests") self.assertEqual( - sorted( - (r['buildrequestid'], r['complete'], r['results'], r['complete_at']) - for r in results - ), + sorted((r.buildrequestid, r.complete, r.results, r.complete_at) for r in results), sorted(expected), ) @@ -664,7 +655,7 @@ def do_test_unclaimMethod(self, method, expected): # just select the unclaimed requests results = yield self.db.buildrequests.getBuildRequests(claimed=False) - self.assertEqual(sorted([r['buildrequestid'] for r in results]), sorted(expected)) + self.assertEqual(sorted([r.buildrequestid for r in results]), sorted(expected)) def test_unclaimBuildRequests(self): to_unclaim = [ diff --git a/master/buildbot/test/unit/process/test_buildrequest.py b/master/buildbot/test/unit/process/test_buildrequest.py index 02bd9bd4e63b..d08c39e33958 100644 --- a/master/buildbot/test/unit/process/test_buildrequest.py +++ b/master/buildbot/test/unit/process/test_buildrequest.py @@ -742,7 +742,6 @@ def test_canBeCollapsed_different_codebases_raises_error(self): Merge cannot be performed and raises error: Merging requests requires both requests to have the same codebases """ - brDicts = [] # list of buildrequests dictionary master = fakemaster.make_master(self, wantData=True, wantDb=True) master.db.insert_test_data([ fakedb.Builder(id=77, name='bldr'), @@ -787,14 +786,8 @@ def test_canBeCollapsed_different_codebases_raises_error(self): fakedb.BuildsetSourceStamp(buildsetid=540, sourcestampid=239), fakedb.BuildRequest(id=289, buildsetid=540, builderid=77), ]) - # use getBuildRequest to minimize the risk from changes to the format - # of the brdict - req = yield master.db.buildrequests.getBuildRequest(288) - brDicts.append(req) - req = yield master.db.buildrequests.getBuildRequest(289) - brDicts.append(req) - can_collapse = yield buildrequest.BuildRequest.canBeCollapsed( - master, brDicts[0], brDicts[1] - ) + old_br = yield master.data.get(('buildrequests', 288)) + new_br = yield master.data.get(('buildrequests', 289)) + can_collapse = yield buildrequest.BuildRequest.canBeCollapsed(master, new_br, old_br) self.assertEqual(can_collapse, False) diff --git a/master/buildbot/test/unit/schedulers/test_triggerable.py b/master/buildbot/test/unit/schedulers/test_triggerable.py index bcbac7d0f4d0..03c48131650c 100644 --- a/master/buildbot/test/unit/schedulers/test_triggerable.py +++ b/master/buildbot/test/unit/schedulers/test_triggerable.py @@ -17,6 +17,7 @@ from twisted.python import log from twisted.trial import unittest +from buildbot.db.buildrequests import BuildRequestModel from buildbot.process import properties from buildbot.schedulers import triggerable from buildbot.test import fakedb @@ -107,21 +108,20 @@ def assertTriggeredBuildset(self, idsDeferred, waited_for, properties=None, sour buildrequest = yield self.master.db.buildrequests.getBuildRequest(brid) self.assertEqual( buildrequest, - { - 'buildrequestid': brid, - 'buildername': 'b', - 'builderid': 77, - 'buildsetid': bsid, - 'claimed': False, - 'claimed_at': None, - 'complete': False, - 'complete_at': None, - 'claimed_by_masterid': None, - 'priority': 0, - 'results': -1, - 'submitted_at': datetime(1999, 12, 31, 23, 59, 59, tzinfo=UTC), - 'waited_for': waited_for, - }, + BuildRequestModel( + buildrequestid=brid, + buildername='b', + builderid=77, + buildsetid=bsid, + claimed_at=None, + complete=False, + complete_at=None, + claimed_by_masterid=None, + priority=0, + results=-1, + submitted_at=datetime(1999, 12, 31, 23, 59, 59, tzinfo=UTC), + waited_for=waited_for, + ), ) def sendCompletionMessage(self, bsid, results=3): diff --git a/master/docs/developer/database/buildrequests.rst b/master/docs/developer/database/buildrequests.rst index bd5cdacfe43a..0daccfe166ed 100644 --- a/master/docs/developer/database/buildrequests.rst +++ b/master/docs/developer/database/buildrequests.rst @@ -25,9 +25,8 @@ Buildrequests connector .. index:: brdict, brid - Build requests are indexed by an ID referred to as a *brid*. The contents - of a request are represented as build request dictionaries (brdicts) with - keys + Build requests are indexed by an ID referred to as a *brid*. The contents of a + request are represented by a :class:`BuildRequestModel` dataclass with the following fields: * ``buildrequestid`` * ``buildsetid`` @@ -46,7 +45,7 @@ Buildrequests connector .. py:method:: getBuildRequest(brid) :param brid: build request id to look up - :returns: brdict or ``None``, via Deferred + :returns: :class:`BuildRequestModel` or ``None``, via Deferred Get a single BuildRequest, in the format described above. This method returns ``None`` if there is no such buildrequest. Note that build @@ -65,7 +64,7 @@ Buildrequests connector :param branch: the branch associated with the sourcestamps originating the requests :param resultSpec: resultSpec containing filters sorting and paging request from data/REST API. If possible, the db layer can optimize the SQL query using this information. - :returns: list of brdicts, via Deferred + :returns: list of :class:`BuildRequestModel`, via Deferred Get a list of build requests matching the given characteristics. diff --git a/master/docs/spelling_wordlist.txt b/master/docs/spelling_wordlist.txt index 8c67f65c4d1d..ed1eb48cc14f 100644 --- a/master/docs/spelling_wordlist.txt +++ b/master/docs/spelling_wordlist.txt @@ -109,6 +109,7 @@ buildnumber buildrequest buildrequestid buildrequests +BuildRequestModel buildset Buildset buildsetid diff --git a/newsfragments/db-brdict.removal b/newsfragments/db-brdict.removal new file mode 100644 index 000000000000..76487ecde2d5 --- /dev/null +++ b/newsfragments/db-brdict.removal @@ -0,0 +1 @@ +:class:`buildbot.db.buildrequests.BrDict` is deprecated in favor of :class:`buildbot.db.buildrequests.BuildRequestModel` \ No newline at end of file diff --git a/newsfragments/db-introduce-BuildRequestModel.change b/newsfragments/db-introduce-BuildRequestModel.change new file mode 100644 index 000000000000..b579852ccee9 --- /dev/null +++ b/newsfragments/db-introduce-BuildRequestModel.change @@ -0,0 +1 @@ +:class:`BuildRequestsConnectorComponent` `getBuildRequest`, and `getBuildRequests` now return a :class`BuildRequestModel` instead of a dictionary of BuildRequest data \ No newline at end of file