Skip to content

Commit

Permalink
Merge pull request #7654 from tdesveaux/typing/db/changesources
Browse files Browse the repository at this point in the history
typing: Add ChangeSourceModel dataclass
  • Loading branch information
p12tic committed May 22, 2024
2 parents e2fd086 + 1ab1165 commit b71cbfd
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 62 deletions.
20 changes: 13 additions & 7 deletions master/buildbot/data/changesources.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
#
# Copyright Buildbot Team Members

from __future__ import annotations

from typing import TYPE_CHECKING

from twisted.internet import defer

Expand All @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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)
37 changes: 33 additions & 4 deletions master/buildbot/db/changesources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
22 changes: 10 additions & 12 deletions master/buildbot/test/fakedb/changesources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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
Expand Down
60 changes: 32 additions & 28 deletions master/buildbot/test/unit/db/test_changesources.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#
# Copyright Buildbot Team Members

from __future__ import annotations

from twisted.internet import defer
from twisted.trial import unittest

Expand All @@ -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):
Expand Down Expand Up @@ -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):
Expand All @@ -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):
Expand Down Expand Up @@ -111,15 +112,15 @@ 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):
"""A 'None' master for a disconnected changesource"""
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"""
Expand All @@ -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):
Expand All @@ -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"""
Expand All @@ -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,
),
Expand All @@ -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,
),
Expand All @@ -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),
]),
)

Expand All @@ -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), [])

Expand All @@ -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'),
]),
)

Expand All @@ -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!

Expand Down
8 changes: 0 additions & 8 deletions master/buildbot/test/util/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
6 changes: 3 additions & 3 deletions master/docs/developer/database/changesources.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -48,15 +48,15 @@ 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.

.. py:method:: getChangeSources(active=None, masterid=None)
: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.

Expand Down

0 comments on commit b71cbfd

Please sign in to comment.