From 6ea4579d69173b73f6abce589bce77da25a93775 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Mon, 19 May 2014 10:31:49 +0200 Subject: [PATCH] SHL-136: Report syntax error location to the user * Fix tests --- .../shell/core/SimpleParser.java | 22 ++++++++- .../springframework/shell/core/Tokenizer.java | 34 +++++++++----- .../shell/core/TokenizingException.java | 47 +++++++++++++++++++ .../shell/core/TokenizerTests.java | 6 +-- 4 files changed, 94 insertions(+), 15 deletions(-) create mode 100644 src/main/java/org/springframework/shell/core/TokenizingException.java diff --git a/src/main/java/org/springframework/shell/core/SimpleParser.java b/src/main/java/org/springframework/shell/core/SimpleParser.java index c7905944..9be2c648 100644 --- a/src/main/java/org/springframework/shell/core/SimpleParser.java +++ b/src/main/java/org/springframework/shell/core/SimpleParser.java @@ -160,6 +160,11 @@ public class SimpleParser implements Parser { try { options = new Tokenizer(methodTarget.getRemainingBuffer()).getTokens(); } + catch (TokenizingException te) { + String commandKey = methodTarget.getKey(); + reportTokenizingException(commandKey, te); + return null; + } catch (IllegalArgumentException e) { LOGGER.warning(ExceptionUtils.extractRootCause(e).getMessage()); return null; @@ -310,6 +315,16 @@ public class SimpleParser implements Parser { } } + private void reportTokenizingException(String commandKey, TokenizingException te) { + StringBuilder caret = new StringBuilder(); + for (int i = 0; i < te.getOffendingOffset() + commandKey.length() + 1; i++) { + caret.append(" "); + } + LOGGER.warning(commandKey + " " + te.getBuffer()); + LOGGER.warning(caret + "^"); + LOGGER.warning(te.getReason()); + } + /** * @param cliOptions * @param options @@ -597,8 +612,13 @@ public class SimpleParser implements Parser { try { tokenizer = new Tokenizer(methodTarget.getRemainingBuffer(), true); } + catch (TokenizingException e) { + // Make sure we don't crash the main shell loop just + // because the user specified some option twice + return -1; + } catch (IllegalArgumentException e) { - // Make sure we don't crash the mail shell loop just + // Make sure we don't crash the main shell loop just // because the user specified some option twice return -1; } diff --git a/src/main/java/org/springframework/shell/core/Tokenizer.java b/src/main/java/org/springframework/shell/core/Tokenizer.java index f9e77f90..d815ea35 100644 --- a/src/main/java/org/springframework/shell/core/Tokenizer.java +++ b/src/main/java/org/springframework/shell/core/Tokenizer.java @@ -113,26 +113,35 @@ public class Tokenizer { eatKeyEqualsValue(); } else { + int offsetInCaseOfFailure = pos; String value = eatValue(true); - store("", value); + store("", value, offsetInCaseOfFailure); } } - private void store(String key, String value) { + /** + * Store a command key/value pair, reporting a failure if a mapping with the same key is already present. + * @param key the command key + * @param value the command value + * @param failureOffset the buffer offset at which the mapping we're trying to store was tokenized + */ + private void store(String key, String value, int failureOffset) { String alreadyThere = result.put(key, value); if (alreadyThere != null) { if ("".equals(key)) { - throw new IllegalArgumentException(String.format( + String explanation = String.format( "You cannot specify '%s' as another value for the default ('') option in a single command.%n" + "You already provided '%s' earlier.%n" + "Did you forget to add quotes around the value of another option?", value, - alreadyThere)); + alreadyThere); + throw new TokenizingException(failureOffset, buffer, explanation); } else { - throw new IllegalArgumentException(String.format( + String explanation = String.format( "You cannot specify '%s' as another value for the '--%s' option in a single command.%n" - + "You already provided '%s' earlier.", value, key, alreadyThere)); + + "You already provided '%s' earlier.", value, key, alreadyThere); + throw new TokenizingException(failureOffset, buffer, explanation); } } } @@ -189,7 +198,7 @@ public class Tokenizer { return sb.toString(); } else { - throw new IllegalArgumentException("Cannot have an unbalanced number of quotation marks"); + throw new TokenizingException(pos, buffer, "Cannot have an unbalanced number of quotation marks"); } } // Eat our delim @@ -205,7 +214,7 @@ public class Tokenizer { private char processCharacterEscapeCodes(char endDelimiter) { pos++; if (pos >= buffer.length) { - throw new IllegalArgumentException("Ran out of input in escape sequence"); + throw new TokenizingException(buffer.length, buffer, "Ran out of input in escape sequence"); } switch (buffer[pos]) { case ESCAPE_CHAR: @@ -225,7 +234,7 @@ public class Tokenizer { return '\f'; case 'u': if (pos + 5 > buffer.length) { - throw new IllegalArgumentException("Ran out input in unicode escape sequence"); + throw new TokenizingException(buffer.length, buffer, "Ran out of input in unicode escape sequence"); } String hex = new String(buffer, pos + 1, 4); try { @@ -234,7 +243,8 @@ public class Tokenizer { return code; } catch (NumberFormatException e) { - throw new IllegalArgumentException("Illegal unicode escape sequence: " + ESCAPE_CHAR + "u" + hex); + throw new TokenizingException(pos - 1, buffer, "Illegal unicode escape sequence: " + ESCAPE_CHAR + "u" + + hex); } default: @@ -303,6 +313,8 @@ public class Tokenizer { * in which case allow for just {@code --key}, using "" for the value. */ private void eatKeyEqualsValue() { + // We already consumed '--' + int offsetInCaseOfFailure = pos - 2; String key = eatKey(); eatWhiteSpace(); String value; @@ -320,7 +332,7 @@ public class Tokenizer { if (key.equals("") && value.equals("")) { return; } - store(key, value); + store(key, value, offsetInCaseOfFailure); } private String eatKey() { diff --git a/src/main/java/org/springframework/shell/core/TokenizingException.java b/src/main/java/org/springframework/shell/core/TokenizingException.java new file mode 100644 index 00000000..114dd1a9 --- /dev/null +++ b/src/main/java/org/springframework/shell/core/TokenizingException.java @@ -0,0 +1,47 @@ +/* + * Copyright 2014 the original author or authors. + * + * Licensed 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.springframework.shell.core; + +@SuppressWarnings("serial") +public class TokenizingException extends RuntimeException { + + private final int offendingOffset; + + private final char[] buffer; + + private final String reason; + + public TokenizingException(int offendingOffset, char[] buffer, String reason) { + super(); + this.offendingOffset = offendingOffset; + this.buffer = buffer; + this.reason = reason; + } + + public int getOffendingOffset() { + return offendingOffset; + } + + public String getBuffer() { + return new String(buffer); + } + + public String getReason() { + return reason; + } + +} diff --git a/src/test/java/org/springframework/shell/core/TokenizerTests.java b/src/test/java/org/springframework/shell/core/TokenizerTests.java index 84b4c46e..7c493138 100644 --- a/src/test/java/org/springframework/shell/core/TokenizerTests.java +++ b/src/test/java/org/springframework/shell/core/TokenizerTests.java @@ -79,7 +79,7 @@ public class TokenizerTests { assertEquals(expected, result); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = TokenizingException.class) public void testTwoOptionsSameKey() { tokenize("--foo bar --foo buzz"); } @@ -97,7 +97,7 @@ public class TokenizerTests { assertThat(result.get("woot"), equalTo("cool")); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = TokenizingException.class) public void testDisallowTwoOptionsSameEmptyKeyIfNotNextToEachOther() { tokenize("bar --foo wizz buzz"); } @@ -118,7 +118,7 @@ public class TokenizerTests { assertEquals(expected, result); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = TokenizingException.class) public void testValueQuotationUnbalanced() { Map result = tokenize("--foo \"bar fizz"); assertEquals(singletonMap("foo", "bar fizz"), result);