Skip to content

Commit 19fb139

Browse files
ClaudenwClaudenw
authored
CLI-322: Add '-' as an option char and implemented extensive tests (apache#217)
* Added '-' as an option char and implemented extensive tests * fixed checkstyle issue * renamed 'ary' parameter, added valid character documentation * renamed 'ary' parameter, added valid character documentation * fixed issues with extra spaces
1 parent 2bf6522 commit 19fb139

3 files changed

Lines changed: 260 additions & 18 deletions

File tree

‎pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,11 @@
187187
<artifactId>junit-jupiter-api</artifactId>
188188
<scope>test</scope>
189189
</dependency>
190+
<dependency>
191+
<groupId>org.junit.jupiter</groupId>
192+
<artifactId>junit-jupiter-params</artifactId>
193+
<scope>test</scope>
194+
</dependency>
190195
<dependency>
191196
<groupId>org.junit.vintage</groupId>
192197
<artifactId>junit-vintage-engine</artifactId>

‎src/main/java/org/apache/commons/cli/OptionValidator.java‎

Lines changed: 72 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,67 @@ Licensed to the Apache Software Foundation (ASF) under one or more
2323
* @since 1.1
2424
*/
2525
final class OptionValidator {
26+
/* package private for testing access */
27+
/** The array of additional characters allowed as the first character in the option but not in the rest of the option */
28+
static final char[] ADDITIONAL_OPTION_CHARS = {'?', '@'};
29+
/** The array of additional characters allowed in the rest of the option but not in the first position */
30+
static final char[] ADDITIONAL_LONG_CHARS = {'-'};
31+
32+
/**
33+
* Checks the char array for a matching char.
34+
* @param chars the char array to search
35+
* @param c the char to look for.
36+
* @return {@code true} if {@code c} was in {@code ary}, {@code false} otherwise.
37+
*/
38+
private static boolean search(final char[] chars, final char c) {
39+
for (char a : chars) {
40+
if (a == c) {
41+
return true;
42+
}
43+
}
44+
return false;
45+
}
46+
2647
/**
2748
* Returns whether the specified character is a valid character.
28-
*
49+
* A character is valid if any of the following conditions are true:
50+
* <ul>
51+
* <li>it is a letter</li>
52+
* <li>it is a currency symbol (such as '$')</li>
53+
* <li>it is a connecting punctuation character (such as '_')</li>
54+
* <li>it is a digit</li>
55+
* <li>it is a numeric letter (such as a Roman numeral character)</li>
56+
* <li>it is a combining mark</li>
57+
* <li>it is a non-spacing mark</li>
58+
* <li>isIdentifierIgnorable returns true for the character</li>
59+
* <li>it is a hyphen/dash ('-')</li>
60+
* </ul>
2961
* @param c the character to validate
30-
* @return true if {@code c} is a letter.
62+
* @return true if {@code c} is a valid character letter.
3163
*/
3264
private static boolean isValidChar(final char c) {
33-
return Character.isJavaIdentifierPart(c);
65+
return Character.isJavaIdentifierPart(c) || search(ADDITIONAL_LONG_CHARS, c);
3466
}
3567

3668
/**
3769
* Returns whether the specified character is a valid Option.
38-
*
70+
* A character is valid if any of the following conditions are true:
71+
* <ul>
72+
* <li>it is a letter</li>
73+
* <li>it is a currency symbol (such as '$')</li>
74+
* <li>it is a connecting punctuation character (such as '_')</li>
75+
* <li>it is a digit</li>
76+
* <li>it is a numeric letter (such as a Roman numeral character)</li>
77+
* <li>it is a combining mark</li>
78+
* <li>it is a non-spacing mark</li>
79+
* <li>isIdentifierIgnorable returns true for the character</li>
80+
* <li>it is a question mark or 'at' sign ('?' or '@')</li>
81+
* </ul>
3982
* @param c the option to validate
4083
* @return true if {@code c} is a letter, '?' or '@', otherwise false.
4184
*/
4285
private static boolean isValidOpt(final char c) {
43-
return isValidChar(c) || c == '?' || c == '@';
86+
return Character.isJavaIdentifierPart(c) || search(ADDITIONAL_OPTION_CHARS, c);
4487
}
4588

4689
/**
@@ -49,9 +92,22 @@ private static boolean isValidOpt(final char c) {
4992
*
5093
* <ul>
5194
* <li>a single character {@code opt} that is either ' '(special case), '?', '@' or a letter</li>
52-
* <li>a multi character {@code opt} that only contains letters.</li>
95+
* <li>a multi character {@code opt} that only contains valid characters</li>
5396
* </ul>
54-
* <p>
97+
* </p><p>
98+
* A character is valid if any of the following conditions are true:
99+
* <ul>
100+
* <li>it is a letter</li>
101+
* <li>it is a currency symbol (such as '$')</li>
102+
* <li>it is a connecting punctuation character (such as '_')</li>
103+
* <li>it is a digit</li>
104+
* <li>it is a numeric letter (such as a Roman numeral character)</li>
105+
* <li>it is a combining mark</li>
106+
* <li>it is a non-spacing mark</li>
107+
* <li>isIdentifierIgnorable returns true for the character</li>
108+
* <li>it is a hyphen/dash ('-')</li>
109+
* </ul>
110+
* </p><p>
55111
* In case {@code opt} is {@code null} no further validation is performed.
56112
*
57113
* @param option The option string to validate, may be null
@@ -63,18 +119,16 @@ static String validate(final String option) throws IllegalArgumentException {
63119
return null;
64120
}
65121

66-
// handle the single character opt
67-
if (option.length() == 1) {
68-
final char ch = option.charAt(0);
122+
char[] chars = option.toCharArray();
69123

70-
if (!isValidOpt(ch)) {
71-
throw new IllegalArgumentException("Illegal option name '" + ch + "'");
72-
}
73-
} else {
74-
// handle the multi character opt
75-
for (final char ch : option.toCharArray()) {
76-
if (!isValidChar(ch)) {
77-
throw new IllegalArgumentException("The option '" + option + "' contains an illegal " + "character : '" + ch + "'");
124+
if (!isValidOpt(chars[0])) {
125+
throw new IllegalArgumentException("Illegal option name '" + chars[0] + "'");
126+
}
127+
// handle the multi-character opt
128+
if (option.length() > 1) {
129+
for (int i = 1; i < chars.length; i++) {
130+
if (!isValidChar(chars[i])) {
131+
throw new IllegalArgumentException("The option '" + option + "' contains an illegal " + "character : '" + chars[i] + "'");
78132
}
79133
}
80134
}

‎src/test/java/org/apache/commons/cli/OptionValidatorTest.java‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
/*
2+
Licensed to the Apache Software Foundation (ASF) under one or more
3+
contributor license agreements. See the NOTICE file distributed with
4+
this work for additional information regarding copyright ownership.
5+
The ASF licenses this file to You under the Apache License, Version 2.0
6+
(the "License"); you may not use this file except in compliance with
7+
the License. You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
*/
17+
18+
package org.apache.commons.cli;
19+
20+
import static org.junit.jupiter.api.Assertions.assertEquals;
21+
import static org.junit.jupiter.api.Assertions.assertFalse;
22+
import static org.junit.jupiter.api.Assertions.assertThrows;
23+
24+
import java.util.ArrayList;
25+
import java.util.List;
26+
import java.util.stream.Stream;
27+
28+
import org.junit.jupiter.api.BeforeAll;
29+
import org.junit.jupiter.api.Test;
30+
import org.junit.jupiter.params.ParameterizedTest;
31+
import org.junit.jupiter.params.provider.Arguments;
32+
import org.junit.jupiter.params.provider.MethodSource;
33+
34+
public class OptionValidatorTest {
35+
36+
/*
37+
* Exemplars of various types of characters
38+
*/
39+
40+
private static final String LETTERS = "a\u00D1"; // a and Ñ
41+
42+
// '\u0660' through '\u0669', Arabic-Indic digits, '\u06F0' through '\u06F9',
43+
// Extended Arabic-Indic digits
44+
// '\u0966' through '\u096F', Devanagari digits, '\uFF10' through '\uFF19',
45+
// Fullwidth digits
46+
private static final String DIGITS = "1\u0661\u06f2\u0968\uFF14";
47+
48+
private static final String CURRENCY = "€$";
49+
50+
// this is the complete puncutation set do not modify it as Character.isJavaIdentifierPart filters
51+
// the good and bad ones out in the setup.
52+
private static final String PUNCTUATION = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~";
53+
54+
private static final String COMBINING_MARK = "\u0303";
55+
56+
private static final String NON_SPACING_MARK = "\u0CBF";
57+
58+
private static final String IDENTIFIER_IGNORABLE = "\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\u0008";
59+
60+
private static String acceptablePunctuation;
61+
62+
private static String notAcceptablePunctuation;
63+
64+
private static String additionalOptonChars;
65+
private static String additionalLongChars;
66+
67+
private static String firstChars;
68+
private static String notFirstChars;
69+
70+
private static String restChars;
71+
private static String notRestChars;
72+
73+
@BeforeAll
74+
public static void setup() {
75+
StringBuilder sb = new StringBuilder();
76+
StringBuilder sb2 = new StringBuilder();
77+
int idx;
78+
79+
for (char c : PUNCTUATION.toCharArray()) {
80+
if (Character.isJavaIdentifierPart(c)) {
81+
sb.append(c);
82+
} else {
83+
sb2.append(c);
84+
}
85+
}
86+
acceptablePunctuation = sb.toString();
87+
notAcceptablePunctuation = sb2.toString();
88+
89+
sb = new StringBuilder();
90+
for (char c : OptionValidator.ADDITIONAL_LONG_CHARS) {
91+
sb.append(c);
92+
}
93+
additionalLongChars = sb.toString();
94+
95+
sb = new StringBuilder();
96+
for (char c : OptionValidator.ADDITIONAL_OPTION_CHARS) {
97+
sb.append(c);
98+
}
99+
additionalOptonChars = sb.toString();
100+
101+
String javaIdentifierPart = LETTERS + DIGITS + CURRENCY + acceptablePunctuation + COMBINING_MARK
102+
+ NON_SPACING_MARK + IDENTIFIER_IGNORABLE;
103+
104+
firstChars = additionalOptonChars + javaIdentifierPart;
105+
106+
sb = new StringBuilder(notAcceptablePunctuation).append(additionalLongChars);
107+
for (char c : OptionValidator.ADDITIONAL_OPTION_CHARS) {
108+
while ((idx = sb.indexOf(Character.toString(c))) > -1) {
109+
sb.deleteCharAt(idx);
110+
}
111+
}
112+
notFirstChars = sb.toString();
113+
114+
restChars = additionalLongChars + javaIdentifierPart;
115+
sb = new StringBuilder(notAcceptablePunctuation).append(additionalOptonChars);
116+
for (char c : OptionValidator.ADDITIONAL_LONG_CHARS) {
117+
while ((idx = sb.indexOf(Character.toString(c))) > -1) {
118+
sb.deleteCharAt(idx);
119+
}
120+
}
121+
notRestChars = sb.toString();
122+
123+
}
124+
125+
@Test
126+
public void testExclusivity() {
127+
/* since we modify acceptable chars by add and removing ADDITIONAL* chars we must verify that they do not exist in the
128+
* base javaIdentiferPart that is used in OptionValidator to validate basic characters */
129+
for (char c : OptionValidator.ADDITIONAL_LONG_CHARS) {
130+
assertFalse(Character.isJavaIdentifierPart(c), () -> String.format("'%s' should not be in 'ADDITIONAL_LONG_CHARS", c));
131+
}
132+
for (char c : OptionValidator.ADDITIONAL_OPTION_CHARS) {
133+
assertFalse(Character.isJavaIdentifierPart(c), () -> String.format("'%s' should not be in 'ADDITIONAL_OPTION_CHARS", c));
134+
}
135+
}
136+
137+
@ParameterizedTest(name = "{2}")
138+
@MethodSource("optionParameters")
139+
public void validateTest(final String str, final boolean expected, final String name) {
140+
if (expected) {
141+
assertEquals(str, OptionValidator.validate(str));
142+
} else {
143+
assertThrows(IllegalArgumentException.class, () -> OptionValidator.validate(str));
144+
}
145+
}
146+
147+
private static Stream<Arguments> optionParameters() {
148+
149+
List<Arguments> args = new ArrayList<>();
150+
151+
args.add(Arguments.of("CamelCase", true, "Camel case error"));
152+
args.add(Arguments.of("Snake_case", true, "Snake case error"));
153+
args.add(Arguments.of("_leadingUnderscore", true, "Leading underscore error"));
154+
args.add(Arguments.of("kabob-case", true, "Kabob case error"));
155+
args.add(Arguments.of("-leadingDash", false, "Leading dash error"));
156+
args.add(Arguments.of("lowercase", true, "Lower case error"));
157+
args.add(Arguments.of("UPPERCASE", true, "Upper case error"));
158+
159+
// build passing test cases
160+
for (char c : firstChars.toCharArray()) {
161+
String s = String.format("%sMoreText", c);
162+
args.add(Arguments.of(s, true, String.format("testing: First character '%s'", c)));
163+
}
164+
165+
for (char c : restChars.toCharArray()) {
166+
String s = String.format("Some%sText", c);
167+
args.add(Arguments.of(s, true, String.format("testing: Middle character '%s'", c)));
168+
}
169+
170+
// build failing test cases
171+
for (char c : notFirstChars.toCharArray()) {
172+
String s = String.format("%sMoreText", c);
173+
args.add(Arguments.of(s, false, String.format("testing: Bad first character '%s'", c)));
174+
}
175+
176+
for (char c : notRestChars.toCharArray()) {
177+
String s = String.format("Some%sText", c);
178+
args.add(Arguments.of(s, false, String.format("testing: Bad middle character '%s'", c)));
179+
}
180+
181+
return args.stream();
182+
}
183+
}

0 commit comments

Comments
 (0)