diff --git a/master/buildbot/data/changesources.py b/master/buildbot/data/changesources.py index 6a5007b2719..f6d183e0cde 100644 --- a/master/buildbot/data/changesources.py +++ b/master/buildbot/data/changesources.py @@ -13,6 +13,9 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + +from typing import TYPE_CHECKING from twisted.internet import defer @@ -21,16 +24,19 @@ from buildbot.data import types from buildbot.db.changesources import ChangeSourceAlreadyClaimedError +if TYPE_CHECKING: + from buildbot.db.changesources import ChangeSourceModel + class Db2DataMixin: @defer.inlineCallbacks - def db2data(self, dbdict): + def db2data(self, dbdict: ChangeSourceModel): master = None - if dbdict['masterid'] is not None: - master = yield self.master.data.get(('masters', dbdict['masterid'])) + if dbdict.masterid is not None: + master = yield self.master.data.get(('masters', dbdict.masterid)) data = { - 'changesourceid': dbdict['id'], - 'name': dbdict['name'], + 'changesourceid': dbdict.id, + 'name': dbdict.name, 'master': master, } return data @@ -46,7 +52,7 @@ class ChangeSourceEndpoint(Db2DataMixin, base.Endpoint): def get(self, resultSpec, kwargs): dbdict = yield self.master.db.changesources.getChangeSource(kwargs['changesourceid']) if 'masterid' in kwargs: - if dbdict['masterid'] != kwargs['masterid']: + if dbdict.masterid != kwargs['masterid']: return None return (yield self.db2data(dbdict)) if dbdict else None @@ -111,4 +117,4 @@ def trapAlreadyClaimedError(why): def _masterDeactivated(self, masterid): changesources = yield self.master.db.changesources.getChangeSources(masterid=masterid) for cs in changesources: - yield self.master.db.changesources.setChangeSourceMaster(cs['id'], None) + yield self.master.db.changesources.setChangeSourceMaster(cs.id, None) diff --git a/master/buildbot/db/changesources.py b/master/buildbot/db/changesources.py index 017941755c8..33516f9bac8 100644 --- a/master/buildbot/db/changesources.py +++ b/master/buildbot/db/changesources.py @@ -13,18 +13,47 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + +from dataclasses import dataclass import sqlalchemy as sa from twisted.internet import defer from buildbot.db import NULL from buildbot.db import base +from buildbot.warnings import warn_deprecated class ChangeSourceAlreadyClaimedError(Exception): pass +@dataclass +class ChangeSourceModel: + id: int + name: str + + masterid: int | None = None + + # For backward compatibility + def __getitem__(self, key: str): + warn_deprecated( + '4.1.0', + ( + 'ChangeSourcesConnectorComponent ' + 'getChangeSource, and getChangeSources ' + 'no longer return ChangeSource as dictionnaries. ' + 'Usage of [] accessor is deprecated: please access the member directly' + ), + ) + + if hasattr(self, key): + return getattr(self, key) + + raise KeyError(key) + + class ChangeSourcesConnectorComponent(base.DBConnectorComponent): def findChangeSourceId(self, name): tbl = self.db.model.changesources @@ -96,9 +125,9 @@ def thd(conn): if wc is not None: q = q.where(wc) - return [ - {"id": row.id, "name": row.name, "masterid": row.masterid} - for row in conn.execute(q).fetchall() - ] + return [self._model_from_row(row) for row in conn.execute(q).fetchall()] return self.db.pool.do(thd) + + def _model_from_row(self, row): + return ChangeSourceModel(id=row.id, name=row.name, masterid=row.masterid) diff --git a/master/buildbot/test/fakedb/changesources.py b/master/buildbot/test/fakedb/changesources.py index 6e7206964ca..d217f682429 100644 --- a/master/buildbot/test/fakedb/changesources.py +++ b/master/buildbot/test/fakedb/changesources.py @@ -66,15 +66,13 @@ def findChangeSourceId(self, name): def getChangeSource(self, changesourceid): if changesourceid in self.changesources: - rv = { - "id": changesourceid, - "name": self.changesources[changesourceid], - "masterid": None, - } - # only set masterid if the relevant changesource master exists and - # is active - rv['masterid'] = self.changesource_masters.get(changesourceid) - return defer.succeed(rv) + return defer.succeed( + changesources.ChangeSourceModel( + id=changesourceid, + name=self.changesources[changesourceid], + masterid=self.changesource_masters.get(changesourceid), + ) + ) return None def getChangeSources(self, active=None, masterid=None): @@ -86,12 +84,12 @@ def filter(results): results = [r[1] for r in results] # filter for masterid if masterid is not None: - results = [r for r in results if r['masterid'] == masterid] + results = [r for r in results if r.masterid == masterid] # filter for active or inactive if necessary if active: - results = [r for r in results if r['masterid'] is not None] + results = [r for r in results if r.masterid is not None] elif active is not None: - results = [r for r in results if r['masterid'] is None] + results = [r for r in results if r.masterid is None] return results return d diff --git a/master/buildbot/test/unit/db/test_changesources.py b/master/buildbot/test/unit/db/test_changesources.py index 184ea1a818d..38c5f1ea548 100644 --- a/master/buildbot/test/unit/db/test_changesources.py +++ b/master/buildbot/test/unit/db/test_changesources.py @@ -13,6 +13,8 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + from twisted.internet import defer from twisted.trial import unittest @@ -21,11 +23,10 @@ from buildbot.test.util import connector_component from buildbot.test.util import db from buildbot.test.util import interfaces -from buildbot.test.util import validation -def changeSourceKey(changeSource): - return changeSource['id'] +def changeSourceKey(changeSource: changesources.ChangeSourceModel): + return changeSource.id class Tests(interfaces.InterfaceTests): @@ -54,7 +55,7 @@ def test_findChangeSourceId_new(self): """findChangeSourceId for a new changesource creates it""" id = yield self.db.changesources.findChangeSourceId('csname') cs = yield self.db.changesources.getChangeSource(id) - self.assertEqual(cs['name'], 'csname') + self.assertEqual(cs.name, 'csname') @defer.inlineCallbacks def test_findChangeSourceId_existing(self): @@ -76,7 +77,7 @@ def test_setChangeSourceMaster_fresh(self): yield self.insert_test_data([self.cs42, self.master13]) yield self.db.changesources.setChangeSourceMaster(42, 13) cs = yield self.db.changesources.getChangeSource(42) - self.assertEqual(cs['masterid'], 13) + self.assertEqual(cs.masterid, 13) @defer.inlineCallbacks def test_setChangeSourceMaster_inactive_but_linked(self): @@ -111,7 +112,7 @@ def test_setChangeSourceMaster_None(self): ]) yield self.db.changesources.setChangeSourceMaster(87, None) cs = yield self.db.changesources.getChangeSource(87) - self.assertEqual(cs['masterid'], None) + self.assertEqual(cs.masterid, None) @defer.inlineCallbacks def test_setChangeSourceMaster_None_unowned(self): @@ -119,7 +120,7 @@ def test_setChangeSourceMaster_None_unowned(self): yield self.insert_test_data([self.cs87]) yield self.db.changesources.setChangeSourceMaster(87, None) cs = yield self.db.changesources.getChangeSource(87) - self.assertEqual(cs['masterid'], None) + self.assertEqual(cs.masterid, None) def test_signature_getChangeSource(self): """getChangeSource has the right signature""" @@ -133,8 +134,8 @@ def test_getChangeSource(self): """getChangeSource for a changesource that exists""" yield self.insert_test_data([self.cs87]) cs = yield self.db.changesources.getChangeSource(87) - validation.verifyDbDict(self, 'changesourcedict', cs) - self.assertEqual(cs, {"id": 87, "name": 'lame_source', "masterid": None}) + self.assertIsInstance(cs, changesources.ChangeSourceModel) + self.assertEqual(cs, changesources.ChangeSourceModel(id=87, name='lame_source')) @defer.inlineCallbacks def test_getChangeSource_missing(self): @@ -147,18 +148,21 @@ def test_getChangeSource_active(self): """getChangeSource for a changesource that exists and is active""" yield self.insert_test_data([self.cs42, self.master13, self.cs42master13]) cs = yield self.db.changesources.getChangeSource(42) - validation.verifyDbDict(self, 'changesourcedict', cs) - self.assertEqual(cs, {"id": 42, "name": 'cool_source', "masterid": 13}) + self.assertIsInstance(cs, changesources.ChangeSourceModel) + self.assertEqual( + cs, changesources.ChangeSourceModel(id=42, name='cool_source', masterid=13) + ) @defer.inlineCallbacks def test_getChangeSource_inactive_but_linked(self): """getChangeSource for a changesource that is assigned but is inactive""" yield self.insert_test_data([self.cs87, self.master14, self.cs87master14]) cs = yield self.db.changesources.getChangeSource(87) - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( - cs, {"id": 87, "name": 'lame_source', "masterid": 14} - ) # row exists, but marked inactive + cs, changesources.ChangeSourceModel(id=87, name='lame_source', masterid=14) + ) + # row exists, but marked inactive def test_signature_getChangeSources(self): """getChangeSources has right signature""" @@ -179,14 +183,14 @@ def test_getChangeSources(self): cslist = yield self.db.changesources.getChangeSources() for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( sorted(cslist, key=changeSourceKey), sorted( [ - {"id": 42, "name": 'cool_source', "masterid": 13}, - {"id": 87, "name": 'lame_source', "masterid": None}, + changesources.ChangeSourceModel(id=42, name='cool_source', masterid=13), + changesources.ChangeSourceModel(id=87, name='lame_source', masterid=None), ], key=changeSourceKey, ), @@ -204,13 +208,13 @@ def test_getChangeSources_masterid(self): cslist = yield self.db.changesources.getChangeSources(masterid=13) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( sorted(cslist, key=changeSourceKey), sorted( [ - {"id": 42, "name": 'cool_source', "masterid": 13}, + changesources.ChangeSourceModel(id=42, name='cool_source', masterid=13), ], key=changeSourceKey, ), @@ -223,12 +227,12 @@ def test_getChangeSources_active(self): cslist = yield self.db.changesources.getChangeSources(active=True) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( sorted(cslist), sorted([ - {"id": 42, "name": 'cool_source', "masterid": 13}, + changesources.ChangeSourceModel(id=42, name='cool_source', masterid=13), ]), ) @@ -239,19 +243,19 @@ def test_getChangeSources_active_masterid(self): cslist = yield self.db.changesources.getChangeSources(active=True, masterid=13) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( sorted(cslist), sorted([ - {"id": 42, "name": 'cool_source', "masterid": 13}, + changesources.ChangeSourceModel(id=42, name='cool_source', masterid=13), ]), ) cslist = yield self.db.changesources.getChangeSources(active=True, masterid=14) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual(sorted(cslist), []) @@ -262,12 +266,12 @@ def test_getChangeSources_inactive(self): cslist = yield self.db.changesources.getChangeSources(active=False) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual( sorted(cslist), sorted([ - {"id": 87, "name": 'lame_source', "masterid": None}, + changesources.ChangeSourceModel(id=87, name='lame_source'), ]), ) @@ -278,14 +282,14 @@ def test_getChangeSources_inactive_masterid(self): cslist = yield self.db.changesources.getChangeSources(active=False, masterid=13) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual(sorted(cslist), []) cslist = yield self.db.changesources.getChangeSources(active=False, masterid=14) for cs in cslist: - validation.verifyDbDict(self, 'changesourcedict', cs) + self.assertIsInstance(cs, changesources.ChangeSourceModel) self.assertEqual(sorted(cslist), []) # always returns [] by spec! diff --git a/master/buildbot/test/util/validation.py b/master/buildbot/test/util/validation.py index 42115a408d3..72fc512127b 100644 --- a/master/buildbot/test/util/validation.py +++ b/master/buildbot/test/util/validation.py @@ -468,14 +468,6 @@ def validate(self, name, arg_object): parent_changeids=ListValidator(IntValidator()), ) -# changesources - -dbdict['changesourcedict'] = DictValidator( - id=IntValidator(), - name=StringValidator(), - masterid=NoneOk(IntValidator()), -) - # builds _build = { diff --git a/master/docs/developer/database/changesources.rst b/master/docs/developer/database/changesources.rst index 9a3bd7f95cf..90bc7e64238 100644 --- a/master/docs/developer/database/changesources.rst +++ b/master/docs/developer/database/changesources.rst @@ -17,7 +17,7 @@ Change sources connector Changesources are identified by their changesourceid, which can be obtained from :py:meth:`findChangeSourceId`. - Changesources are represented by dictionaries with the following keys: + Changesources are represented as :class:`ChangeSourceModel` dataclass with the following fields: * ``id`` - changesource's ID * ``name`` - changesource's name @@ -48,7 +48,7 @@ Change sources connector .. py:method:: getChangeSource(changesourceid) :param changesourceid: changesource ID - :returns: changesource dictionary or None, via Deferred + :returns: :class:`ChangeSourceModel` or `None`, via Deferred Get the changesource dictionary for the given changesource. @@ -56,7 +56,7 @@ Change sources connector :param boolean active: if specified, filter for active or inactive changesources :param integer masterid: if specified, only return changesources attached associated with this master - :returns: list of changesource dictionaries in unspecified order + :returns: list of :class:`ChangeSourceModel` in unspecified order Get a list of changesources. diff --git a/master/docs/spelling_wordlist.txt b/master/docs/spelling_wordlist.txt index 9ca3a66cc15..1ea2b61a822 100644 --- a/master/docs/spelling_wordlist.txt +++ b/master/docs/spelling_wordlist.txt @@ -164,6 +164,7 @@ changeroot changeset changesource changesourceid +ChangeSourceModel changesources Changesources chdict diff --git a/newsfragments/db-introduce-ChangeSourceModel.change b/newsfragments/db-introduce-ChangeSourceModel.change new file mode 100644 index 00000000000..70ab04f8e9a --- /dev/null +++ b/newsfragments/db-introduce-ChangeSourceModel.change @@ -0,0 +1 @@ +:class:`ChangeSourcesConnectorComponent` `getChangeSource`, and `getChangeSources` now return a :class`ChangeSourceModel` instead of a dictionary of ChangeSource data \ No newline at end of file