From e0a629db58505a1e6ebbe2b92d8816dfb02ca475 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Thu, 1 Jun 2017 09:14:12 -0400 Subject: [PATCH] improve error formatting for jsonschema failures on Credential.inputs this provides error messages keyed by input fields, so that instead of e.g., { 'inputs': ['Invalid certificate or key: u'XYZ'] } ...you get: { 'inputs': { 'ssh_key_data': ['Invalid certificate or key: u'XYZ'] } } Includes /api/v1/ compatability for error message format. Requests to /api/v1/ will get: {'ssh_key_data': ['Invalid certificate or key: u'XYZ']} --- awx/api/serializers.py | 14 ++++- awx/main/fields.py | 51 +++++++++++-------- .../tests/functional/api/test_credential.py | 30 +++++++---- awx/main/tests/functional/test_credential.py | 8 ++- 4 files changed, 68 insertions(+), 35 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index ee0b9edb94..a5d6f9a4e1 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -2087,7 +2087,19 @@ class CredentialSerializerCreate(CredentialSerializer): attrs.pop(field) if not owner_fields: raise serializers.ValidationError({"detail": _("Missing 'user', 'team', or 'organization'.")}) - return super(CredentialSerializerCreate, self).validate(attrs) + try: + return super(CredentialSerializerCreate, self).validate(attrs) + except ValidationError as e: + # TODO: remove when API v1 is removed + # If we have an `inputs` error on `/api/v1/`: + # {'inputs': {'username': [...]}} + # ...instead, send back: + # {'username': [...]} + if self.version == 1 and isinstance(e.detail.get('inputs'), dict): + e.detail = e.detail['inputs'] + raise e + else: + raise def create(self, validated_data): user = validated_data.pop('user', None) diff --git a/awx/main/fields.py b/awx/main/fields.py index 8031d2e110..b8f8d9fbf3 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -35,6 +35,9 @@ import jsonschema.exceptions from jsonfield import JSONField as upstream_JSONField from jsonbfield.fields import JSONField as upstream_JSONBField +# DRF +from rest_framework import serializers + # AWX from awx.main.utils.filters import SmartFilter from awx.main.validators import validate_ssh_private_key @@ -410,7 +413,7 @@ def format_ssh_private_key(value): value = value.replace(r'\u003d', '=') try: validate_ssh_private_key(value) - except jsonschema.exceptions.ValidationError as e: + except django_exceptions.ValidationError as e: raise jsonschema.exceptions.FormatError(e.message) return value @@ -445,7 +448,7 @@ class CredentialInputField(JSONSchemaField): properties = {} for field in model_instance.credential_type.inputs.get('fields', []): field = field.copy() - properties[field.pop('id')] = field + properties[field['id']] = field return { 'type': 'object', 'properties': properties, @@ -471,37 +474,43 @@ class CredentialInputField(JSONSchemaField): else: decrypted_values[k] = v - super(CredentialInputField, self).validate(decrypted_values, - model_instance) + super(JSONSchemaField, self).validate(decrypted_values, model_instance) + errors = {} + for error in Draft4Validator( + self.schema(model_instance), + format_checker=self.format_checker + ).iter_errors(decrypted_values): + if error.validator == 'pattern' and 'error' in error.schema: + error.message = error.schema['error'] % error.instance + if 'id' not in error.schema: + # If the error is not for a specific field, it's specific to + # `inputs` in general + raise django_exceptions.ValidationError( + error.message, + code='invalid', + params={'value': value}, + ) + errors[error.schema['id']] = [error.message] - errors = [] inputs = model_instance.credential_type.inputs for field in inputs.get('required', []): if not value.get(field, None): - errors.append( - _('%s required for %s credential.') % ( - field, model_instance.credential_type.name - ) - ) + errors[field] = [_('required for %s') % ( + model_instance.credential_type.name + )] # `ssh_key_unlock` requirements are very specific and can't be # represented without complicated JSON schema if model_instance.credential_type.kind == 'ssh': if model_instance.has_encrypted_ssh_key_data and not value.get('ssh_key_unlock'): - errors.append( - _('SSH key unlock must be set when SSH key is encrypted.') - ) + errors['ssh_key_unlock'] = [_('must be set when SSH key is encrypted.')] if not model_instance.has_encrypted_ssh_key_data and value.get('ssh_key_unlock'): - errors.append( - _('SSH key unlock should not be set when SSH key is not encrypted.') - ) + errors['ssh_key_unlock'] = [_('should not be set when SSH key is not encrypted.')] if errors: - raise django_exceptions.ValidationError( - errors, - code='invalid', - params={'value': value}, - ) + raise serializers.ValidationError({ + 'inputs': errors + }) class CredentialTypeInputField(JSONSchemaField): diff --git a/awx/main/tests/functional/api/test_credential.py b/awx/main/tests/functional/api/test_credential.py index dddf853ac2..45096c2433 100644 --- a/awx/main/tests/functional/api/test_credential.py +++ b/awx/main/tests/functional/api/test_credential.py @@ -1,4 +1,3 @@ -import json import mock # noqa import pytest @@ -636,7 +635,7 @@ def test_inputs_cannot_contain_extra_fields(get, post, organization, admin, cred admin ) assert response.status_code == 400 - assert "'invalid_field' was unexpected" in json.dumps(response.data) + assert "'invalid_field' was unexpected" in response.data['inputs'][0] # @@ -1032,8 +1031,11 @@ def test_aws_create_fail_required_fields(post, organization, admin, version, par assert response.status_code == 400 assert Credential.objects.count() == 0 - assert 'username' in json.dumps(response.data) - assert 'password' in json.dumps(response.data) + errors = response.data + if version == 'v2': + errors = response.data['inputs'] + assert errors['username'] == ['required for %s' % aws.name] + assert errors['password'] == ['required for %s' % aws.name] # @@ -1100,9 +1102,12 @@ def test_vmware_create_fail_required_fields(post, organization, admin, version, assert response.status_code == 400 assert Credential.objects.count() == 0 - assert 'username' in json.dumps(response.data) - assert 'password' in json.dumps(response.data) - assert 'host' in json.dumps(response.data) + errors = response.data + if version == 'v2': + errors = response.data['inputs'] + assert errors['username'] == ['required for %s' % vmware.name] + assert errors['password'] == ['required for %s' % vmware.name] + assert errors['host'] == ['required for %s' % vmware.name] # @@ -1160,10 +1165,13 @@ def test_openstack_create_fail_required_fields(post, organization, admin, versio admin ) assert response.status_code == 400 - assert 'username' in json.dumps(response.data) - assert 'password' in json.dumps(response.data) - assert 'host' in json.dumps(response.data) - assert 'project' in json.dumps(response.data) + errors = response.data + if version == 'v2': + errors = response.data['inputs'] + assert errors['username'] == ['required for %s' % openstack.name] + assert errors['password'] == ['required for %s' % openstack.name] + assert errors['host'] == ['required for %s' % openstack.name] + assert errors['project'] == ['required for %s' % openstack.name] @pytest.mark.django_db diff --git a/awx/main/tests/functional/test_credential.py b/awx/main/tests/functional/test_credential.py index 95f36a98e6..4c519285ec 100644 --- a/awx/main/tests/functional/test_credential.py +++ b/awx/main/tests/functional/test_credential.py @@ -7,6 +7,8 @@ from django.core.exceptions import ValidationError from awx.main.utils.common import decrypt_field from awx.main.models import Credential, CredentialType +from rest_framework import serializers + EXAMPLE_PRIVATE_KEY = '-----BEGIN PRIVATE KEY-----\nxyz==\n-----END PRIVATE KEY-----' EXAMPLE_ENCRYPTED_PRIVATE_KEY = '-----BEGIN PRIVATE KEY-----\nProc-Type: 4,ENCRYPTED\nxyz==\n-----END PRIVATE KEY-----' @@ -86,8 +88,9 @@ def test_cred_type_input_schema_validity(input_, valid): inputs=input_ ) if valid is False: - with pytest.raises(ValidationError): + with pytest.raises(Exception) as e: type_.full_clean() + assert e.type in (ValidationError, serializers.ValidationError) else: type_.full_clean() @@ -216,8 +219,9 @@ def test_ssh_key_data_validation(credentialtype_ssh, organization, ssh_key_data, if valid: cred.full_clean() else: - with pytest.raises(ValidationError): + with pytest.raises(Exception) as e: cred.full_clean() + assert e.type in (ValidationError, serializers.ValidationError) @pytest.mark.django_db