Adding null checks for instantiating InstrHttpInputStream/InstrHttpOutputStream #3415
Conversation
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
|
/test smoke-tests |
| if (outputStream != null) { | ||
| return new InstrHttpOutputStream(outputStream, networkMetricBuilder, timer); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Is the caller of getOutputStream() okay to deal with null?
There was a problem hiding this comment.
This question would apply to both input and output stream.
I suspect that it depends on whether the developers thinks a null may occur with getInputStream (but since the docs don't mention it can be null, devs may not add a check).
However, we use this class to wrap the original url connect and call the underlying object as a delegate. We shouldn't be returning our instrumented input stream when the original url connection would have returned a null. If they did not have Firebase Performance, they would have needed to handle the null case anyways.
There was a problem hiding this comment.
I updated it to return outputStream instead of return null to showcase that we are not changing the original behavior.
There was a problem hiding this comment.
Makes sense! Thanks!
raymondlam
force-pushed
the
fixNPE_3406
branch
from
6bf1959 to
c02f4e6
Compare
|
/test smoke-tests |
This should remove the possibility of a null being passed to InstrHttpInputStream/InstrHttpOutputStream.
Fixes #3406