From b03a4522cbf68c6661406216828b0548ab15850e Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 31 Oct 2018 10:24:08 -0400 Subject: [PATCH] fix: Use schema instead of metadata.schema for replacement check Recently added replacement check incorrectly uses metadata.schema and metadata.name to key on the document -- but it should be schema and metadata.name, the combination of which uniquely defines a document. Change-Id: I6cd1679ad41be38cb78d65ce2763e60f7da390d2 --- deckhand/engine/_replacement.py | 12 +++--- deckhand/errors.py | 41 +++++++++---------- .../schema-validation-success.yaml | 2 +- deckhand/tests/unit/control/test_errors.py | 4 +- .../control/test_validations_controller.py | 2 +- .../tests/unit/db/test_documents_negative.py | 2 +- deckhand/tests/unit/db/test_revisions.py | 4 +- ...ument_layering_and_replacement_negative.py | 12 +++--- 8 files changed, 38 insertions(+), 41 deletions(-) diff --git a/deckhand/engine/_replacement.py b/deckhand/engine/_replacement.py index a420aad1..d9bf3d7a 100644 --- a/deckhand/engine/_replacement.py +++ b/deckhand/engine/_replacement.py @@ -107,16 +107,14 @@ def check_replacement_is_false_uniqueness( if not document.is_control: document_identifier = ( document['metadata']['name'], - document['metadata']['schema'] + document['schema'] ) if document_identifier in non_replacement_documents: error_message = ( - 'Documents with the same name and schema existing in ' - 'different layers without any of them having ' - '`replacement = true` cannot exist as Deckhand will ' - 'arbitrarily select any of them for processing and none are ' - 'distinguishable from one another because none are a ' - 'parent or child or a replacement document.') + 'More than one document with the same name and schema was ' + 'found, but none has `replacement: true`. Ensure that only 2 ' + 'documents exist for each replacement and that one has ' + '`replacement: true` and the other `replacement: false`.') raise errors.InvalidDocumentReplacement( schema=document.schema, name=document.name, layer=document.layer, reason=error_message) diff --git a/deckhand/errors.py b/deckhand/errors.py index 54a30cf1..9d317247 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -160,7 +160,7 @@ class DeckhandException(Exception): a 'msg_fmt' property. That msg_fmt will get printf'd with the keyword arguments provided to the constructor. """ - msg_fmt = "An unknown exception occurred." + msg_fmt = "An unknown exception occurred" def __init__(self, message=None, code=500, **kwargs): kwargs.setdefault('code', code) @@ -195,7 +195,7 @@ class InvalidDocumentFormat(DeckhandException): **Troubleshoot:** """ - msg_fmt = ("The provided documents failed schema validation.") + msg_fmt = ("The provided documents failed schema validation") code = 400 @@ -210,7 +210,7 @@ class InvalidDocumentLayer(DeckhandException): msg_fmt = ("Invalid layer '%(document_layer)s' for document " "[%(document_schema)s] %(document_name)s was not found in " "layerOrder: %(layer_order)s for provided LayeringPolicy: " - "%(layering_policy_name)s.") + "%(layering_policy_name)s") code = 400 @@ -234,7 +234,7 @@ class IndeterminateDocumentParent(DeckhandException): **Troubleshoot:** """ msg_fmt = ("Too many parent documents found for document [%(schema)s, " - "%(layer)s] %(name)s. Found: %(found)s. Expected: 1.") + "%(layer)s] %(name)s. Found: %(found)s. Expected: 1") code = 400 @@ -246,7 +246,7 @@ class SubstitutionDependencyCycle(DeckhandException): * Check that there is no two-way substitution dependency between documents. """ msg_fmt = ('Cannot determine substitution order as a dependency ' - 'cycle exists for the following documents: %(cycle)s.') + 'cycle exists for the following documents: %(cycle)s') code = 400 @@ -266,8 +266,7 @@ class MissingDocumentKey(DeckhandException): msg_fmt = ("Missing action path in %(action)s needed for layering from " "either the data section of the parent [%(parent_schema)s, " "%(parent_layer)s] %(parent_name)s or child [%(child_schema)s, " - "%(child_layer)s] %(child_name)s " - "document.") + "%(child_layer)s] %(child_name)s document") code = 400 @@ -283,7 +282,7 @@ class MissingDocumentPattern(DeckhandException): msg_fmt = ("The destination document's `data` section is missing the " "pattern %(pattern)s specified under " "`substitutions.dest.pattern` at path %(jsonpath)s, specified " - "under `substitutions.dest.path`.") + "under `substitutions.dest.path`") code = 400 @@ -308,7 +307,7 @@ class UnsupportedActionMethod(DeckhandException): **Troubleshoot:** """ - msg_fmt = ("Method in %(actions)s is invalid for document %(document)s.") + msg_fmt = ("Method in %(actions)s is invalid for document %(document)s") code = 400 @@ -318,7 +317,7 @@ class RevisionTagBadFormat(DeckhandException): **Troubleshoot:** """ msg_fmt = ("The requested tag data %(data)s must either be null or " - "dictionary.") + "dictionary") code = 400 @@ -338,7 +337,7 @@ class SubstitutionSourceDataNotFound(DeckhandException): "%(src_path)s in source document [%(src_schema)s, %(src_layer)s] " "%(src_name)s which is referenced by destination document " "[%(dest_schema)s, %(dest_layer)s] %(dest_name)s under its " - "`metadata.substitutions`.") + "`metadata.substitutions`") code = 400 @@ -352,7 +351,7 @@ class EncryptionSourceNotFound(DeckhandException): msg_fmt = ( "Required encryption source reference could not be resolved into a " "secret because it was not found among encryption sources. Ref: " - "%(secret_ref)s. Referenced by: [%(schema)s, %(layer)s] %(name)s.") + "%(secret_ref)s. Referenced by: [%(schema)s, %(layer)s] %(name)s") code = 400 # Indicates bad data was passed in, causing a lookup to fail. @@ -362,7 +361,7 @@ class DocumentNotFound(DeckhandException): **Troubleshoot:** """ msg_fmt = ("The requested document using filters: %(filters)s was not " - "found.") + "found") code = 404 @@ -371,7 +370,7 @@ class RevisionNotFound(DeckhandException): **Troubleshoot:** """ - msg_fmt = "The requested revision=%(revision_id)s was not found." + msg_fmt = "The requested revision=%(revision_id)s was not found" code = 404 @@ -381,7 +380,7 @@ class RevisionTagNotFound(DeckhandException): **Troubleshoot:** """ msg_fmt = ("The requested tag '%(tag)s' for revision %(revision)s was " - "not found.") + "not found") code = 404 @@ -392,7 +391,7 @@ class ValidationNotFound(DeckhandException): """ msg_fmt = ("The requested validation entry %(entry_id)s was not found " "for validation name %(validation_name)s and revision ID " - "%(revision_id)s.") + "%(revision_id)s") code = 404 @@ -403,7 +402,7 @@ class DuplicateDocumentExists(DeckhandException): **Troubleshoot:** """ msg_fmt = ("Document [%(schema)s, %(layer)s] %(name)s already exists in " - "bucket: %(bucket)s.") + "bucket: %(bucket)s") code = 409 @@ -416,7 +415,7 @@ class SingletonDocumentConflict(DeckhandException): msg_fmt = ("A singleton document [%(schema)s, %(layer)s] %(name)s already " "exists in the system. The new document(s) %(conflict)s cannot " "be created. To create a document with a new name, delete the " - "current one first.") + "current one first") code = 409 @@ -425,7 +424,7 @@ class LayeringPolicyNotFound(DeckhandException): **Troubleshoot:** """ - msg_fmt = ("Required LayeringPolicy was not found for layering.") + msg_fmt = ("Required LayeringPolicy was not found for layering") code = 409 @@ -440,7 +439,7 @@ class SubstitutionSourceNotFound(DeckhandException): msg_fmt = ( "Required substitution source document [%(src_schema)s] %(src_name)s " "was not found, yet is referenced by [%(document_schema)s] " - "%(document_name)s.") + "%(document_name)s") code = 409 @@ -449,7 +448,7 @@ class PolicyNotAuthorized(DeckhandException): **Troubleshoot:** """ - msg_fmt = "Policy doesn't allow %(action)s to be performed." + msg_fmt = "Policy doesn't allow %(action)s to be performed" code = 403 diff --git a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml index d984a1cd..da032d0c 100644 --- a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml +++ b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml @@ -161,7 +161,7 @@ tests: $.[0].details.messageList[0].level: Error $.[0].details.messageList[0].name: D002 $.[0].kind: Status - $.[0].message: The provided documents failed schema validation. + $.[0].message: The provided documents failed schema validation $.[0].reason: Validation $.[0].status: Failure diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py index ed184072..538973a0 100644 --- a/deckhand/tests/unit/control/test_errors.py +++ b/deckhand/tests/unit/control/test_errors.py @@ -158,7 +158,7 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest): } ] }, - 'message': 'The provided documents failed schema validation.', + 'message': 'The provided documents failed schema validation', 'metadata': {} } body = yaml.safe_load(resp.text) @@ -224,7 +224,7 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest): } ] }, - 'message': 'The provided documents failed schema validation.', + 'message': 'The provided documents failed schema validation', 'metadata': {} } body = yaml.safe_load(resp.text) diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 05c94d4e..15bcb7c0 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py @@ -348,7 +348,7 @@ class TestValidationsController(BaseValidationsControllerTest): VALIDATION_FAILURE_RESULT) self.assertEqual(201, resp.status_code) expected_error = ('The requested validation entry 5 was not found for ' - 'validation name %s and revision ID %d.' % ( + 'validation name %s and revision ID %d' % ( validation_name, revision_id)) resp = self.app.simulate_get( diff --git a/deckhand/tests/unit/db/test_documents_negative.py b/deckhand/tests/unit/db/test_documents_negative.py index 1633a13b..dcd5bb9b 100644 --- a/deckhand/tests/unit/db/test_documents_negative.py +++ b/deckhand/tests/unit/db/test_documents_negative.py @@ -61,7 +61,7 @@ class TestDocumentsNegative(base.DeckhandWithDBTestCase): # Verify that the document cannot be created in another bucket. alt_bucket_name = test_utils.rand_name('bucket') - error_re = r"^Document .* already exists in bucket: %s.$" % bucket_name + error_re = r"^Document .* already exists in bucket: %s$" % bucket_name self.assertRaisesRegex( errors.DuplicateDocumentExists, error_re, self.create_documents, alt_bucket_name, payload) diff --git a/deckhand/tests/unit/db/test_revisions.py b/deckhand/tests/unit/db/test_revisions.py index 34419ba2..e26c0f9f 100644 --- a/deckhand/tests/unit/db/test_revisions.py +++ b/deckhand/tests/unit/db/test_revisions.py @@ -126,7 +126,7 @@ class TestRevisions(base.DeckhandWithDBTestCase): # Validate that all revisions were deleted. for revision_id in all_revision_ids: - error_re = (r'^The requested revision=%s was not found.$' + error_re = (r'^The requested revision=%s was not found$' % revision_id) self.assertRaisesRegex(errors.RevisionNotFound, error_re, self.show_revision, revision_id) @@ -135,7 +135,7 @@ class TestRevisions(base.DeckhandWithDBTestCase): for doc in created_documents: filters = {'id': doc['id']} error_re = (r'^The requested document using filters: %s was not ' - 'found.$' % filters) + 'found$' % filters) self.assertRaisesRegex(errors.DocumentNotFound, error_re, self.show_document, **filters) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py index 7f73a940..2515f0c0 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py @@ -139,9 +139,9 @@ class TestDocumentLayeringReplacementNegative( self._test_layering, documents) def test_replacement_true_with_parent_replacement_true_raises_exc(self): - """Validate that when documents have the same `metadata.name` and - `metadata.schema` existing in different layers without any of them - having `replacement = true` raises an exception + """Validate that when documents have the same ``metadata.name`` and + ``schema`` existing in different layers without any of them + having ``replacement = true`` raises an exception """ doc_factory = factories.DocumentFactory(2, [1, 1]) documents = doc_factory.gen_test({}) @@ -154,8 +154,8 @@ class TestDocumentLayeringReplacementNegative( document['metadata']['layeringDefinition'].pop( 'parentSelector') - error_re = (r'Documents with the same name and schema existing in ' - 'different layers without any of them having ' - '`replacement = true` cannot exist.*') + error_re = ( + r'More than one document with the same name and schema was found, ' + 'but none has `replacement: true`.*') self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, self._test_layering, documents)