diff --git a/deckhand/control/buckets.py b/deckhand/control/buckets.py index 66bcea5f..50d5b313 100644 --- a/deckhand/control/buckets.py +++ b/deckhand/control/buckets.py @@ -62,7 +62,11 @@ class BucketsResource(api_base.BaseResource): documents = self._encrypt_secret_documents(documents) created_documents = self._create_revision_documents( - bucket_name, documents, validations) + bucket_name, documents) + + if created_documents: + revision_id = created_documents[0]['revision_id'] + self._create_revision_validations(revision_id, validations) resp.body = self.view_builder.list(created_documents) resp.status = falcon.HTTP_200 @@ -75,14 +79,17 @@ class BucketsResource(api_base.BaseResource): document['data'] = secret_ref return documents - def _create_revision_documents(self, bucket_name, documents, - validations): + def _create_revision_documents(self, bucket_name, documents): try: - created_documents = db_api.documents_create( - bucket_name, documents, validations=validations) + created_documents = db_api.documents_create(bucket_name, documents) except (deckhand_errors.DuplicateDocumentExists, deckhand_errors.SingletonDocumentConflict) as e: with excutils.save_and_reraise_exception(): LOG.exception(e.format_message()) return created_documents + + def _create_revision_validations(self, revision_id, validations): + for validation in validations: + db_api.validation_create(revision_id, validation['name'], + validation) diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index 2e416a1e..0de2749c 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/deckhand/db/sqlalchemy/api.py @@ -149,8 +149,7 @@ def require_unique_document_schema(schema=None): @require_unique_document_schema(types.LAYERING_POLICY_SCHEMA) -def documents_create(bucket_name, documents, validations=None, - session=None): +def documents_create(bucket_name, documents, session=None): """Create a set of documents and associated bucket. If no changes are detected, a new revision will not be created. This @@ -160,45 +159,43 @@ def documents_create(bucket_name, documents, validations=None, :param bucket_name: The name of the bucket with which to associate created documents. :param documents: List of documents to be created. - :param validation_policies: List of validation policies to be created. :param session: Database session object. :returns: List of created documents in dictionary format. :raises DocumentExists: If the document already exists in the DB for any bucket. """ session = session or get_session() - documents_to_create = _documents_create(bucket_name, documents, session) - resp = [] - # The documents to be deleted are computed by comparing the documents for - # the previous revision (if it exists) that belong to `bucket_name` with - # `documents`: the difference between the former and the latter. - document_history = [ - d for d in revision_documents_get(bucket_name=bucket_name) - ] - documents_to_delete = [ - h for h in document_history if _meta(h) not in [ - _meta(d) for d in documents] - ] + with session.begin(): + documents_to_create = _documents_create(bucket_name, documents, + session=session) - # Only create a revision if any docs have been created, changed or deleted. - if any([documents_to_create, documents_to_delete]): - bucket = bucket_get_or_create(bucket_name) - revision = revision_create() - if validations: - for validation in validations: - validation_create(revision['id'], validation['name'], - validation) + # The documents to be deleted are computed by comparing the documents + # for the previous revision (if it exists) that belong to `bucket_name` + # with `documents`: the difference between the former and the latter. + document_history = [ + d for d in revision_documents_get(bucket_name=bucket_name, + session=session) + ] + documents_to_delete = [ + h for h in document_history if _meta(h) not in [ + _meta(d) for d in documents] + ] - if documents_to_delete: - LOG.debug('Deleting documents: %s.', - [_meta(d) for d in documents_to_delete]) - deleted_documents = [] + # Only create a revision if any docs have been created, changed or + # deleted. + if any([documents_to_create, documents_to_delete]): + revision = revision_create(session=session) + bucket = bucket_get_or_create(bucket_name, session=session) - for d in documents_to_delete: - doc = models.Document() - with session.begin(): + if documents_to_delete: + LOG.debug('Deleting documents: %s.', + [_meta(d) for d in documents_to_delete]) + deleted_documents = [] + + for d in documents_to_delete: + doc = models.Document() # Store bare minimum information about the document. doc['schema'] = d['schema'] doc['name'] = d['name'] @@ -219,14 +216,16 @@ def documents_create(bucket_name, documents, validations=None, name=doc['name'], bucket=bucket['name']) doc.safe_delete(session=session) deleted_documents.append(doc) - resp.append(doc.to_dict()) + resp.append(doc.to_dict()) - if documents_to_create: - LOG.debug( - 'Creating documents: %s.', [(d['schema'], d['layer'], d['name']) - for d in documents_to_create]) - for doc in documents_to_create: - with session.begin(): + if documents_to_create: + LOG.debug( + 'Creating documents: %s.', [ + (d['schema'], d['layer'], d['name']) + for d in documents_to_create + ] + ) + for doc in documents_to_create: doc['bucket_id'] = bucket['id'] doc['revision_id'] = revision['id'] if not doc.get('orig_revision_id'): @@ -239,7 +238,7 @@ def documents_create(bucket_name, documents, validations=None, schema=doc['schema'], layer=doc['layer'], name=doc['name'], bucket=bucket['name']) - resp.append(doc.to_dict()) + resp.append(doc.to_dict()) # NOTE(fmontei): The orig_revision_id is not copied into the # revision_id for each created document, because the revision_id here # should reference the just-created revision. In case the user needs @@ -256,8 +255,7 @@ def _documents_create(bucket_name, documents, session=None): def _document_create(document): model = models.Document() - with session.begin(): - model.update(document) + model.update(document) return model for document in documents: @@ -457,9 +455,8 @@ def bucket_get_or_create(bucket_name, session=None): .one() except sa_orm.exc.NoResultFound: bucket = models.Bucket() - with session.begin(): - bucket.update({'name': bucket_name}) - bucket.save(session=session) + bucket.update({'name': bucket_name}) + bucket.save(session=session) return bucket.to_dict() @@ -476,8 +473,7 @@ def revision_create(session=None): session = session or get_session() revision = models.Revision() - with session.begin(): - revision.save(session=session) + revision.save(session=session) return revision.to_dict() diff --git a/deckhand/tests/unit/db/test_documents.py b/deckhand/tests/unit/db/test_documents.py index 3e2af79e..18ab94c4 100644 --- a/deckhand/tests/unit/db/test_documents.py +++ b/deckhand/tests/unit/db/test_documents.py @@ -12,6 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. +import mock +import testtools + +from deckhand.db.sqlalchemy import api as db_api +from deckhand import errors from deckhand import factories from deckhand.tests import test_utils from deckhand.tests.unit.db import base @@ -299,3 +304,34 @@ class TestDocuments(base.TestDbBase): sorted(orig_documents, key=lambda d: d['created_at']), sorted(duplicate_documents, key=lambda d: d['created_at']), ignore=['created_at', 'updated_at', 'revision_id', 'id']) + + def test_document_creation_failure_rolls_back_in_flight_revision(self): + """Regression test that an exception that occurs between creation of + a revision and creation of all bucket documents results in the + in-flight database objects getting rolled back. + """ + bucket_name = test_utils.rand_name('bucket') + payload = base.DocumentFixture.get_minimal_fixture() + + original_revision_create = db_api.revision_create + + # Mock the revision_create function so we can assert whether it was + # called, but still call the real function. + with mock.patch.object( + db_api, 'revision_create', autospec=True, + side_effect=original_revision_create) as m_revision_create: + # Raise any exception on a function call following the creation of + # a revision. The transaction will not complete so the result will + # not be committed to the database. + with mock.patch.object(db_api, 'bucket_get_or_create', + autospec=True, + side_effect=errors.DuplicateDocumentExists): + with testtools.ExpectedException( + errors.DuplicateDocumentExists): + self.create_documents(bucket_name, [payload]) + + # Validate that the actual revision_create call was indeed invoked. + self.assertTrue(m_revision_create.called) + # Finally validate that the revision doesn't exist. + with testtools.ExpectedException(errors.RevisionNotFound): + self.show_revision(1)