ptuomola commented on pull request #1426: URL: https://github.com/apache/fineract/pull/1426#issuecomment-711362015
@vorburger I tried to understand what this code was originally introduced for. As far as I can see, the code in retrieveImage() used to do result.getContent() != null to see if it actually got an image. The problem with that is that calling getContent() reads the full input stream to EoS - so the image content is then not available later when the code tries to actually get it with getContent() when building the response to client. So they coded available() as sort of "peek" to determine whether there's data to be returned in a "non-invasive" way. So as you say the risk that this change runs is that there's some way for the image not to be available that the original code was build to cater for. It's clearly not the scenario where the file is missing altogether - but perhaps there's some case where the file is deemed present but no content is returned? But if that's the case then would be interesting to understand when it arises... In summary I think this is OK to merge in my view as long as we keep our eyes open for some failure scenario with missing content / image data... ---------------------------------------------------------------- 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]
