New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #6654 - Fix NPEs from bad Servlet API use #6655
Issue #6654 - Fix NPEs from bad Servlet API use #6655
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@@ -135,10 +140,13 @@ public int getHeaderInt(String name) | |||
{ | |||
Map<String, List<String>> headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); | |||
Enumeration<String> headerNames = request.getHeaderNames(); | |||
while (headerNames.hasMoreElements()) | |||
if (headerNames != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The servlet spec indicates that this can also be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like returning null
if there are no cookies, as it is prone to errors 😃
...-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/ServerUpgradeRequest.java
Show resolved
Hide resolved
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
} | ||
else | ||
{ | ||
cookies = List.of(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also return null here if the request had no cookies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had this discussion already recently and determined that emptyList
was both more readable and more effficient.
cookies = List.of(); | |
cookies = Collections.emptyList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one tiny niggle....
} | ||
else | ||
{ | ||
cookies = List.of(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had this discussion already recently and determined that emptyList
was both more readable and more effficient.
cookies = List.of(); | |
cookies = Collections.emptyList(); |
The call to |
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com