RANGER-584 User friendly error messages for service validation error failures
Project: http://git-wip-us.apache.org/repos/asf/incubator-ranger/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-ranger/commit/8f03ed10 Tree: http://git-wip-us.apache.org/repos/asf/incubator-ranger/tree/8f03ed10 Diff: http://git-wip-us.apache.org/repos/asf/incubator-ranger/diff/8f03ed10 Branch: refs/heads/HDP-2.3.2-groupid Commit: 8f03ed10c62d0ce29c4df8e8837dd210f7b7c605 Parents: 1aa79ff Author: Alok Lal <[email protected]> Authored: Thu Jul 30 15:33:42 2015 -0700 Committer: Alok Lal <[email protected]> Committed: Wed Sep 2 13:09:42 2015 -0700 ---------------------------------------------------------------------- .../plugin/errors/ValidationErrorCode.java | 76 +++++++++ .../validation/RangerServiceValidator.java | 159 ++++++++++++------- .../plugin/errors/TestValidationErrorCode.java | 72 +++++++++ .../TestValidationFailureDetails.java | 3 - 4 files changed, 248 insertions(+), 62 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8f03ed10/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java b/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java new file mode 100644 index 0000000..77d16f5 --- /dev/null +++ b/agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.plugin.errors; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import java.text.MessageFormat; +import java.util.Arrays; + +public enum ValidationErrorCode { + + SERVICE_VALIDATION_ERR_UNSUPPORTED_ACTION(1001, "Internal error: unsupported action[{0}]; isValid(Long) is only supported for DELETE"), + SERVICE_VALIDATION_ERR_MISSING_FIELD(1002, "Internal error: missing field[{0}]"), + SERVICE_VALIDATION_ERR_NULL_SERVICE_OBJECT(1003, "Internal error: service object passed in was null"), + SERVICE_VALIDATION_ERR_EMPTY_SERVICE_ID(1004, "Internal error: service id was null/empty/blank"), + SERVICE_VALIDATION_ERR_INVALID_SERVICE_ID(1005, "No service found for id [{0}]"), + SERVICE_VALIDATION_ERR_INVALID_SERVICE_NAME(1006, "Service name[{0}] was null/empty/blank"), + SERVICE_VALIDATION_ERR_SERVICE_NAME_CONFICT(1007, "service with the name[{0}] already exists"), + SERVICE_VALIDATION_ERR_ID_NAME_CONFLICT(1008, "id/name conflict: another service already exists with name[{0}], its id is [{1}]"), + SERVICE_VALIDATION_ERR_MISSING_SERVICE_DEF(1009, "service def [{0}] was null/empty/blank"), + SERVICE_VALIDATION_ERR_INVALID_SERVICE_DEF(1010, "service def named[{0}] not found"), + SERVICE_VALIDATION_ERR_REQUIRED_PARM_MISSING(1011, "required configuration parameter is missing; missing parameters: {0}"), + ; + + + private static final Log LOG = LogFactory.getLog(ValidationErrorCode.class); + + final int _errorCode; + final String _template; + + ValidationErrorCode(int errorCode, String template) { + _errorCode = errorCode; + _template = template; + } + + public String getMessage(Object... items) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("<== ValidationErrorCode.getMessage(%s)", Arrays.toString(items))); + } + + MessageFormat mf = new MessageFormat(_template); + String result = mf.format(items); + + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("<== ValidationErrorCode.getMessage(%s): %s", Arrays.toString(items), result)); + } + return result; + } + + public int getErrorCode() { + return _errorCode; + } + + @Override + public String toString() { + return String.format("Code: %d, template: %s", _errorCode, _template); + } +} http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8f03ed10/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java index 615e385..3cfaa3e 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceValidator.java @@ -26,6 +26,7 @@ import java.util.Set; import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.ranger.plugin.errors.ValidationErrorCode; import org.apache.ranger.plugin.model.RangerService; import org.apache.ranger.plugin.model.RangerServiceDef; import org.apache.ranger.plugin.store.ServiceStore; @@ -67,16 +68,21 @@ public class RangerServiceValidator extends RangerValidator { boolean valid = true; if (action != Action.DELETE) { - failures.add(new ValidationFailureDetailsBuilder() - .isAnInternalError() - .becauseOf("unsupported action[" + action + "]; isValid(Long) is only supported for DELETE") - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_UNSUPPORTED_ACTION; + failures.add(new RangerServiceValidationErrorBuilder() + .isAnInternalError() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(action)) + .build()); valid = false; } else if (id == null) { - failures.add(new ValidationFailureDetailsBuilder() - .field("id") - .isMissing() - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_MISSING_FIELD; + failures.add(new RangerServiceValidationErrorBuilder() + .field("id") + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(id)) + .build()); valid = false; } else if (getService(id) == null) { if (LOG.isDebugEnabled()) { @@ -100,32 +106,34 @@ public class RangerServiceValidator extends RangerValidator { boolean valid = true; if (service == null) { - String message = "service object passed in was null"; - LOG.debug(message); - failures.add(new ValidationFailureDetailsBuilder() - .field("service") - .isMissing() - .becauseOf(message) - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_NULL_SERVICE_OBJECT; + failures.add(new RangerServiceValidationErrorBuilder() + .field("service") + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage()) + .build()); valid = false; } else { Long id = service.getId(); if (action == Action.UPDATE) { // id is ignored for CREATE if (id == null) { - String message = "service id was null/empty/blank"; - LOG.debug(message); - failures.add(new ValidationFailureDetailsBuilder() - .field("id") - .isMissing() - .becauseOf(message) - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_EMPTY_SERVICE_ID; + failures.add(new RangerServiceValidationErrorBuilder() + .field("id") + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage()) + .build()); valid = false; } else if (getService(id) == null) { - failures.add(new ValidationFailureDetailsBuilder() - .field("id") - .isSemanticallyIncorrect() - .becauseOf("no service exists with id[" + id +"]") - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_INVALID_SERVICE_ID; + failures.add(new RangerServiceValidationErrorBuilder() + .field("id") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(id)) + .build()); valid = false; } } @@ -133,48 +141,56 @@ public class RangerServiceValidator extends RangerValidator { boolean nameSpecified = StringUtils.isNotBlank(name); RangerServiceDef serviceDef = null; if (!nameSpecified) { - String message = "service name[" + name + "] was null/empty/blank"; - LOG.debug(message); - failures.add(new ValidationFailureDetailsBuilder() - .field("name") - .isMissing() - .becauseOf(message) - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_INVALID_SERVICE_NAME; + failures.add(new RangerServiceValidationErrorBuilder() + .field("name") + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(name)) + .build()); valid = false; } else { RangerService otherService = getService(name); if (otherService != null && action == Action.CREATE) { - failures.add(new ValidationFailureDetailsBuilder() - .field("name") - .isSemanticallyIncorrect() - .becauseOf("service with the name[" + name + "] already exists") - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_SERVICE_NAME_CONFICT; + failures.add(new RangerServiceValidationErrorBuilder() + .field("name") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(name)) + .build()); valid = false; } else if (otherService != null && otherService.getId() !=null && !otherService.getId().equals(id)) { - failures.add(new ValidationFailureDetailsBuilder() - .field("id/name") - .isSemanticallyIncorrect() - .becauseOf("id/name conflict: another service already exists with name[" + name + "], its id is [" + otherService.getId() + "]") - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_ID_NAME_CONFLICT; + failures.add(new RangerServiceValidationErrorBuilder() + .field("id/name") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(name, otherService.getId())) + .build()); valid = false; } } String type = service.getType(); boolean typeSpecified = StringUtils.isNotBlank(type); if (!typeSpecified) { - failures.add(new ValidationFailureDetailsBuilder() - .field("type") - .isMissing() - .becauseOf("service def [" + type + "] was null/empty/blank") - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_MISSING_SERVICE_DEF; + failures.add(new RangerServiceValidationErrorBuilder() + .field("type") + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(type)) + .build()); valid = false; } else { serviceDef = getServiceDef(type); if (serviceDef == null) { - failures.add(new ValidationFailureDetailsBuilder() - .field("type") - .isSemanticallyIncorrect() - .becauseOf("service def named[" + type + "] not found") + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_INVALID_SERVICE_DEF; + failures.add(new RangerServiceValidationErrorBuilder() + .field("type") + .isSemanticallyIncorrect() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(type)) .build()); valid = false; } @@ -185,12 +201,14 @@ public class RangerServiceValidator extends RangerValidator { Set<String> inputParameters = getServiceConfigParameters(service); Set<String> missingParameters = Sets.difference(reqiredParameters, inputParameters); if (!missingParameters.isEmpty()) { - failures.add(new ValidationFailureDetailsBuilder() - .field("configuration") - .subField(missingParameters.iterator().next()) // we return any one parameter! - .isMissing() - .becauseOf("required configuration parameter is missing; missing parameters: " + missingParameters) - .build()); + ValidationErrorCode error = ValidationErrorCode.SERVICE_VALIDATION_ERR_REQUIRED_PARM_MISSING; + failures.add(new RangerServiceValidationErrorBuilder() + .field("configuration") + .subField(missingParameters.iterator().next()) // we return any one parameter! + .isMissing() + .errorCode(error.getErrorCode()) + .becauseOf(error.getMessage(missingParameters)) + .build()); valid = false; } } @@ -201,4 +219,27 @@ public class RangerServiceValidator extends RangerValidator { } return valid; } + + static class RangerServiceValidationErrorBuilder extends ValidationFailureDetailsBuilder { + + @Override + ValidationFailureDetails build() { + return new RangerPolicyValidationFailure(_errorCode, _fieldName, _subFieldName, _missing, _semanticError, _internalError, _reason); + } + } + + static class RangerPolicyValidationFailure extends ValidationFailureDetails { + + public RangerPolicyValidationFailure(int errorCode, String fieldName, String subFieldName, boolean missing, boolean semanticError, boolean internalError, String reason) { + super(errorCode, fieldName, subFieldName, missing, semanticError, internalError, reason); + } + + // TODO remove and move to baseclass when all 3 move to new message framework + @Override + public String toString() { + LOG.debug("RangerServiceValidationFailure.toString"); + return String.format("%s: %d, %s", "Policy validation failure", _errorCode, _reason); + } + } + } http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8f03ed10/agents-common/src/test/java/org/apache/ranger/plugin/errors/TestValidationErrorCode.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/errors/TestValidationErrorCode.java b/agents-common/src/test/java/org/apache/ranger/plugin/errors/TestValidationErrorCode.java new file mode 100644 index 0000000..d6b2d16 --- /dev/null +++ b/agents-common/src/test/java/org/apache/ranger/plugin/errors/TestValidationErrorCode.java @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.plugin.errors; + +import com.google.common.collect.ImmutableSet; +import junit.framework.TestCase; + +import java.util.HashSet; +import java.util.Set; + +/** + * Created by alal on 7/30/15. + */ +public class TestValidationErrorCode extends TestCase { + + + public void testUserMessage() throws Exception { + ValidationErrorCode errorCode = ValidationErrorCode.SERVICE_VALIDATION_ERR_UNSUPPORTED_ACTION; + String aParameter = "FOO"; + String expected = errorCode._template.replace("{0}", aParameter); + assertEquals(expected, errorCode.getMessage(aParameter)); + } + + /** + * tests if template has any trivial template variable problems, e.g. if template has {3} in it then it + * better also have {0}, {1} and {2} in it else MessageFormat output might be unexpected. + * + * This check is far from perfect. It may give false alarms if the message itself contains strings of the form {1} + * which have been escaped using single quotes. If that happens we would have to make this test smarter. + */ + public void testTemplates() { + + // we check up to 5 substitution variables. If there are more than 5 then you probably have a different set of problems + Set<ValidationErrorCode> may = ImmutableSet.copyOf(ValidationErrorCode.values()); + + // set of enums that must not hvae any subsequent placeholders in it + Set<ValidationErrorCode> mustNot = new HashSet<ValidationErrorCode>(); + + for (int i = 0; i < 5; i++) { + String token = String.format("{%d", i); + // check which ones should not have anymore substition varabile placehoders in them, {0}, {1}, etc. + for (ValidationErrorCode anEnum : may) { + if (!anEnum._template.contains(token)) { + // if template does not have {1} then it surely must not have {2}, {3}, etc. + mustNot.add(anEnum); + } + } + // check for incorrectly numbers substition variable placeholders + for (ValidationErrorCode anEnum : mustNot) { + assertFalse(anEnum.toString() + ": contains " + token + ". Check for wongly numberd substition variable placeholders.", + anEnum._template.contains(token)); + } + } + } +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8f03ed10/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestValidationFailureDetails.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestValidationFailureDetails.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestValidationFailureDetails.java index cf929c6..815d41c 100644 --- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestValidationFailureDetails.java +++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestValidationFailureDetails.java @@ -27,9 +27,6 @@ import java.util.regex.Pattern; import static org.junit.Assert.assertEquals; -/** - * Created by alal on 6/17/15. - */ public class TestValidationFailureDetails { @Test
