From 8fc98631b9260b3f3c4c210b015f9e835f78b3f5 Mon Sep 17 00:00:00 2001 From: Smruti Soumitra Khuntia Date: Wed, 31 Oct 2018 12:03:42 +0530 Subject: [PATCH] Revision diffing issue with revision rollback. * Fix for diffing issue after rollback in conjunction with created and deleted buckets. * Changed rollback function to check against the full set of documents for a revision instead of just the documents at that particular revision * Created a document_delete function to encapsulate document deletion * Added additional test case to check that a rollback to something other than 0 deletes the created buckets in between Co-Authored-By: Michael Beaver Change-Id: I0d57e67d68def1f15255a8c89290e8c70deedc03 --- deckhand/db/sqlalchemy/api.py | 168 ++++++++++++++---- .../revision-diff-rollback-null-success.yaml | 99 +++++++++++ .../tests/unit/db/test_revision_rollback.py | 83 ++++++++- 3 files changed, 316 insertions(+), 34 deletions(-) create mode 100644 deckhand/tests/functional/gabbits/revision-diff/revision-diff-rollback-null-success.yaml diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index f6425876..b0e8d782 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/deckhand/db/sqlalchemy/api.py @@ -188,28 +188,11 @@ def documents_create(bucket_name, documents, session=None): 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'] - doc['layer'] = d['layer'] - doc['data'] = {} - doc['meta'] = d['metadata'] - doc['data_hash'] = _make_hash({}) - doc['metadata_hash'] = _make_hash({}) - doc['bucket_id'] = bucket['id'] - doc['revision_id'] = revision['id'] + doc = document_delete(d, revision['id'], bucket, + session=session) - # Save and mark the document as `deleted` in the database. - try: - doc.save(session=session) - except db_exception.DBDuplicateEntry: - raise errors.DuplicateDocumentExists( - schema=doc['schema'], layer=doc['layer'], - name=doc['name'], bucket=bucket['name']) - doc.safe_delete(session=session) deleted_documents.append(doc) - resp.append(doc.to_dict()) + resp.append(doc) if documents_to_create: LOG.debug( @@ -240,6 +223,81 @@ def documents_create(bucket_name, documents, session=None): return resp +def document_delete(document, revision_id, bucket, session=None): + """Delete a document + + Creates a new document with the bare minimum information about the document + that is to be deleted, and then sets the appropriate deleted fields + + :param document: document object/dict to be deleted + :param revision_id: id of the revision where the document is to be deleted + :param bucket: bucket object/dict where the document will be deleted from + :param session: Database session object. + :return: dict representation of deleted document + """ + session = session or get_session() + + doc = models.Document() + # Store bare minimum information about the document. + doc['schema'] = document['schema'] + doc['name'] = document['name'] + doc['layer'] = document['layer'] + doc['data'] = {} + doc['meta'] = document['metadata'] + doc['data_hash'] = _make_hash({}) + doc['metadata_hash'] = _make_hash({}) + doc['bucket_id'] = bucket['id'] + doc['revision_id'] = revision_id + + # Save and mark the document as `deleted` in the database. + try: + doc.save(session=session) + except db_exception.DBDuplicateEntry: + raise errors.DuplicateDocumentExists( + schema=doc['schema'], layer=doc['layer'], + name=doc['name'], bucket=bucket['name']) + doc.safe_delete(session=session) + + return doc.to_dict() + + +def documents_delete_from_buckets_list(bucket_names, session=None): + """Delete all documents in the provided list of buckets + + :param bucket_names: list of bucket names for which the associated + buckets and their documents need to be deleted. + :param session: Database session object. + :returns: A new model.Revisions object after all the documents have been + deleted. + """ + session = session or get_session() + + with session.begin(): + # Create a new revision + revision = models.Revision() + revision.save(session=session) + + for bucket_name in bucket_names: + + documents_to_delete = [ + d for d in revision_documents_get(bucket_name=bucket_name, + session=session) + if "deleted" not in d or not d['deleted'] + ] + + bucket = bucket_get_or_create(bucket_name, session=session) + + if documents_to_delete: + LOG.debug('Deleting documents: %s.', + [eng_utils.meta(d) for d in documents_to_delete]) + + for document in documents_to_delete: + document_delete(document, revision['id'], bucket, + session=session) + + return revision + + def _documents_create(bucket_name, documents, session=None): documents = copy.deepcopy(documents) session = session or get_session() @@ -456,6 +514,24 @@ def bucket_get_or_create(bucket_name, session=None): #################### +def bucket_get_all(session=None, **filters): + """Return list of all buckets. + + :param session: Database session object. + :returns: List of dictionary representations of retrieved buckets. + """ + session = session or get_session() + + buckets = session.query(models.Bucket)\ + .all() + result = [] + for bucket in buckets: + revision_dict = bucket.to_dict() + if utils.deepfilter(revision_dict, **filters): + result.append(bucket) + + return result + def revision_create(session=None): """Create a revision. @@ -777,30 +853,61 @@ def revision_rollback(revision_id, latest_revision, session=None): :returns: The newly created revision. """ session = session or get_session() + latest_revision_docs = revision_documents_get(latest_revision['id'], + session=session) latest_revision_hashes = [ - (d['data_hash'], d['metadata_hash']) - for d in latest_revision['documents']] + (d['data_hash'], d['metadata_hash']) for d in latest_revision_docs + ] if latest_revision['id'] == revision_id: LOG.debug('The revision being rolled back to is the current revision.' 'Expect no meaningful changes.') if revision_id == 0: - # Placeholder revision as revision_id=0 doesn't exist. - orig_revision = {'documents': []} + # Delete all existing documents in all buckets + all_buckets = bucket_get_all(deleted=False) + bucket_names = [str(b['name']) for b in all_buckets] + revision = documents_delete_from_buckets_list(bucket_names, + session=session) + + return revision.to_dict() else: - orig_revision = revision_get(revision_id, session=session) + # Sorting the documents so the documents in the new revision are in + # the same order as the previous revision to support stable testing + orig_revision_docs = sorted(revision_documents_get(revision_id, + session=session), + key=lambda d: d['id']) # A mechanism for determining whether a particular document has changed # between revisions. Keyed with the document_id, the value is True if # it has changed, else False. doc_diff = {} - for orig_doc in orig_revision['documents']: + # List of unique buckets that exist in this revision + unique_buckets = [] + for orig_doc in orig_revision_docs: if ((orig_doc['data_hash'], orig_doc['metadata_hash']) not in latest_revision_hashes): doc_diff[orig_doc['id']] = True else: doc_diff[orig_doc['id']] = False + if orig_doc['bucket_id'] not in unique_buckets: + unique_buckets.append(orig_doc['bucket_id']) + + # We need to find which buckets did not exist at this revision + buckets_to_delete = [] + all_buckets = bucket_get_all(deleted=False) + for bucket in all_buckets: + if bucket['id'] not in unique_buckets: + buckets_to_delete.append(str(bucket['name'])) + + # Create the new revision, + if len(buckets_to_delete) > 0: + new_revision = documents_delete_from_buckets_list(buckets_to_delete, + session=session) + else: + new_revision = models.Revision() + with session.begin(): + new_revision.save(session=session) # No changes have been made between the target revision to rollback to # and the latest revision. @@ -809,13 +916,8 @@ def revision_rollback(revision_id, latest_revision, session=None): 'as that of the current revision. Expect no meaningful ' 'changes.') - # Create the new revision, - new_revision = models.Revision() - with session.begin(): - new_revision.save(session=session) - # Create the documents for the revision. - for orig_document in orig_revision['documents']: + for orig_document in orig_revision_docs: orig_document['revision_id'] = new_revision['id'] orig_document['meta'] = orig_document.pop('metadata') @@ -831,7 +933,7 @@ def revision_rollback(revision_id, latest_revision, session=None): if doc_diff[orig_document['id']]: new_document['orig_revision_id'] = new_revision['id'] else: - new_document['orig_revision_id'] = orig_revision['id'] + new_document['orig_revision_id'] = revision_id with session.begin(): new_document.save(session=session) diff --git a/deckhand/tests/functional/gabbits/revision-diff/revision-diff-rollback-null-success.yaml b/deckhand/tests/functional/gabbits/revision-diff/revision-diff-rollback-null-success.yaml new file mode 100644 index 00000000..b161a089 --- /dev/null +++ b/deckhand/tests/functional/gabbits/revision-diff/revision-diff-rollback-null-success.yaml @@ -0,0 +1,99 @@ +# Test success path for revision diff with rollback to null revision. +# +# 1. Purges existing data to ensure test isolation +# 2. Creates an initial document bucket +# 3. Rollback to null (i.e revision 0) +# 4. Verify diff between null (revision 0) and rollback revision to null +# 5. Verify diff between rollback revision to null and null (revision 0) +# 6. Create another document after rollback +# 7. Verify diff between rollback revision and present revision +# 8. Verify diff between present revision and rollback revision + +defaults: + request_headers: + content-type: application/x-yaml + response_headers: + content-type: application/x-yaml + verbose: true + +tests: + - name: purge + desc: Begin testing from known state. + DELETE: /api/v1.0/revisions + status: 204 + response_headers: null + + - name: create_a + desc: Create documents in bucket a + PUT: /api/v1.0/buckets/bucket_a/documents + status: 200 + data: |- + --- + schema: example/Kind/v1 + metadata: + schema: metadata/Document/v1 + name: doc-a + storagePolicy: cleartext + layeringDefinition: + abstract: false + layer: site + data: + value: 1 + ... + + - name: rollback_to_null + desc: Rollback to revision 0 + POST: /api/v1.0/rollback/0 + status: 201 + + - name: verify_null_with_rollback_to_null + desc: Validates response for null diff rollback to null revision + GET: /api/v1.0/revisions/0/diff/$HISTORY['rollback_to_null'].$RESPONSE['$.[0].id'] + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[0]: {} + + - name: verify_rollback_to_null_with_null + desc: Validates response for rollback to null revision with null revision + GET: /api/v1.0/revisions/$HISTORY['rollback_to_null'].$RESPONSE['$.[0].id']/diff/0 + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[0]: {} + + - name: create_b + desc: Create documents in bucket b + PUT: /api/v1.0/buckets/bucket_b/documents + status: 200 + data: |- + --- + schema: example/Kind/v1 + metadata: + schema: metadata/Document/v1 + name: doc-b + storagePolicy: cleartext + layeringDefinition: + abstract: false + layer: site + data: + value: 2 + ... + + - name: verify_rollback_with_present + desc: Validates response for diff with rollack to null and create bucket b + GET: /api/v1.0/revisions/$HISTORY['rollback_to_null'].$RESPONSE['$.[0].id']/diff/$HISTORY['create_b'].$RESPONSE['$.[0].status.revision'] + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[0]: + bucket_b: created + + - name: verify_present_with_rollback + desc: Validates response for diff with rollack to null and create bucket b + GET: /api/v1.0/revisions/$HISTORY['create_b'].$RESPONSE['$.[0].status.revision']/diff/$HISTORY['rollback_to_null'].$RESPONSE['$.[0].id'] + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[0]: + bucket_b: created \ No newline at end of file diff --git a/deckhand/tests/unit/db/test_revision_rollback.py b/deckhand/tests/unit/db/test_revision_rollback.py index 602ec1cd..74aa4aa1 100644 --- a/deckhand/tests/unit/db/test_revision_rollback.py +++ b/deckhand/tests/unit/db/test_revision_rollback.py @@ -109,7 +109,7 @@ class TestRevisionRollback(base.DeckhandWithDBTestCase): rollback_revision = self.rollback_revision(0) rollback_documents = self.list_revision_documents( - rollback_revision['id'], include_history=False) + rollback_revision['id'], include_history=False, deleted=False) self.assertEqual(orig_revision_id + 1, rollback_revision['id']) self.assertEmpty(rollback_documents) @@ -123,6 +123,87 @@ class TestRevisionRollback(base.DeckhandWithDBTestCase): self.assertEqual(1, rollback_revision['id']) self.assertEmpty(rollback_documents) + def test_rollback_to_revision_n_removes_buckets(self): + """Rolling back to revision 1 should create a revision without the + buckets in between. + """ + payload_a = base.DocumentFixture.get_minimal_multi_fixture(count=2) + bucket_name_a = test_utils.rand_name('bucket') + created_documents_a = self.create_documents(bucket_name_a, payload_a) + + payload_b = base.DocumentFixture.get_minimal_multi_fixture(count=3) + bucket_name_b = test_utils.rand_name('bucket') + self.create_documents(bucket_name_b, payload_b) + + payload_c = base.DocumentFixture.get_minimal_multi_fixture(count=3) + bucket_name_c = test_utils.rand_name('bucket') + created_documents_c = self.create_documents(bucket_name_c, payload_c) + orig_revision_id_c = created_documents_c[0]['revision_id'] + + rollback_revision = self.rollback_revision(1) + rollback_documents = self.list_revision_documents( + rollback_revision['id'], include_history=False, deleted=False) + self.assertEqual(orig_revision_id_c + 1, rollback_revision['id']) + sorted_roll = sorted(rollback_documents, key=lambda k: k['id']) + sorted_a = sorted(created_documents_a, key=lambda k: k['id']) + self.assertEqual(len(created_documents_a), len(rollback_documents)) + ignored_fields = ['created_at', + 'updated_at', + 'orig_revision_id', + 'revision_id', + 'id'] + self.assertDictItemsAlmostEqual(sorted_a, sorted_roll, ignored_fields) + + def test_rollback_with_deleting_buckets(self): + """Even if deleting entire buckets before a rollback, rolling back to + a revision should have all the same documents + """ + # Revision 1: create bucket a + payload_a = base.DocumentFixture.get_minimal_multi_fixture(count=2) + bucket_name_a = test_utils.rand_name('bucket') + self.create_documents(bucket_name_a, payload_a) + + # Revision 2: create bucket b + payload_b = base.DocumentFixture.get_minimal_multi_fixture(count=3) + bucket_name_b = test_utils.rand_name('bucket') + created_documents_b = self.create_documents(bucket_name_b, payload_b) + orig_revision_id_b = created_documents_b[0]['revision_id'] + revision_2_docs = self.list_revision_documents(orig_revision_id_b) + + # Revision 3: explicitly delete bucket b + self.create_documents(bucket_name_b, []) + + # Revision 4: rollback to 2, bucket a and b should exist + rollback_revision = self.rollback_revision(orig_revision_id_b) + rollback_docs = self.list_revision_documents( + rollback_revision['id'], include_history=False, deleted=False) + + self.assertEqual(4, rollback_revision['id']) + self.assertEqual(len(revision_2_docs), len(rollback_docs)) + sorted_roll = sorted(rollback_docs, key=lambda k: k['id']) + sorted_b = sorted(revision_2_docs, key=lambda k: k['id']) + ignored_fields = ['created_at', + 'updated_at', + 'orig_revision_id', + 'revision_id', + 'id'] + self.assertDictItemsAlmostEqual(sorted_b, sorted_roll, ignored_fields) + + # Revision 5: rollback to 0, should delete everything + self.rollback_revision(0) + + # Revision 6: rollback to 2, bucket a and b should exist + rollback_revision = self.rollback_revision(orig_revision_id_b) + rollback_docs = self.list_revision_documents( + rollback_revision['id'], include_history=False, deleted=False) + revision_2_docs = self.list_revision_documents(orig_revision_id_b) + + self.assertEqual(6, rollback_revision['id']) + self.assertEqual(len(revision_2_docs), len(rollback_docs)) + sorted_roll = sorted(rollback_docs, key=lambda k: k['id']) + sorted_b = sorted(revision_2_docs, key=lambda k: k['id']) + self.assertDictItemsAlmostEqual(sorted_b, sorted_roll, ignored_fields) + class TestRevisionRollbackNegative(base.DeckhandWithDBTestCase):