bmarwell commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599715248
##########
File path:
core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,11 +212,14 @@ protected void processUserDefinitions(Map<String, String>
userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
- }
- return set;
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (NoSuchElementException e) {
+ e.printStackTrace();
+ }
+ return set;
Review comment:
> how was this the exception triggered?
> Link to Apache's SonarCloud: the bug.
yields: `Use try-with-resources or close this "Scanner" in a "finally"
clause.`
That is, you would not want to catch the exception. Just the try would be
fine in this case, as this is only about closing the scanner. But as we are
reading a String, this is a noop anyway:
https://github.com/openjdk/jdk/blob/d7268fa3a68c2356548651a27b307d7f7158e700/src/java.base/share/classes/java/util/Scanner.java#L1171-L1175
So in this case, while Sonar is right in the sense of "good habit" (as the
source could change): My -1 on this as we are not "fixing" anything or making
anything better. Closing the scanner is fine for me, but there was no
underlying bug and the API would also never be able to change to something
`Closeable`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
