From 1e25ccd51424b1129b36acf7a6488cb2d272c8af Mon Sep 17 00:00:00 2001 From: J08nY Date: Tue, 4 Jul 2017 17:54:47 +0200 Subject: Save the complete workflow stack, not only last step. --- src/mailman/app/tests/test_workflow.py | 2 +- src/mailman/app/workflow.py | 22 ++--- .../versions/7c5b39d1ecc4_workflow_steps.py | 90 ++++++++++++++++++++ src/mailman/database/tests/test_migrations.py | 99 ++++++++++++++++++++++ src/mailman/interfaces/workflow.py | 8 +- src/mailman/model/tests/test_workflow.py | 22 ++--- src/mailman/model/workflow.py | 6 +- 7 files changed, 215 insertions(+), 34 deletions(-) create mode 100644 src/mailman/database/alembic/versions/7c5b39d1ecc4_workflow_steps.py diff --git a/src/mailman/app/tests/test_workflow.py b/src/mailman/app/tests/test_workflow.py index e82001540..70e9a5035 100644 --- a/src/mailman/app/tests/test_workflow.py +++ b/src/mailman/app/tests/test_workflow.py @@ -153,7 +153,7 @@ class TestWorkflow(unittest.TestCase): # Save the state of an old version of the workflow that would not have # the cat attribute. state_manager.save( - self._workflow.token, 'first', + self._workflow.token, '["first"]', json.dumps({'ant': 1, 'bee': 2})) # Restore in the current version that needs the cat attribute. new_workflow = MyWorkflow() diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index 92b78c184..e705668f8 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -56,7 +56,7 @@ class Workflow: self._next.append(step) def _pop(self): - name = self._next.popleft() + name = self._next.pop() step = getattr(self, '_step_{}'.format(name)) self._count += 1 if self.debug: # pragma: nocover @@ -119,20 +119,12 @@ class Workflow: assert self.token, 'Workflow token must be set' state_manager = getUtility(IWorkflowStateManager) data = {attr: getattr(self, attr) for attr in self.SAVE_ATTRIBUTES} - # Note: only the next step is saved, not the whole stack. This is not - # an issue in practice, since there's never more than a single step in - # the queue anyway. If we want to support more than a single step in - # the queue *and* want to support state saving/restoring, change this - # method and the restore() method. + # Save the workflow stack. if len(self._next) == 0: - step = None - elif len(self._next) == 1: - step = self._next[0] + steps = '[]' else: - raise AssertionError( - "Can't save a workflow state with more than one step " - "in the queue") - state_manager.save(self.token, step, json.dumps(data)) + steps = json.dumps(list(self._next)) + state_manager.save(self.token, steps, json.dumps(data)) def restore(self): state_manager = getUtility(IWorkflowStateManager) @@ -141,8 +133,8 @@ class Workflow: # The token doesn't exist in the database. raise LookupError(self.token) self._next.clear() - if state.step: - self._next.append(state.step) + if state.steps: + self._next.extend(json.loads(state.steps)) data = json.loads(state.data) for attr in self.SAVE_ATTRIBUTES: try: diff --git a/src/mailman/database/alembic/versions/7c5b39d1ecc4_workflow_steps.py b/src/mailman/database/alembic/versions/7c5b39d1ecc4_workflow_steps.py new file mode 100644 index 000000000..c2daffd63 --- /dev/null +++ b/src/mailman/database/alembic/versions/7c5b39d1ecc4_workflow_steps.py @@ -0,0 +1,90 @@ +# Copyright (C) 2015-2017 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Save whole workflow stack in workflowstate. + +Revision ID: 7c5b39d1ecc4 +Revises: 4bd95c99b2e7 +Create Date: 2017-07-04 16:45:36.470746 + +""" + +import json +import sqlalchemy as sa + +from alembic import op +from mailman.database.types import SAUnicode + +# revision identifiers, used by Alembic. +revision = '7c5b39d1ecc4' +down_revision = '4bd95c99b2e7' + +old_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('step', SAUnicode), + sa.sql.column('data', SAUnicode) + ) +new_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('steps', SAUnicode), + sa.sql.column('data', SAUnicode) + ) + + +def upgrade(): + # Rename the column + with op.batch_alter_table('workflowstate') as batch_op: + batch_op.alter_column('step', new_column_name='steps', + existing_type=SAUnicode) + + connection = op.get_bind() + for token, step, data in connection.execute( + new_state_table.select()).fetchall(): + # Wrap the single step in a JSON array. + if step is not None: + new_steps = json.dumps([step]) + else: + new_steps = '[]' + connection.execute( + new_state_table.update().where( + new_state_table.c.token == token).values( + steps=new_steps) + ) + + +def downgrade(): + # Rename back the column + connection = op.get_bind() + with op.batch_alter_table('workflowstate') as batch_op: + batch_op.alter_column('steps', new_column_name='step', + existing_type=SAUnicode) + + for token, steps, data in connection.execute( + old_state_table.select()).fetchall(): + # Extract and save at least the last state. + step_stack = json.loads(steps) + if len(step_stack) == 0: + new_step = None + else: + new_step = step_stack.pop() + connection.execute( + old_state_table.update().where( + old_state_table.c.token == token).values( + step=new_step) + ) diff --git a/src/mailman/database/tests/test_migrations.py b/src/mailman/database/tests/test_migrations.py index ce1bc101b..5b686d13b 100644 --- a/src/mailman/database/tests/test_migrations.py +++ b/src/mailman/database/tests/test_migrations.py @@ -18,6 +18,7 @@ """Test database schema migrations with Alembic""" import os +import json import unittest import sqlalchemy as sa import alembic.command @@ -495,3 +496,101 @@ class TestMigrations(unittest.TestCase): self.assertEqual( len(list(config.db.store.execute(mlist_table.select()))), 0) + + def test_7c5b39d1ecc4_workflow_steps_upgrade(self): + old_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('step', SAUnicode), + sa.sql.column('data', SAUnicode) + ) + new_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('steps', SAUnicode), + sa.sql.column('data', SAUnicode) + ) + with transaction(): + # Start at the previous revision. + alembic.command.downgrade(alembic_cfg, '4bd95c99b2e7') + config.db.store.execute(old_state_table.insert().values([ + dict(token='12345', + step='some_step', + data='whatever data'), + dict(token='6789', + step=None, + data='other data') + ])) + + # Now upgrade. + alembic.command.upgrade(alembic_cfg, '7c5b39d1ecc4') + + token, steps, data = config.db.store.execute( + new_state_table.select().where( + new_state_table.c.token == '12345' + )).fetchone() + self.assertEqual(token, '12345') + self.assertEqual(steps, json.dumps(['some_step'])) + self.assertEqual(data, 'whatever data') + + token, steps, data = config.db.store.execute( + new_state_table.select().where( + new_state_table.c.token == '6789' + )).fetchone() + self.assertEqual(token, '6789') + self.assertEqual(steps, json.dumps([])) + self.assertEqual(data, 'other data') + + def test_7c5b39d1ecc4_workflow_steps_downgrade(self): + old_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('step', SAUnicode), + sa.sql.column('data', SAUnicode) + ) + new_state_table = sa.sql.table( + 'workflowstate', + sa.sql.column('token', SAUnicode), + sa.sql.column('steps', SAUnicode), + sa.sql.column('data', SAUnicode) + ) + with transaction(): + # Start at the revision. + alembic.command.downgrade(alembic_cfg, '7c5b39d1ecc4') + config.db.store.execute(new_state_table.insert().values([ + dict(token='12345', + steps=json.dumps(['next_step', 'some_step']), + data='whatever data'), + dict(token='6789', + steps=json.dumps(['only_step']), + data='other data'), + dict(token='abcde', + steps=json.dumps([]), + data='another data') + ])) + # Now downgrade. + alembic.command.downgrade(alembic_cfg, '4bd95c99b2e7') + + token, step, data = config.db.store.execute( + old_state_table.select().where( + old_state_table.c.token == '12345' + )).fetchone() + self.assertEqual(token, '12345') + self.assertEqual(step, 'some_step') + self.assertEqual(data, 'whatever data') + + token, step, data = config.db.store.execute( + old_state_table.select().where( + old_state_table.c.token == '6789' + )).fetchone() + self.assertEqual(token, '6789') + self.assertEqual(step, 'only_step') + self.assertEqual(data, 'other data') + + token, step, data = config.db.store.execute( + old_state_table.select().where( + old_state_table.c.token == 'abcde' + )).fetchone() + self.assertEqual(token, 'abcde') + self.assertEqual(step, None) + self.assertEqual(data, 'another data') diff --git a/src/mailman/interfaces/workflow.py b/src/mailman/interfaces/workflow.py index 5b3582b58..a0a5cce31 100644 --- a/src/mailman/interfaces/workflow.py +++ b/src/mailman/interfaces/workflow.py @@ -27,7 +27,7 @@ class IWorkflowState(Interface): token = Attribute('A unique key identifying the workflow instance.') - step = Attribute("This workflow's next step.") + steps = Attribute("This workflow's next steps.") data = Attribute('Additional data (may be JSON-encoded).') @@ -36,13 +36,13 @@ class IWorkflowState(Interface): class IWorkflowStateManager(Interface): """The workflow states manager.""" - def save(token, step, data=None): + def save(token, steps, data=None): """Save the state of a workflow. :param token: A unique token identifying this workflow instance. :type token: str - :param step: The next step for this workflow. - :type step: str + :param steps: The next steps for this workflow. + :type steps: str :param data: Additional data (workflow-specific). :type data: str """ diff --git a/src/mailman/model/tests/test_workflow.py b/src/mailman/model/tests/test_workflow.py index 9cc5446b7..f969f395c 100644 --- a/src/mailman/model/tests/test_workflow.py +++ b/src/mailman/model/tests/test_workflow.py @@ -33,12 +33,12 @@ class TestWorkflow(unittest.TestCase): def test_save_restore_workflow(self): # Save and restore a workflow. token = 'bee' - step = 'cat' + steps = 'cat' data = 'dog' - self._manager.save(token, step, data) + self._manager.save(token, steps, data) state = self._manager.restore(token) self.assertEqual(state.token, token) - self.assertEqual(state.step, step) + self.assertEqual(state.steps, steps) self.assertEqual(state.data, data) def test_save_restore_workflow_without_step(self): @@ -48,17 +48,17 @@ class TestWorkflow(unittest.TestCase): self._manager.save(token, data=data) state = self._manager.restore(token) self.assertEqual(state.token, token) - self.assertIsNone(state.step) + self.assertIsNone(state.steps) self.assertEqual(state.data, data) def test_save_restore_workflow_without_data(self): # Save and restore a workflow that contains no data. token = 'bee' - step = 'cat' - self._manager.save(token, step) + steps = 'cat' + self._manager.save(token, steps) state = self._manager.restore(token) self.assertEqual(state.token, token) - self.assertEqual(state.step, step) + self.assertEqual(state.steps, steps) self.assertIsNone(state.data) def test_save_restore_workflow_without_step_or_data(self): @@ -67,7 +67,7 @@ class TestWorkflow(unittest.TestCase): self._manager.save(token) state = self._manager.restore(token) self.assertEqual(state.token, token) - self.assertIsNone(state.step) + self.assertIsNone(state.steps) self.assertIsNone(state.data) def test_restore_workflow_with_no_matching_token(self): @@ -106,13 +106,13 @@ class TestWorkflow(unittest.TestCase): self._manager.discard('token2') self.assertEqual(self._manager.count, 3) state = self._manager.restore('token1') - self.assertEqual(state.step, 'one') + self.assertEqual(state.steps, 'one') state = self._manager.restore('token2') self.assertIsNone(state) state = self._manager.restore('token3') - self.assertEqual(state.step, 'three') + self.assertEqual(state.steps, 'three') state = self._manager.restore('token4') - self.assertEqual(state.step, 'four') + self.assertEqual(state.steps, 'four') def test_discard_missing_workflow(self): self._manager.discard('bogus-token') diff --git a/src/mailman/model/workflow.py b/src/mailman/model/workflow.py index 3ca3412a6..b9d316840 100644 --- a/src/mailman/model/workflow.py +++ b/src/mailman/model/workflow.py @@ -34,7 +34,7 @@ class WorkflowState(Model): __tablename__ = 'workflowstate' token = Column(SAUnicode, primary_key=True) - step = Column(SAUnicode) + steps = Column(SAUnicode) data = Column(SAUnicode) @@ -44,9 +44,9 @@ class WorkflowStateManager: """See `IWorkflowStateManager`.""" @dbconnection - def save(self, store, token, step=None, data=None): + def save(self, store, token, steps=None, data=None): """See `IWorkflowStateManager`.""" - state = WorkflowState(token=token, step=step, data=data) + state = WorkflowState(token=token, steps=steps, data=data) store.add(state) @dbconnection -- cgit v1.2.3-70-g09d2