Fix contract for FileSourceBlobStore #2451
Conversation
…it should return Optional.empty() wiremock#2420
dkhozyainov
changed the title
@oleg-nenashev Can you tell me how nice it is to make diagnostic logs? maybe there is an example somewhere? |
|
WireMock uses I normally use stock logging frameworks, so not sure how much of debug logging we can make with it |
fix code style, add logs
|
@oleg-nenashev thanks for the hint, I added it in the next commit+ fixed code style |
| return Optional.of(fileSource.getBinaryFileNamed(key).readContents()); | ||
| try { | ||
| return Optional.of(fileSource.getBinaryFileNamed(key).readContents()); | ||
| } catch (Exception exception) { |
There was a problem hiding this comment.
How about we catch FileNotFoundException and don't log anything, since this is a normal event, then log at error for any other type of exception (which in practice would probably indicate a misconfigured file system)?
There was a problem hiding this comment.
Done. I used instanceof because FileNotFoundException is cast to Throwable in the BinaryFile class via the throwUnchecked method
fix comment
| notifier() | ||
| .info("Error when working with FileSource:\n" + Json.write(exception.getMessage())); | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
I think we should still throw the exception if it's not FileNotFoundException. Only return empty when it's exactly that exception.
We can probably do this with two catch blocks rather than instanceof.
The notifier call should be at error if it's not a not-found error.
There was a problem hiding this comment.
Unfortunately, catch FileNotFoundException is unavailable due to wrapping into an unchecked exception in the Binary File class via the throwUnchecked method.
Other comments fixed
fix comment
|
Thanks! |
Fix for: FileSourceBlobStore throws FileNotFoundExceptions when it should return Optional.empty() wiremock#2420 --------- Co-authored-by: d.khoziainov <d.khoziainov@uzum.com>
Fix for: FileSourceBlobStore throws FileNotFoundExceptions when it should return Optional.empty() wiremock#2420 --------- Co-authored-by: d.khoziainov <d.khoziainov@uzum.com>
Greetings, please look at my PR for #2420
Submitter checklist
#help-contributingor a project-specific channel like#wiremock-java