From 060a403adc885e99a6b3cc413b4b34537d960917 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 14 May 2024 11:07:57 +0200 Subject: [PATCH] db: Add BuildDataModel dataclass --- master/buildbot/data/build_data.py | 36 +++++----- master/buildbot/db/build_data.py | 59 ++++++++++++---- master/buildbot/test/fakedb/build_data.py | 33 ++++----- .../test/unit/data/test_build_data.py | 15 ++-- .../buildbot/test/unit/db/test_build_data.py | 69 ++++++++++--------- master/buildbot/test/util/validation.py | 8 --- master/docs/developer/database/build_data.rst | 11 ++- master/docs/spelling_wordlist.txt | 1 + newsfragments/db-builddatadict.removal | 1 + .../db-introduce-BuildDataModel.change | 1 + 10 files changed, 133 insertions(+), 101 deletions(-) create mode 100644 newsfragments/db-builddatadict.removal create mode 100644 newsfragments/db-introduce-BuildDataModel.change diff --git a/master/buildbot/data/build_data.py b/master/buildbot/data/build_data.py index 7876d29e21b5..383517a27796 100644 --- a/master/buildbot/data/build_data.py +++ b/master/buildbot/data/build_data.py @@ -13,26 +13,30 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + +from typing import TYPE_CHECKING from twisted.internet import defer from buildbot.data import base from buildbot.data import types +if TYPE_CHECKING: + from buildbot.db.build_data import BuildDataModel -class Db2DataMixin: - def db2data(self, dbdict): - data = { - 'buildid': dbdict['buildid'], - 'name': dbdict['name'], - 'value': dbdict['value'], - 'length': dbdict['length'], - 'source': dbdict['source'], - } - return defer.succeed(data) + +def _db2data(model: BuildDataModel): + return { + 'buildid': model.buildid, + 'name': model.name, + 'value': model.value, + 'length': model.length, + 'source': model.source, + } -class BuildDatasNoValueEndpoint(Db2DataMixin, base.BuildNestingMixin, base.Endpoint): +class BuildDatasNoValueEndpoint(base.BuildNestingMixin, base.Endpoint): kind = base.EndpointKind.COLLECTION pathPatterns = """ /builders/n:builderid/builds/n:build_number/data @@ -48,11 +52,11 @@ def get(self, resultSpec, kwargs): results = [] for dbdict in build_datadicts: - results.append((yield self.db2data(dbdict))) + results.append(_db2data(dbdict)) return results -class BuildDataNoValueEndpoint(Db2DataMixin, base.BuildNestingMixin, base.Endpoint): +class BuildDataNoValueEndpoint(base.BuildNestingMixin, base.Endpoint): kind = base.EndpointKind.SINGLE pathPatterns = """ /builders/n:builderid/builds/n:build_number/data/i:name @@ -67,7 +71,7 @@ def get(self, resultSpec, kwargs): build_datadict = yield self.master.db.build_data.getBuildDataNoValue(buildid, name) - return (yield self.db2data(build_datadict)) if build_datadict else None + return _db2data(build_datadict) if build_datadict else None class BuildDataEndpoint(base.BuildNestingMixin, base.Endpoint): @@ -88,9 +92,9 @@ def get(self, resultSpec, kwargs): return None return { - 'raw': dbdict['value'], + 'raw': dbdict.value, 'mime-type': 'application/octet-stream', - 'filename': dbdict['name'], + 'filename': dbdict.name, } diff --git a/master/buildbot/db/build_data.py b/master/buildbot/db/build_data.py index 6c62545e4222..72a5d249224c 100644 --- a/master/buildbot/db/build_data.py +++ b/master/buildbot/db/build_data.py @@ -13,14 +13,48 @@ # # Copyright Buildbot Team Members + +from __future__ import annotations + +from dataclasses import dataclass + import sqlalchemy as sa from twisted.internet import defer +from twisted.python import deprecate +from twisted.python import versions from buildbot.db import NULL from buildbot.db import base +from buildbot.warnings import warn_deprecated + + +@dataclass +class BuildDataModel: + buildid: int + name: str + length: int + source: str + value: bytes | None + + # For backward compatibility + def __getitem__(self, key: str): + warn_deprecated( + '3.12.0', + ( + 'BuildDataConnectorComponent getBuildData, getBuildDataNoValue, and getAllBuildDataNoValues ' + 'no longer return BuildData as dictionnaries. ' + 'Usage of [] accessor is deprecated: please access the member directly' + ), + ) + + if hasattr(self, key): + return getattr(self, key) + + raise KeyError(key) -class BuildDataDict(dict): +@deprecate.deprecated(versions.Version("buildbot", 3, 12, 0), BuildDataModel) +class BuildDataDict(BuildDataModel): pass @@ -73,7 +107,7 @@ def thd(conn): @defer.inlineCallbacks def getBuildData(self, buildid, name): - def thd(conn): + def thd(conn) -> BuildDataModel | None: build_data_table = self.db.model.build_data q = build_data_table.select().where( @@ -83,14 +117,14 @@ def thd(conn): row = res.fetchone() if not row: return None - return self._row2dict(conn, row) + return self._model_from_row(row, value=row.value) res = yield self.db.pool.do(thd) return res @defer.inlineCallbacks def getBuildDataNoValue(self, buildid, name): - def thd(conn): + def thd(conn) -> BuildDataModel | None: build_data_table = self.db.model.build_data q = sa.select( @@ -104,14 +138,14 @@ def thd(conn): row = res.fetchone() if not row: return None - return self._row2dict_novalue(conn, row) + return self._model_from_row(row, value=None) res = yield self.db.pool.do(thd) return res @defer.inlineCallbacks def getAllBuildDataNoValues(self, buildid): - def thd(conn): + def thd(conn) -> list[BuildDataModel]: build_data_table = self.db.model.build_data q = sa.select( @@ -122,7 +156,7 @@ def thd(conn): ) q = q.where(build_data_table.c.buildid == buildid) - return [self._row2dict_novalue(conn, row) for row in conn.execute(q).fetchall()] + return [self._model_from_row(row, value=None) for row in conn.execute(q).fetchall()] res = yield self.db.pool.do(thd) return res @@ -166,16 +200,11 @@ def thd(conn): res = yield self.db.pool.do(thd) return res - def _row2dict(self, conn, row): - return BuildDataDict( + def _model_from_row(self, row, value: bytes | None): + return BuildDataModel( buildid=row.buildid, name=row.name, - value=row.value, length=row.length, source=row.source, - ) - - def _row2dict_novalue(self, conn, row): - return BuildDataDict( - buildid=row.buildid, name=row.name, value=None, length=row.length, source=row.source + value=value, ) diff --git a/master/buildbot/test/fakedb/build_data.py b/master/buildbot/test/fakedb/build_data.py index 13c4402b9a08..c463a5f17e20 100644 --- a/master/buildbot/test/fakedb/build_data.py +++ b/master/buildbot/test/fakedb/build_data.py @@ -13,8 +13,11 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + from twisted.internet import defer +from buildbot.db.build_data import BuildDataModel from buildbot.test.fakedb.base import FakeDBComponent from buildbot.test.fakedb.row import Row @@ -68,26 +71,26 @@ def setBuildData(self, buildid, name, value, source): } # returns a Deferred - def getBuildData(self, buildid, name): + def getBuildData(self, buildid, name) -> defer.Deferred[BuildDataModel | None]: row = self._get_build_data_row(buildid, name) if row is not None: - return defer.succeed(self._row2dict(row)) + return defer.succeed(self._model_from_row(row, value=row.get('value'))) return defer.succeed(None) # returns a Deferred - def getBuildDataNoValue(self, buildid, name): + def getBuildDataNoValue(self, buildid, name) -> defer.Deferred[BuildDataModel | None]: row = self._get_build_data_row(buildid, name) if row is not None: - return defer.succeed(self._row2dict_novalue(row)) + return defer.succeed(self._model_from_row(row, value=None)) return defer.succeed(None) # returns a Deferred - def getAllBuildDataNoValues(self, buildid): + def getAllBuildDataNoValues(self, buildid) -> defer.Deferred[list[BuildDataModel]]: ret = [] for row in self.build_data.values(): if row['buildid'] != buildid: continue - ret.append(self._row2dict_novalue(row)) + ret.append(self._model_from_row(row, value=None)) return defer.succeed(ret) @@ -115,13 +118,11 @@ def deleteOldBuildData(self, older_than_timestamp): return defer.succeed(count_before - count_after) - def _row2dict(self, row): - ret = row.copy() - del ret['id'] - return ret - - def _row2dict_novalue(self, row): - ret = row.copy() - del ret['id'] - ret['value'] = None - return ret + def _model_from_row(self, row, value: bytes | None): + return BuildDataModel( + buildid=row['buildid'], + name=row['name'], + length=row['length'], + source=row['source'], + value=value, + ) diff --git a/master/buildbot/test/unit/data/test_build_data.py b/master/buildbot/test/unit/data/test_build_data.py index 7d627214c34b..02bf0c8a28e9 100644 --- a/master/buildbot/test/unit/data/test_build_data.py +++ b/master/buildbot/test/unit/data/test_build_data.py @@ -18,6 +18,7 @@ from twisted.trial import unittest from buildbot.data import build_data +from buildbot.db.build_data import BuildDataModel from buildbot.test import fakedb from buildbot.test.fake import fakemaster from buildbot.test.reactor import TestReactorMixin @@ -343,11 +344,11 @@ def test_set_build_data(self): result = yield self.master.db.build_data.getBuildData(2, 'name1') self.assertEqual( result, - { - 'buildid': 2, - 'name': 'name1', - 'value': b'value1', - 'length': 6, - 'source': 'source1', - }, + BuildDataModel( + buildid=2, + name='name1', + value=b'value1', + length=6, + source='source1', + ), ) diff --git a/master/buildbot/test/unit/db/test_build_data.py b/master/buildbot/test/unit/db/test_build_data.py index 17066d0d7fad..0dbca698fde6 100644 --- a/master/buildbot/test/unit/db/test_build_data.py +++ b/master/buildbot/test/unit/db/test_build_data.py @@ -21,7 +21,6 @@ from buildbot.test import fakedb from buildbot.test.util import connector_component from buildbot.test.util import interfaces -from buildbot.test.util import validation class Tests(interfaces.InterfaceTests): @@ -66,16 +65,16 @@ def test_add_data_get_data(self): buildid=30, name='mykey', value=b'myvalue', source='mysource' ) data_dict = yield self.db.build_data.getBuildData(buildid=30, name='mykey') - validation.verifyDbDict(self, 'build_datadict', data_dict) + self.assertIsInstance(data_dict, build_data.BuildDataModel) self.assertEqual( data_dict, - { - 'buildid': 30, - 'name': 'mykey', - 'value': b'myvalue', - 'length': 7, - 'source': 'mysource', - }, + build_data.BuildDataModel( + buildid=30, + name='mykey', + value=b'myvalue', + length=7, + source='mysource', + ), ) @defer.inlineCallbacks @@ -95,16 +94,16 @@ def test_add_data_replace_value(self): ) data_dict = yield self.db.build_data.getBuildData(buildid=30, name='mykey') - validation.verifyDbDict(self, 'build_datadict', data_dict) + self.assertIsInstance(data_dict, build_data.BuildDataModel) self.assertEqual( data_dict, - { - 'buildid': 30, - 'name': 'mykey', - 'value': b'myvalue2', - 'length': 8, - 'source': 'mysource2', - }, + build_data.BuildDataModel( + buildid=30, + name='mykey', + value=b'myvalue2', + length=8, + source='mysource2', + ), ) @defer.inlineCallbacks @@ -130,16 +129,16 @@ def hook(conn): ) data_dict = yield self.db.build_data.getBuildData(buildid=30, name='mykey') - validation.verifyDbDict(self, 'build_datadict', data_dict) + self.assertIsInstance(data_dict, build_data.BuildDataModel) self.assertEqual( data_dict, - { - 'buildid': 30, - 'name': 'mykey', - 'value': b'myvalue', - 'length': 7, - 'source': 'mysource', - }, + build_data.BuildDataModel( + buildid=30, + name='mykey', + value=b'myvalue', + length=7, + source='mysource', + ), ) @defer.inlineCallbacks @@ -149,10 +148,12 @@ def test_add_data_get_data_no_value(self): buildid=30, name='mykey', value=b'myvalue', source='mysource' ) data_dict = yield self.db.build_data.getBuildDataNoValue(buildid=30, name='mykey') - validation.verifyDbDict(self, 'build_datadict', data_dict) + self.assertIsInstance(data_dict, build_data.BuildDataModel) self.assertEqual( data_dict, - {'buildid': 30, 'name': 'mykey', 'value': None, 'length': 7, 'source': 'mysource'}, + build_data.BuildDataModel( + buildid=30, name='mykey', value=None, length=7, source='mysource' + ), ) @defer.inlineCallbacks @@ -179,20 +180,22 @@ def test_get_all_build_data_no_values(self): ) data_dicts = yield self.db.build_data.getAllBuildDataNoValues(30) - self.assertEqual([d['name'] for d in data_dicts], ['name1', 'name2']) + self.assertEqual([d.name for d in data_dicts], ['name1', 'name2']) for d in data_dicts: - validation.verifyDbDict(self, 'build_datadict', d) + self.assertIsInstance(d, build_data.BuildDataModel) # note that value is not in dict, but length is self.assertEqual( data_dicts[0], - {'buildid': 30, 'name': 'name1', 'value': None, 'length': 6, 'source': 'source1'}, + build_data.BuildDataModel( + buildid=30, name='name1', value=None, length=6, source='source1' + ), ) data_dicts = yield self.db.build_data.getAllBuildDataNoValues(31) - self.assertEqual([d['name'] for d in data_dicts], ['name3']) + self.assertEqual([d.name for d in data_dicts], ['name3']) data_dicts = yield self.db.build_data.getAllBuildDataNoValues(32) - self.assertEqual([d['name'] for d in data_dicts], []) + self.assertEqual([d.name for d in data_dicts], []) @parameterized.expand([ (1000000, 0, ['name1', 'name2', 'name3', 'name4', 'name5', 'name6']), @@ -260,7 +263,7 @@ def test_remove_old_build_data( remaining_names = [] for buildid in [50, 51, 52, 53]: data_dicts = yield self.db.build_data.getAllBuildDataNoValues(buildid) - remaining_names += [d['name'] for d in data_dicts] + remaining_names.extend(d.name for d in data_dicts) self.assertEqual(sorted(remaining_names), sorted(exp_remaining_names)) diff --git a/master/buildbot/test/util/validation.py b/master/buildbot/test/util/validation.py index aa860282c376..f12e9f1a801d 100644 --- a/master/buildbot/test/util/validation.py +++ b/master/buildbot/test/util/validation.py @@ -574,14 +574,6 @@ def validate(self, name, arg_object): message['build_data'] = Selector() message['build_data'].add(None, MessageValidator(events=[], messageValidator=_build_data_msgdict)) -dbdict['build_datadict'] = DictValidator( - buildid=IntValidator(), - name=StringValidator(), - value=NoneOk(BinaryValidator()), - length=IntValidator(), - source=StringValidator(), -) - # steps _step = { diff --git a/master/docs/developer/database/build_data.rst b/master/docs/developer/database/build_data.rst index 4a1ebf23cf6f..2451645e035b 100644 --- a/master/docs/developer/database/build_data.rst +++ b/master/docs/developer/database/build_data.rst @@ -14,12 +14,12 @@ Build data connector An instance of this class is available at ``master.db.build_data``. - Builds are indexed by *build_dataid* and their contents represented as *build_datadicts* (build data dictionaries), with the following keys: + Builds are indexed by *build_dataid* and their contents represented as :class:`BuildDataModel` dataclass, with the following fields: * ``id`` (the build data ID, globally unique) * ``buildid`` (the ID of the build that the data is attached to) * ``name`` (the name of the data) - * ``value`` (the value of the data. It must be an instance of ``bytes``) + * ``value`` (the value of the data. It must be an instance of ``bytes``. Can be ``None`` when queried with ``getBuildDataNoValue``) * ``source`` (an string identifying the source of this value) .. py:method:: setBuildData(buildid, name, value, source) @@ -36,7 +36,7 @@ Build data connector :param integer buildid: build id retrieve data for :param unicode name: the name of the data - :returns: Build data dictionary as above or ``None``, via Deferred + :returns: :class:`BuildDataModel` or ``None``, via Deferred Get a single build data, in the format described above, specified by build and by name. Returns ``None`` if build has no data with such name. @@ -45,7 +45,7 @@ Build data connector :param integer buildid: build id retrieve data for :param unicode name: the name of the data - :returns: Build data dictionary as above or ``None``, via Deferred + :returns: :class:`BuildDataModel` or ``None``, via Deferred Get a single build data, in the format described above, specified by build and by name. The ``value`` field is omitted. @@ -55,7 +55,7 @@ Build data connector :param integer buildid: build id retrieve data for :param unicode name: the name of the data - :returns: a list of build data dictionaries + :returns: a list of :class:`BuildDataModel` Returns all data for a specific build. The values are not loaded. @@ -68,4 +68,3 @@ Build data connector Delete old build data (helper for the ``build_data_horizon`` policy). Old logs have their build data deleted from the database as they are only useful while build is running and shortly afterwards. - diff --git a/master/docs/spelling_wordlist.txt b/master/docs/spelling_wordlist.txt index 8c67f65c4d1d..b894f37d557d 100644 --- a/master/docs/spelling_wordlist.txt +++ b/master/docs/spelling_wordlist.txt @@ -95,6 +95,7 @@ buildbot Buildbot buildCacheSize builddir +BuildDataModel buildDict builderid builderids diff --git a/newsfragments/db-builddatadict.removal b/newsfragments/db-builddatadict.removal new file mode 100644 index 000000000000..d2e39929a629 --- /dev/null +++ b/newsfragments/db-builddatadict.removal @@ -0,0 +1 @@ +:class:`buildbot.db.build_data.BuildDataDict` is deprecated in favor of :class:`buildbot.db.build_data.BuildDataModel` \ No newline at end of file diff --git a/newsfragments/db-introduce-BuildDataModel.change b/newsfragments/db-introduce-BuildDataModel.change new file mode 100644 index 000000000000..7a9eb10cd5f6 --- /dev/null +++ b/newsfragments/db-introduce-BuildDataModel.change @@ -0,0 +1 @@ +:class:`BuildDataConnectorComponent` `getBuildData`, `getBuildDataNoValue`, and `getAllBuildDataNoValues` now return a :class`BuildDataModel` instead of a dictionary of BuildData data \ No newline at end of file