fix NullPointerException #551
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeitlinger
requested review from
AlexanderYastrebov,
lukasniemeier-zalando,
pc-alves and
whiskeysierra
as code owners
fatroom
reviewed
Apr 23, 2021
...nsion/src/main/java/org/zalando/opentracing/spring/webflux/extension/ErrorSpanDecorator.java
Outdated
Show resolved
Hide resolved
|
@zeitlinger Is this fixing the NPE on #550 ? |
no, I think it's unrelated. |
|
Now you need to provide test, to satisfy coverage and let the build pass |
| @@ -30,7 +30,8 @@ public void onResponse( | |||
| final ServerWebExchange exchange, | |||
| final Span span) { | |||
|
|
|||
| if (predicate.test(exchange.getResponse().getStatusCode())) { | |||
| HttpStatus statusCode = exchange.getResponse().getStatusCode(); | |||
| if (statusCode == null || predicate.test(statusCode)) { | |||
There was a problem hiding this comment.
Under which circumstances is the status null?
There was a problem hiding this comment.
Javadoc: The return value may be null if the status code value is outside the HttpStatus enum range, or if there is no default value from the underlying server.
There was a problem hiding this comment.
it actually happened..
There was a problem hiding this comment.
I'm not questioning that it happens. But it's important to know the reason, otherwise it's impossible to judge whether null should be considered an error.
Do we have access to the original, low level status as an integer?
There was a problem hiding this comment.
The javadoc suggests that it's just a normal case where nobody set a status code yet and there is no default. Doesn't sound like an error to me. Neither does a value out of range.
There was a problem hiding this comment.
HTTP/1.1 mandates presence of status codes, so missing status code seems rather error to me than not
There was a problem hiding this comment.
there's getRawStatusCode() exists, maybe verifying based on it would be better. If not present - error, if present but unknown - not an error.
There was a problem hiding this comment.
This is a server side filter. I don't think absent status code means what you think it means here.
There was a problem hiding this comment.
raw status also doesn't help
@Nullable
default Integer getRawStatusCode() {
HttpStatus httpStatus = getStatusCode();
return (httpStatus != null ? httpStatus.value() : null);
}
zeitlinger
force-pushed
the
fix_npe
branch
4 times, most recently
from
54de9cd
to
28ac0aa
Compare
|
@fatroom can't get my commit to be signed.. can you help out? |
|
@zeitlinger I don't have write access to his repo. Tried to sign in #552 but my sign seems not recognised as well by GH? |
|
👍 |
|
Anyway as I see there's no checks for signed commits on this repo, so it's just a question of write access to merge it. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@zalando.de>
figured out how to sign the commit... |
|
👍 |
|
I'm still not convinced that this is the right change to address that issue. Under which circumstances has someone observed the faulty behavior? Client- or server side? Which dependencies were in play? What was the actual response that the user/client received? |
|
I had this error come up when running an integration test. The Spring Boot WebFlux version was 2.4.2 and the test looked like this: |
|
Where exactly did the error occur in this? Can you post the stack trace here? |
|
Sure, this is the stacktrace: |
|
This is the server side that throws and it's the test implementation for Spring WebFlux. This feels like the wrong fix to even more now. |
can you suggest a different fix - or how we should proceed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.