From 58d85968fa309203bbc77604a29e11c97a948ea8 Mon Sep 17 00:00:00 2001 From: Luka Peschke Date: Tue, 20 Aug 2019 15:08:36 +0200 Subject: [PATCH] Fix StateManager.set_state() logic This fixes the logic of the StateManager's set_state() method: if, by accident, the state of an identifier is set to the same timestamp as it currently is at, we won't attempt to create another object. The 'uq_cloudkitty_storage_states_identifier' constraint was preventing a bug from happening, but this fixes the code logic. Change-Id: I530e63dbd67c1357d4aac964fffcdb45ce495768 --- cloudkitty/storage_state/__init__.py | 9 +++++---- cloudkitty/tests/test_storage_state.py | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/cloudkitty/storage_state/__init__.py b/cloudkitty/storage_state/__init__.py index 46b40442..c965aedf 100644 --- a/cloudkitty/storage_state/__init__.py +++ b/cloudkitty/storage_state/__init__.py @@ -97,7 +97,7 @@ class StateManager(object): # In case the identifier exists with empty columns, update them if not r: - # NOTE(peschk_l): We must use == instead of 'is' because sqlachmey + # NOTE(peschk_l): We must use == instead of 'is' because sqlalchemy # overloads this operator r = q.filter(self.model.identifier == identifier). \ filter(self.model.scope_key == None). \ @@ -138,9 +138,10 @@ class StateManager(object): r = self._get_db_item( session, identifier, fetcher, collector, scope_key) - if r and r.state != state: - r.state = state - session.commit() + if r: + if r.state != state: + r.state = state + session.commit() else: state_object = self.model( identifier=identifier, diff --git a/cloudkitty/tests/test_storage_state.py b/cloudkitty/tests/test_storage_state.py index f4bf149a..e13e51e8 100644 --- a/cloudkitty/tests/test_storage_state.py +++ b/cloudkitty/tests/test_storage_state.py @@ -108,3 +108,29 @@ class StateManagerTest(tests.TestCase): with mock.patch('cloudkitty.db.get_session'): self._test_x_state_no_column_update( lambda x: self._state.set_state(x, datetime(2042, 1, 1))) + + def test_set_state_does_not_duplicate_entries(self): + state = datetime(2042, 1, 1) + _, query_mock = self._get_query_mock( + self._get_r_mock('a', 'b', 'c', state)) + with mock.patch( + 'oslo_db.sqlalchemy.utils.model_query', + new=query_mock), mock.patch('cloudkitty.db.get_session') as sm: + sm.return_value = session_mock = mock.MagicMock() + self._state.set_state('fake_identifier', state) + session_mock.commit.assert_not_called() + session_mock.add.assert_not_called() + + def test_set_state_does_update_state(self): + r_mock = self._get_r_mock('a', 'b', 'c', datetime(2000, 1, 1)) + _, query_mock = self._get_query_mock(r_mock) + new_state = datetime(2042, 1, 1) + with mock.patch( + 'oslo_db.sqlalchemy.utils.model_query', + new=query_mock), mock.patch('cloudkitty.db.get_session') as sm: + sm.return_value = session_mock = mock.MagicMock() + self.assertNotEqual(r_mock.state, new_state) + self._state.set_state('fake_identifier', new_state) + self.assertEqual(r_mock.state, new_state) + session_mock.commit.assert_called_once() + session_mock.add.assert_not_called()