Close unreleased InputStreams in the Rule Engines and ApiReader #1234
Conversation
Add closing statement before returning the output of openAsString
.../main/java/gov/hhs/cdc/trustedintermediary/external/openapi/OpenApiReaderImplementation.java
Outdated
Show resolved
Hide resolved
.../main/java/gov/hhs/cdc/trustedintermediary/external/openapi/OpenApiReaderImplementation.java
Outdated
Show resolved
Hide resolved
.../main/java/gov/hhs/cdc/trustedintermediary/external/openapi/OpenApiReaderImplementation.java
Outdated
Show resolved
Hide resolved
| @@ -143,7 +144,8 @@ public Map<HttpEndpoint, Function<DomainRequest, DomainResponse>> domainRegistra | |||
| @Override | |||
| public String openApiSpecification() throws UnableToReadOpenApiSpecificationException { | |||
| String fileName = "openapi_etor.yaml"; | |||
| return OpenApiReaderImplementation.getInstance().openAsString(fileName); | |||
| return OpenApiReaderImplementation.getInstance() | |||
| .openAsString(fileName, StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
I wouldn't pass in StandardCharset.UTF_8. There's no reason for .openAsString(...) to support other charsets. Instead, hardcode StandardCharsets.UTF_8 in OpenApiReaderImplementation#openAsString.
This applies to the other places where we call OpenApiReaderImplementation.getInstance() .openAsString(...).
There was a problem hiding this comment.
I had made this change for this PR. Reverted the change, the diff should be a bit smaller now too
| public String openAsString(String fileName, Charset charset) | ||
| throws UnableToReadOpenApiSpecificationException { | ||
| try (InputStream stream = getClass().getClassLoader().getResourceAsStream(fileName)) { | ||
| var out = new String(Objects.requireNonNull(stream).readAllBytes(), charset); |
There was a problem hiding this comment.
I don't think we need the Objects.requireNonNull(). stream will be not be null if we get to this point.
There was a problem hiding this comment.
I have that there to silence a warning on the IDE, but since we're catching the NullPointerException that it's worried about it's probably a false positive. Will do!
| @@ -53,6 +54,7 @@ public void ensureRulesLoaded() throws RuleLoaderException { | |||
| ruleLoader.loadRules(resourceStream, new TypeReference<>() {}); | |||
| rules.addAll(parsedRules); | |||
| rulesLoaded = true; | |||
| IOUtils.closeQuietly(resourceStream); | |||
There was a problem hiding this comment.
I wouldn't use this. Use the "try with resources" mechanism. I.e. the same thing that @basiliskus mentioned below.
There was a problem hiding this comment.
For the catch, I would catch IOException | NullPointerException and throw a throw new RuleLoaderException("File not found: " + ruleDefinitionsFileName, new FileNotFoundException()); like above.
|
|
We're keeping the engine code separate as they are expected to be very different in the future. |
Close unreleased InputStreams in the Rule Engines and ApiReader
Added closing statements in such a way that output is unchanged for a couple methods.
Issue
#1212
Checklist
Note: You may remove items that are not applicable