From cce6ddaf6e0ce11dc4fb7cceea518957b46a9edf Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Mon, 19 Mar 2018 20:43:20 +0000 Subject: [PATCH] Fix uniqueness not being enforced at DB level for documents UniqueConstraint is currently implemented incorrectly in terms of syntax in Deckhand's Document DB model. This PS fixes that. Now UniqueConstraint should be enforcing document uniqueness at DB level such that an error is thrown for duplicate documents (with same metadata.name and schema). Closes #17 Change-Id: I7d66457f471ec48b5766733046977117b509d592 --- deckhand/control/buckets.py | 2 +- deckhand/db/sqlalchemy/api.py | 18 +++++++++++++++--- deckhand/db/sqlalchemy/models.py | 8 ++++++-- deckhand/errors.py | 4 ++-- .../control/test_validations_controller.py | 5 +++-- .../tests/unit/db/test_documents_negative.py | 13 ++++++++++--- docs/source/exceptions.rst | 8 ++++---- 7 files changed, 41 insertions(+), 17 deletions(-) diff --git a/deckhand/control/buckets.py b/deckhand/control/buckets.py index 5ea19a6f..64ea0075 100644 --- a/deckhand/control/buckets.py +++ b/deckhand/control/buckets.py @@ -94,7 +94,7 @@ class BucketsResource(api_base.BaseResource): try: created_documents = db_api.documents_create( bucket_name, documents, validations=validations) - except (deckhand_errors.DocumentExists, + except (deckhand_errors.DuplicateDocumentExists, deckhand_errors.SingletonDocumentConflict) as e: raise falcon.HTTPConflict(description=e.format_message()) except Exception as e: diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index 1507f769..47b502f7 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/deckhand/db/sqlalchemy/api.py @@ -207,7 +207,12 @@ def documents_create(bucket_name, documents, validations=None, doc['revision_id'] = revision['id'] # Save and mark the document as `deleted` in the database. - doc.save(session=session) + try: + doc.save(session=session) + except db_exception.DBDuplicateEntry: + raise errors.DuplicateDocumentExists( + schema=doc['schema'], name=doc['name'], + bucket=bucket['name']) doc.safe_delete(session=session) deleted_documents.append(doc) resp.append(doc.to_dict()) @@ -219,7 +224,14 @@ def documents_create(bucket_name, documents, validations=None, with session.begin(): doc['bucket_id'] = bucket['id'] doc['revision_id'] = revision['id'] - doc.save(session=session) + + try: + doc.save(session=session) + except db_exception.DBDuplicateEntry: + raise errors.DuplicateDocumentExists( + schema=doc['schema'], name=doc['name'], + bucket=bucket['name']) + 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 @@ -266,7 +278,7 @@ def _documents_create(bucket_name, values_list, session=None): if (existing_document['bucket_name'] != bucket_name and not existing_document['schema'].startswith( types.VALIDATION_POLICY_SCHEMA)): - raise errors.DocumentExists( + raise errors.DuplicateDocumentExists( schema=existing_document['schema'], name=existing_document['name'], bucket=existing_document['bucket_name']) diff --git a/deckhand/db/sqlalchemy/models.py b/deckhand/db/sqlalchemy/models.py index 2e9441c4..0de9d7ea 100644 --- a/deckhand/db/sqlalchemy/models.py +++ b/deckhand/db/sqlalchemy/models.py @@ -133,8 +133,14 @@ def __build_tables(blob_type_obj, blob_type_list): class Document(BASE, DeckhandBase): UNIQUE_CONSTRAINTS = ('schema', 'name', 'revision_id') + __tablename__ = 'documents' + __table_args__ = ( + UniqueConstraint(*UNIQUE_CONSTRAINTS, + name='duplicate_document_constraint'), + ) + id = Column(Integer, primary_key=True) name = Column(String(64), nullable=False) schema = Column(String(64), nullable=False) @@ -163,8 +169,6 @@ def __build_tables(blob_type_obj, blob_type_list): Integer, ForeignKey('revisions.id', ondelete='CASCADE'), nullable=True) - UniqueConstraint(*UNIQUE_CONSTRAINTS) - @hybrid_property def bucket_name(self): if hasattr(self, 'bucket') and self.bucket: diff --git a/deckhand/errors.py b/deckhand/errors.py index 6c820fc4..d6abbe7a 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -317,14 +317,14 @@ class ValidationNotFound(DeckhandException): code = 404 -class DocumentExists(DeckhandException): +class DuplicateDocumentExists(DeckhandException): """A document attempted to be put into a bucket where another document with the same schema and metadata.name already exist. **Troubleshoot:** """ msg_fmt = ("Document with schema %(schema)s and metadata.name " - "%(name)s already exists in bucket %(bucket)s.") + "%(name)s already exists in bucket: %(bucket)s.") code = 409 diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 91569a0e..7ca70c64 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py @@ -662,9 +662,10 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest): {'_GLOBAL_DATA_1_': {'data': {'a': 'fail'}}}, global_abstract=False)[-1] fail_doc['schema'] = 'example/foo/v1' - fail_doc['metadata']['name'] = 'test_doc' + fail_doc['metadata']['name'] = 'fail_doc' pass_doc = copy.deepcopy(fail_doc) + pass_doc['metadata']['name'] = 'pass_doc' pass_doc['data']['a'] = 5 revision_id = self._create_revision( @@ -708,7 +709,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest): body = yaml.safe_load(resp.text) expected_errors = [{ 'error_section': {'a': 'fail'}, - 'name': 'test_doc', + 'name': 'fail_doc', 'path': '.data.a', 'schema': 'example/foo/v1', 'message': "'fail' is not of type 'integer'", diff --git a/deckhand/tests/unit/db/test_documents_negative.py b/deckhand/tests/unit/db/test_documents_negative.py index 8919c98c..714f8ee1 100644 --- a/deckhand/tests/unit/db/test_documents_negative.py +++ b/deckhand/tests/unit/db/test_documents_negative.py @@ -46,7 +46,14 @@ class TestDocumentsNegative(base.TestDbBase): self.show_document, id=-1) - def test_create_bucket_conflict(self): + def test_create_duplicate_document_same_bucket_raises_exc(self): + bucket_name = test_utils.rand_name('bucket') + document = base.DocumentFixture.get_minimal_fixture() + payload = [document, document.copy()] + self.assertRaises(errors.DuplicateDocumentExists, + self.create_documents, bucket_name, payload) + + def test_create_duplicate_document_in_another_bucket_raises_exc(self): # Create the document in one bucket. payload = base.DocumentFixture.get_minimal_fixture() bucket_name = test_utils.rand_name('bucket') @@ -55,9 +62,9 @@ class TestDocumentsNegative(base.TestDbBase): # Verify that the document cannot be created in another bucket. alt_bucket_name = test_utils.rand_name('bucket') error_re = ("Document with schema %s and metadata.name " - "%s already exists in bucket %s." % ( + "%s already exists in bucket: %s." % ( payload['schema'], payload['metadata']['name'], bucket_name)) self.assertRaisesRegex( - errors.DocumentExists, error_re, self.create_documents, + errors.DuplicateDocumentExists, error_re, self.create_documents, alt_bucket_name, payload) diff --git a/docs/source/exceptions.rst b/docs/source/exceptions.rst index 3a62e482..f4e95e4f 100644 --- a/docs/source/exceptions.rst +++ b/docs/source/exceptions.rst @@ -29,13 +29,13 @@ Deckhand Exceptions :members: :show-inheritance: :undoc-members: - * - DocumentExists - - .. autoexception:: deckhand.errors.DocumentExists + * - DocumentNotFound + - .. autoexception:: deckhand.errors.DocumentNotFound :members: :show-inheritance: :undoc-members: - * - DocumentNotFound - - .. autoexception:: deckhand.errors.DocumentNotFound + * - DuplicateDocumentExists + - .. autoexception:: deckhand.errors.DuplicateDocumentExists :members: :show-inheritance: :undoc-members: