This is an automated email from the ASF dual-hosted git repository. reschke pushed a commit to branch 1.22 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit b634dcacefb4eb807d9341642698435b818fa61d Author: mbaedke <manfred.bae...@gmail.com> AuthorDate: Tue Jan 23 13:51:46 2024 +0100 OAK-10616: Make error messages from o.a.j.o.namepath.JcrNameParser/Jc… (#1277) * OAK-10616: Make error messages from o.a.j.o.namepath.JcrNameParser/JcrPathParser consistent and less misleading Unified error messages from JcrPathParser and JcrNameParser. --- .../jackrabbit/oak/namepath/JcrNameParser.java | 14 ++-- .../jackrabbit/oak/namepath/JcrPathParser.java | 85 ++++++++++++++-------- .../jackrabbit/oak/namepath/PathParserTest.java | 38 ++++------ 3 files changed, 77 insertions(+), 60 deletions(-) diff --git a/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrNameParser.java b/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrNameParser.java index d0a6471d43..8322e5926a 100644 --- a/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrNameParser.java +++ b/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrNameParser.java @@ -80,7 +80,7 @@ public final class JcrNameParser { // trivial check int len = jcrName == null ? 0 : jcrName.length(); if (len == 0) { - listener.error("Empty name"); + listener.error("Empty name."); return false; } if (".".equals(jcrName) || "..".equals(jcrName)) { @@ -97,7 +97,7 @@ public final class JcrNameParser { char c = jcrName.charAt(i); if (c == ':') { if (state == STATE_PREFIX_START) { - listener.error("Prefix must not be empty"); + listener.error("Prefix must not be empty."); return false; } else if (state == STATE_PREFIX) { prefix = jcrName.substring(0, i); @@ -109,17 +109,17 @@ public final class JcrNameParser { } else if (state == STATE_URI) { // ignore -> validation of uri later on. } else { - listener.error("'" + c + "' not allowed in name"); + listener.error("'" + c + "' not allowed in name."); return false; } } else if (c == '[' || c == ']' || c == '*' || c == '|') { - listener.error("'" + c + "' not allowed in name"); + listener.error("'" + c + "' not allowed in name."); return false; } else if (c == '/') { if (state == STATE_URI_START) { state = STATE_URI; } else if (state != STATE_URI) { - listener.error("'" + c + "' not allowed in name"); + listener.error("'" + c + "' not allowed in name."); return false; } } else if (c == '{') { @@ -179,12 +179,12 @@ public final class JcrNameParser { // take care of qualified jcrNames starting with '{' that are not having // a terminating '}' -> make sure there are no illegal characters present. if (state == STATE_URI && (jcrName.indexOf(':') > -1 || jcrName.indexOf('/') > -1)) { - listener.error("Local name may not contain ':' nor '/'"); + listener.error("Local name may not contain ':' nor '/'."); return false; } if (nameStart == len || state == STATE_NAME_START) { - listener.error("Local name must not be empty"); + listener.error("Local name must not be empty."); return false; } diff --git a/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java b/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java index 553126b00c..614c6d7a1d 100644 --- a/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java +++ b/oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java @@ -44,6 +44,42 @@ public final class JcrPathParser { boolean parent(); } + private static final class PathAwareListener implements Listener { + + private final Listener listener; + private final String jcrPath; + + private PathAwareListener(Listener listener, String jcrPath) { + this.listener = listener; + this.jcrPath = jcrPath; + } + + @Override + public void error(String message) { + listener.error("'" + jcrPath + "' is not a valid path. " + message); + } + + @Override + public boolean name(String name, int index) { + return listener.name(name, index); + } + + @Override + public boolean root() { + return listener.root(); + } + + @Override + public boolean current() { + return listener.current(); + } + + @Override + public boolean parent() { + return listener.parent(); + } + } + public static boolean parse(String jcrPath, Listener listener) { // check for length int len = jcrPath == null ? 0 : jcrPath.length(); @@ -77,6 +113,8 @@ public final class JcrPathParser { int index = 0; boolean wasSlash = false; + final PathAwareListener pathAwareListener = new PathAwareListener(listener, jcrPath); + while (pos <= len) { char c = pos == len ? EOF : jcrPath.charAt(pos); pos++; @@ -85,8 +123,7 @@ public final class JcrPathParser { case '/': case EOF: if (state == STATE_PREFIX_START && c != EOF) { - listener.error('\'' + jcrPath + "' is not a valid path. " + - "double slash '//' not allowed."); + pathAwareListener.error("Double slash '//' not allowed."); return false; } if (state == STATE_PREFIX @@ -97,14 +134,13 @@ public final class JcrPathParser { // eof path element if (name == null) { if (wasSlash) { - listener.error('\'' + jcrPath + "' is not a valid path: " + - "Trailing slashes not allowed in prefixes and names."); + pathAwareListener.error("Trailing slashes not allowed in prefixes and names."); return false; } name = jcrPath.substring(lastPos, pos - 1); } - if (!JcrNameParser.parse(name, listener, index)) { + if (!JcrNameParser.parse(name, pathAwareListener, index)) { return false; } state = STATE_PREFIX_START; @@ -112,25 +148,24 @@ public final class JcrPathParser { name = null; index = 0; } else if (state == STATE_DOT) { - if (!listener.current()) { + if (!pathAwareListener.current()) { return false; } lastPos = pos; state = STATE_PREFIX_START; } else if (state == STATE_DOTDOT) { - if (!listener.parent()) { + if (!pathAwareListener.parent()) { return false; } lastPos = pos; state = STATE_PREFIX_START; } else if (state != STATE_URI && !(state == STATE_PREFIX_START && c == EOF)) { // ignore trailing slash - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not a valid name character."); + pathAwareListener.error("'" + c + "' not allowed in name."); return false; } else if (state == STATE_URI && c == EOF) { // we reached EOF without finding the closing curly brace '}' - listener.error('\'' + jcrPath + "' is not a valid path. Missing '}'."); + pathAwareListener.error("Missing '}'."); return false; } break; @@ -143,28 +178,24 @@ public final class JcrPathParser { } else if (state == STATE_DOTDOT) { state = STATE_PREFIX; } else if (state == STATE_INDEX_END) { - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not valid after index. '/' expected."); + pathAwareListener.error("'" + c + "' not valid after index. '/' expected."); return false; } break; case ':': if (state == STATE_PREFIX_START) { - listener.error('\'' + jcrPath + "' is not a valid path. Prefix " + - "must not be empty"); + pathAwareListener.error("Prefix must not be empty."); return false; } else if (state == STATE_PREFIX) { if (wasSlash) { - listener.error('\'' + jcrPath + "' is not a valid path: " + - "Trailing slashes not allowed in prefixes and names."); + pathAwareListener.error("Trailing slashes not allowed in prefixes and names."); return false; } state = STATE_NAME_START; // don't reset the lastPos/pos since prefix+name are passed together to the NameResolver } else if (state != STATE_URI) { - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not valid name character"); + pathAwareListener.error("'" + c + "' not allowed in name."); return false; } break; @@ -172,8 +203,7 @@ public final class JcrPathParser { case '[': if (state == STATE_PREFIX || state == STATE_NAME) { if (wasSlash) { - listener.error('\'' + jcrPath + "' is not a valid path: " + - "Trailing slashes not allowed in prefixes and names."); + pathAwareListener.error("Trailing slashes not allowed in prefixes and names."); return false; } state = STATE_INDEX; @@ -187,28 +217,24 @@ public final class JcrPathParser { try { index = Integer.parseInt(jcrPath.substring(lastPos, pos - 1)); } catch (NumberFormatException e) { - listener.error('\'' + jcrPath + "' is not a valid path. " + - "NumberFormatException in index: " + + pathAwareListener.error("NumberFormatException in index: " + jcrPath.substring(lastPos, pos - 1)); return false; } if (index < 0) { - listener.error('\'' + jcrPath + "' is not a valid path. " + - "Index number invalid: " + index); + pathAwareListener.error("Index number invalid: " + index); return false; } state = STATE_INDEX_END; } else { - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not a valid name character."); + pathAwareListener.error("'" + c + "' not allowed in name."); return false; } break; case '*': case '|': - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not a valid name character."); + pathAwareListener.error("'" + c + "' not allowed in name."); return false; case '{': if (state == STATE_PREFIX_START && lastPos == pos-1) { @@ -234,8 +260,7 @@ public final class JcrPathParser { } else if (state == STATE_NAME_START) { state = STATE_NAME; } else if (state == STATE_INDEX_END) { - listener.error('\'' + jcrPath + "' is not a valid path. '" + c + - "' not valid after index. '/' expected."); + pathAwareListener.error("'" + c + "' not valid after index. '/' expected."); return false; } } diff --git a/oak-core-spi/src/test/java/org/apache/jackrabbit/oak/namepath/PathParserTest.java b/oak-core-spi/src/test/java/org/apache/jackrabbit/oak/namepath/PathParserTest.java index 46fcdaea5e..edf24e47ba 100755 --- a/oak-core-spi/src/test/java/org/apache/jackrabbit/oak/namepath/PathParserTest.java +++ b/oak-core-spi/src/test/java/org/apache/jackrabbit/oak/namepath/PathParserTest.java @@ -95,9 +95,6 @@ public class PathParserTest { private static final ParserCallbackResult CALLBACKRESULT_ROOT = new ParserCallbackResult(ParserCallbackResultType.CALLBACK_ROOT, null, 0); private static final ParserCallbackResult CALLBACKRESULT_CURRENT = new ParserCallbackResult(ParserCallbackResultType.CALLBACK_CURRENT, null, 0); private static final ParserCallbackResult CALLBACKRESULT_PARENT = new ParserCallbackResult(ParserCallbackResultType.CALLBACK_PARENT, null, 0); - private static final ParserCallbackResult CALLBACKRESULT_ERROR_ANY = new ParserCallbackResult(ParserCallbackResultType.CALLBACK_ERROR, null, 0); - private static final ParserCallbackResult CALLBACKRESULT_NAME_ANY = new ParserCallbackResult(ParserCallbackResultType.CALLBACK_NAME, null, 0); - private static ParserCallbackResult CALLBACKRESULT_NAME(String name) { return new ParserCallbackResult(ParserCallbackResultType.CALLBACK_NAME, name, 0); } @@ -167,7 +164,7 @@ public class PathParserTest { CALLBACKRESULT_CURRENT, CALLBACKRESULT_PARENT, CALLBACKRESULT_NAME("c", 1), - CALLBACKRESULT_ERROR("'" + path + "' is not a valid path. ']' not a valid name character.") + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); @@ -195,7 +192,7 @@ public class PathParserTest { public void testUnexpectedOpeningSquareBracket() throws RepositoryException { String path = "["; TestListener listener = new TestListener( - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -204,7 +201,7 @@ public class PathParserTest { path = "/["; listener = new TestListener( CALLBACKRESULT_ROOT, - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -213,7 +210,7 @@ public class PathParserTest { path = "./["; listener = new TestListener( CALLBACKRESULT_CURRENT, - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -222,7 +219,7 @@ public class PathParserTest { path = "../["; listener = new TestListener( CALLBACKRESULT_PARENT, - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -230,7 +227,7 @@ public class PathParserTest { path = ".["; listener = new TestListener( - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -238,7 +235,7 @@ public class PathParserTest { path = "..["; listener = new TestListener( - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -246,7 +243,7 @@ public class PathParserTest { path = "{[}"; listener = new TestListener( - CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName('[')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path,'[')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -280,7 +277,7 @@ public class PathParserTest { public void testUnxepectedClosingSquareBracket() throws RepositoryException { String path = "]"; TestListener listener = new TestListener( - CALLBACKRESULT_ERROR(errorInvalidCharacterInName(path, ']')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -289,7 +286,7 @@ public class PathParserTest { path = "/]"; listener = new TestListener( CALLBACKRESULT_ROOT, - CALLBACKRESULT_ERROR(errorInvalidCharacterInName(path, ']')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -298,7 +295,7 @@ public class PathParserTest { path = ".]"; listener = new TestListener( //TODO improve error message? - CALLBACKRESULT_ERROR(errorInvalidCharacterInName(path, ']')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -307,7 +304,7 @@ public class PathParserTest { path = "..]"; listener = new TestListener( //TODO improve error message? - CALLBACKRESULT_ERROR(errorInvalidCharacterInName(path, ']')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -315,7 +312,7 @@ public class PathParserTest { path = "{]}"; listener = new TestListener( - CALLBACKRESULT_ERROR(errorInvalidCharacterInName(path, ']')) + CALLBACKRESULT_ERROR(errorCharacterNotAllowedInName(path, ']')) ); assertFalse(JcrPathParser.validate(path)); assertFalse(JcrPathParser.parse(path, listener)); @@ -353,13 +350,8 @@ public class PathParserTest { listener.evaluate(); } - //TODO replace with #errorInvalidCharacterInName() to make error messages consistent? - private static String errorCharacterNotAllowedInName(char c) { - return "'" + c + "' not allowed in name"; - } - - private static String errorInvalidCharacterInName(String path, char c) { - return "'" + path + "' is not a valid path. ']' not a valid name character."; + private static String errorCharacterNotAllowedInName(String path, char c) { + return "'" + path + "' is not a valid path. '" + c + "' not allowed in name."; } private static String errorClosingQuareBracketExpected(String path) {