From 0ed3d4de83cf241649d2448d0b29aced371e6355 Mon Sep 17 00:00:00 2001 From: jgilaber Date: Fri, 21 Mar 2025 11:09:42 +0100 Subject: [PATCH] Set number of decimal digits in efficacy indicator Configure the numeric type of the EfficacyIndicator value to use Float. Add a new column named data and deprecate the existing value columen. With the current model, value will use the default scale of the Decimal type of mysql, which in some enviornments is 0. This change also adds a test with mysql as backend to reproduce the issue, since the existing tests using sqlite do not reproduce the problem, as well as some simple migration tests. Closes-Bug: #2103458 Change-Id: Ib281fa32e902d2181449091f493d6506b5199094 --- ...737_change_efficiacy_indicator_decimals.py | 20 ++ watcher/db/sqlalchemy/api.py | 43 +++- watcher/db/sqlalchemy/models.py | 3 + watcher/tests/db/test_efficacy_indicator.py | 10 +- watcher/tests/db/test_migrations.py | 205 ++++++++++++++++++ 5 files changed, 269 insertions(+), 12 deletions(-) create mode 100644 watcher/db/sqlalchemy/alembic/versions/15f7375ca737_change_efficiacy_indicator_decimals.py create mode 100644 watcher/tests/db/test_migrations.py diff --git a/watcher/db/sqlalchemy/alembic/versions/15f7375ca737_change_efficiacy_indicator_decimals.py b/watcher/db/sqlalchemy/alembic/versions/15f7375ca737_change_efficiacy_indicator_decimals.py new file mode 100644 index 000000000..00166b9bd --- /dev/null +++ b/watcher/db/sqlalchemy/alembic/versions/15f7375ca737_change_efficiacy_indicator_decimals.py @@ -0,0 +1,20 @@ +"""change_efficiacy_indicator_decimals + +Revision ID: 15f7375ca737 +Revises: 609bec748f2a +Create Date: 2025-03-24 10:15:19.269061 + +""""" + +# revision identifiers, used by Alembic. +revision = '15f7375ca737' +down_revision = '609bec748f2a' + +from alembic import op +import sqlalchemy as sa + +def upgrade(): + op.add_column('efficacy_indicators', + sa.Column('data', sa.Float()) + ) + diff --git a/watcher/db/sqlalchemy/api.py b/watcher/db/sqlalchemy/api.py index 33ed128bf..a9bebe326 100644 --- a/watcher/db/sqlalchemy/api.py +++ b/watcher/db/sqlalchemy/api.py @@ -905,15 +905,34 @@ class Connection(api.BaseConnection): # ### EFFICACY INDICATORS ### # def get_efficacy_indicator_list(self, *args, **kwargs): - return self._get_model_list(models.EfficacyIndicator, - self._add_efficacy_indicators_filters, - *args, **kwargs) + eff_ind_models = self._get_model_list( + models.EfficacyIndicator, + self._add_efficacy_indicators_filters, + *args, **kwargs + ) + for indicator in eff_ind_models: + if indicator.data is not None: + # jgilaber: use the data value since it stores the value + # properly, see https://bugs.launchpad.net/watcher/+bug/2103458 + # for more details + indicator.value = indicator.data + else: + # if data is None, it means that we're reading data from a + # database created before the 15f7375ca737 revision, use the + # value column + indicator.data = indicator.value + + return eff_ind_models def create_efficacy_indicator(self, values): # ensure defaults are present for new efficacy indicators if not values.get('uuid'): values['uuid'] = utils.generate_uuid() + # jgilaber: use the data column since it stores the value + # properly, see https://bugs.launchpad.net/watcher/+bug/2103458 + # for more details + values['data'] = values.get('value') try: efficacy_indicator = self._create(models.EfficacyIndicator, values) except db_exc.DBDuplicateEntry: @@ -922,8 +941,22 @@ class Connection(api.BaseConnection): def _get_efficacy_indicator(self, context, fieldname, value, eager): try: - return self._get(context, model=models.EfficacyIndicator, - fieldname=fieldname, value=value, eager=eager) + efficacy_indicator = self._get(context, + model=models.EfficacyIndicator, + fieldname=fieldname, + value=value, eager=eager) + + if efficacy_indicator.data is not None: + # jgilaber: use the data value since it stores the value + # properly, see https://bugs.launchpad.net/watcher/+bug/2103458 + # for more details + efficacy_indicator.value = efficacy_indicator.data + else: + # if data is None, it means that we're reading data from a + # database created before the 15f7375ca737 revision, use the + # value column + efficacy_indicator.data = efficacy_indicator.value + return efficacy_indicator except exception.ResourceNotFound: raise exception.EfficacyIndicatorNotFound(efficacy_indicator=value) diff --git a/watcher/db/sqlalchemy/models.py b/watcher/db/sqlalchemy/models.py index 6ae06378f..41778c15a 100644 --- a/watcher/db/sqlalchemy/models.py +++ b/watcher/db/sqlalchemy/models.py @@ -238,7 +238,10 @@ class EfficacyIndicator(Base): name = Column(String(63)) description = Column(String(255), nullable=True) unit = Column(String(63), nullable=True) + # this column is deprecated due to bug + # https://bugs.launchpad.net/watcher/+bug/2103458 value = Column(Numeric()) + data = Column(Float()) action_plan_id = Column(Integer, ForeignKey('action_plans.id'), nullable=False) diff --git a/watcher/tests/db/test_efficacy_indicator.py b/watcher/tests/db/test_efficacy_indicator.py index a0b55b53a..aa39fd437 100644 --- a/watcher/tests/db/test_efficacy_indicator.py +++ b/watcher/tests/db/test_efficacy_indicator.py @@ -452,12 +452,8 @@ class MySQLDbEfficacyIndicatorTestCase(base.MySQLDbTestCase): action_plan_id=self.action_plan.id, id=3, uuid=None, name="efficacy_indicator3", description="Test Indicator 3") - def test_efficacy_indicator_value_decimals(self): + def test_efficacy_indicator_value(self): db_efficacy_indicator = self.dbapi.get_efficacy_indicator_by_id( self.context, 1) - self.assertAlmostEqual(float(db_efficacy_indicator.value), - 0.00, places=2) - # FIXME: once the database bug is fixed check that the value is stored - # correctly - # self.assertAlmostEqual(float(db_efficacy_indicator.value), - # 0.01, places=2) + self.assertAlmostEqual(db_efficacy_indicator.value, 0.012, places=3) + self.assertAlmostEqual(db_efficacy_indicator.data, 0.012, places=3) diff --git a/watcher/tests/db/test_migrations.py b/watcher/tests/db/test_migrations.py new file mode 100644 index 000000000..efee6d5e1 --- /dev/null +++ b/watcher/tests/db/test_migrations.py @@ -0,0 +1,205 @@ +# Copyright 2025 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +""""Tests for database migration for Watcher. + +These are "opportunistic" tests which allow testing against all three databases +(sqlite in memory, mysql, pg) in a properly configured unit test environment. + +For the opportunistic testing you need to set up DBs named 'openstack_citest' +with user 'openstack_citest' and password 'openstack_citest' on localhost. The +test will then use that DB and username/password combo to run the tests. Refer +to the 'tools/test-setup.sh' for an example of how to configure this. +""" + +from alembic import command as alembic_api +from alembic.script import ScriptDirectory +from oslo_config import cfg +from oslo_db.sqlalchemy import enginefacade +from oslo_db.sqlalchemy import test_fixtures +from oslo_log import log as logging +import sqlalchemy + +from watcher.common import utils as w_utils +from watcher.db import api as dbapi +from watcher.db.sqlalchemy import migration +from watcher.tests import base +from watcher.tests.db import utils + +LOG = logging.getLogger(__name__) + + +class MySQLDbMigrationsTestCase(test_fixtures.OpportunisticDBTestMixin, + base.TestCase): + + FIXTURE = test_fixtures.MySQLOpportunisticFixture + + def setUp(self): + conn_str = "mysql+pymysql://root:insecure_slave@127.0.0.1" + # to use mysql db + cfg.CONF.set_override("connection", conn_str, + group="database") + super().setUp() + self.engine = enginefacade.writer.get_engine() + self.dbapi = dbapi.get_instance() + self.alembic_config = migration._alembic_config() + self.revisions_tested = set(["15f7375ca737"]) + + def _apply_migration(self, connection, revision): + if revision not in self.revisions_tested: + # if we don't have tests for this version, just upgrade to it + alembic_api.upgrade(self.alembic_config, revision) + return + + pre_upgrade = getattr(self, f"_pre_upgrade_{revision}", None) + if pre_upgrade: + pre_upgrade(connection) + + alembic_api.upgrade(self.alembic_config, revision) + + post_upgrade = getattr(self, f"_check_{revision}", None) + if post_upgrade: + post_upgrade(connection) + + def _pre_upgrade_15f7375ca737(self, connection): + inspector = sqlalchemy.inspect(connection) + columns = inspector.get_columns("efficacy_indicators") + for column in columns: + if column['name'] != "value": + continue + value_type = column['type'] + self.assertIsInstance(value_type, sqlalchemy.Numeric) + self.assertEqual(value_type.scale, 0) + self.assertEqual(value_type.precision, 10) + + def _check_15f7375ca737(self, connection): + inspector = sqlalchemy.inspect(connection) + columns = inspector.get_columns("efficacy_indicators") + for column in columns: + if column['name'] == "value": + value_type = column['type'] + self.assertIsInstance(value_type, sqlalchemy.Numeric) + self.assertEqual(value_type.scale, 0) + self.assertEqual(value_type.precision, 10) + elif column['name'] == "data": + value_type = column['type'] + self.assertIsInstance(value_type, sqlalchemy.Float) + + def test_migration_revisions(self): + with self.engine.begin() as connection: + self.alembic_config.attributes["connection"] = connection + script = ScriptDirectory.from_config(self.alembic_config) + revisions = [x.revision for x in script.walk_revisions()] + + # for some reason, 'walk_revisions' gives us the revisions in + # reverse chronological order so we have to invert this + revisions.reverse() + + for revision in revisions: + LOG.info('Testing revision %s', revision) + self._apply_migration(connection, revision) + + +class MySQLDbDataMigrationsTestCase(MySQLDbMigrationsTestCase): + + def _create_manual_efficacy_indicator(self, connection, **kwargs): + eff_ind_values = utils.get_test_efficacy_indicator(**kwargs) + metadata = sqlalchemy.MetaData() + metadata.reflect(bind=self.engine) + eff_ind_table = sqlalchemy.Table('efficacy_indicators', metadata) + connection.execute(eff_ind_table.insert(), eff_ind_values) + connection.commit() + + def _pre_upgrade_15f7375ca737(self, connection): + """Add data to the database before applying the 15f7375ca737 revision. + + This data will then be checked after applying the revision to ensure it + was not affected by the db upgrade. + + """ + self.goal = utils.create_test_goal( + id=1, uuid=w_utils.generate_uuid(), + name="GOAL_1", display_name='Goal 1') + self.strategy = utils.create_test_strategy( + id=1, uuid=w_utils.generate_uuid(), + name="STRATEGY_ID_1", display_name='My Strategy 1') + self.audit_template = utils.create_test_audit_template( + name="Audit Template", id=1, uuid=None) + self.audit = utils.create_test_audit( + audit_template_id=self.audit_template.id, id=1, uuid=None, + name="AUDIT_1") + self.action_plan = utils.create_test_action_plan( + audit_id=self.audit.id, id=1, uuid=None) + + self._create_manual_efficacy_indicator( + connection, + action_plan_id=self.action_plan.id, id=1, uuid=None, + name="efficacy_indicator1", description="Test Indicator 1", + value=1.01234567912345678) + + def _check_15f7375ca737(self, connection): + """Check data integrity after the database migration.""" + # check that creating a new efficacy_indicator after the migration + # works + utils.create_test_efficacy_indicator( + action_plan_id=self.action_plan.id, id=2, uuid=None, + name="efficacy_indicator1", description="Test Indicator 2", + value=0.01234567912345678) + db_efficacy_indicator = self.dbapi.get_efficacy_indicator_by_id( + self.context, 2) + self.assertAlmostEqual(db_efficacy_indicator.value, 0.012, places=3) + # check that getting the efficacy_indicator using the + # get_efficacy_indicator_list method reports the correct values + efficacy_indicators = self.dbapi.get_efficacy_indicator_list( + self.context) + self.assertEqual(len(efficacy_indicators), 2) + self.assertEqual(efficacy_indicators[0].id, 1) + self.assertAlmostEqual(efficacy_indicators[0].value, + 1.00, places=3) + + self.assertEqual(efficacy_indicators[1].id, 2) + self.assertAlmostEqual(efficacy_indicators[1].value, + 0.012, places=3) + # check that the existing data is there after the migration + db_goal = self.dbapi.get_goal_by_id(self.context, 1) + self.assertEqual(db_goal.name, "GOAL_1") + db_strategy = self.dbapi.get_strategy_by_id(self.context, 1) + self.assertEqual(db_strategy.name, "STRATEGY_ID_1") + db_audit_template = self.dbapi.get_audit_template_by_id( + self.context, 1) + self.assertEqual(db_audit_template.name, "Audit Template") + db_audit = self.dbapi.get_audit_by_id(self.context, 1) + self.assertEqual(db_audit.name, "AUDIT_1") + db_efficacy_indicator_1 = self.dbapi.get_efficacy_indicator_by_id( + self.context, 1) + self.assertAlmostEqual(db_efficacy_indicator_1.data, + 1.00, places=2) + self.assertAlmostEqual(db_efficacy_indicator_1.value, + 1.00, places=2) + self.assertEqual(db_efficacy_indicator_1.name, "efficacy_indicator1") + + def test_migration_revisions(self): + with self.engine.begin() as connection: + self.alembic_config.attributes["connection"] = connection + script = ScriptDirectory.from_config(self.alembic_config) + revisions = [x.revision for x in script.walk_revisions()] + + # for some reason, 'walk_revisions' gives us the revisions in + # reverse chronological order so we have to invert this + revisions.reverse() + + for revision in revisions: + LOG.info('Testing revision %s', revision) + self._apply_migration(connection, revision)