Mark, On 3/15/24 13:27, markt-asf (via GitHub) wrote:
markt-asf commented on code in PR #707: URL: https://github.com/apache/tomcat/pull/707#discussion_r1526610303 ########## java/org/apache/catalina/realm/JNDIRealm.java: ########## @@ -966,7 +967,7 @@ private String[] getCipherSuitesArray() { containerLog.warn(sm.getString("jndiRealm.emptyCipherSuites")); this.cipherSuitesArray = null; } else { - this.cipherSuitesArray = cipherSuites.trim().split("\\s*,\\s*"); + this.cipherSuitesArray = StringUtils.splitCommaSeparated(cipherSuites.trim()); if (containerLog.isTraceEnabled()) { Review Comment: Is this trim necessary here? I know it was there before but it looks redundant.
Looking back, it does make sense to remove this. I was thinking that as I wrote it, but the trim was technically necessary before the change from regex to split-then-trim.
I'll shave off this sharp edge.
########## java/org/apache/tomcat/util/buf/StringUtils.java: ########## @@ -94,4 +94,28 @@ public static <T> void join(Iterable<T> iterable, char separator, Function<T,Str sb.append(function.apply(value)); } } + + /** + * Splits a comma-separated string into an array of String values. + * + * Whitespace around the commas is removed. + * + * Null or empty values will return a zero-element array. + * + * @param s The string to split by commas. + * + * @return An array of String values. + */ + public static String[] splitCommaSeparated(String s) { + if (s == null || s.length() == 0) { + return new String[0]; + } + + String[] splits = s.split(","); + for (int i=0; i<splits.length; ++i) { Review Comment: The formatting police are going to complain about spaces around operators here.
Checkstyle was happy enough. :) I'll get that corrected. -chris --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org