Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close unreleased InputStreams in the Rule Engines and ApiReader #1234

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Close unreleased InputStreams in the Rule Engines and ApiReader #1234

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c007b12
Update OpenApiReaderImplementation.java
luis-pabon-tf Aug 8, 2024
977cd7c
Close inputStreams quietly on rule engines
luis-pabon-tf Aug 8, 2024
9a0552e
Used a managed scope on the InputStream, and added a parameter to ope…
luis-pabon-tf Aug 9, 2024
7f2c7e1
Update OpenApiReaderImplementation.java
luis-pabon-tf Aug 12, 2024
df1fbd2
Revert new parameter on OpenApiReader.openAsString
luis-pabon-tf Aug 12, 2024
3ae31d5
Merge branch 'main' into story/1212/release-resources
luis-pabon-tf Aug 12, 2024
6ff3051
Use try-with-resources for the InputStream uses
luis-pabon-tf Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/src/main/java/gov/hhs/cdc/trustedintermediary/OpenApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import gov.hhs.cdc.trustedintermediary.external.openapi.OpenApiReaderImplementation;
import gov.hhs.cdc.trustedintermediary.wrappers.YamlCombiner;
import gov.hhs.cdc.trustedintermediary.wrappers.YamlCombinerException;
import java.nio.charset.StandardCharsets;
import java.util.Set;

/** Directs the construction of a full YAML OpenAPI specification */
Expand Down Expand Up @@ -37,6 +38,7 @@ public String generateApiDocumentation(final Set<String> openApiSpecifications)

String getBaselineDocumentation() throws UnableToReadOpenApiSpecificationException {
String fileName = "openapi_base.yaml";
return OpenApiReaderImplementation.getInstance().openAsString(fileName);
return OpenApiReaderImplementation.getInstance()
.openAsString(fileName, StandardCharsets.UTF_8);
}
}
4 changes: 3 additions & 1 deletion app/src/main/java/gov/hhs/cdc/trustedintermediary/auth/AuthDomainRegistration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import gov.hhs.cdc.trustedintermediary.external.openapi.OpenApiReaderImplementation;
import gov.hhs.cdc.trustedintermediary.wrappers.InvalidTokenException;
import gov.hhs.cdc.trustedintermediary.wrappers.Logger;
import java.nio.charset.StandardCharsets;
import java.util.Map;
import java.util.function.Function;
import javax.inject.Inject;
Expand Down Expand Up @@ -41,7 +42,8 @@ public Map<HttpEndpoint, Function<DomainRequest, DomainResponse>> domainRegistra
@Override
public String openApiSpecification() throws UnableToReadOpenApiSpecificationException {
String fileName = "openapi_auth.yaml";
return OpenApiReaderImplementation.getInstance().openAsString(fileName);
return OpenApiReaderImplementation.getInstance()
.openAsString(fileName, StandardCharsets.UTF_8);
}

DomainResponse handleAuth(DomainRequest request) {
Expand Down
4 changes: 3 additions & 1 deletion etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/EtorDomainRegistration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import gov.hhs.cdc.trustedintermediary.wrappers.FhirParseException;
import gov.hhs.cdc.trustedintermediary.wrappers.HapiFhir;
import gov.hhs.cdc.trustedintermediary.wrappers.Logger;
import java.nio.charset.StandardCharsets;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had made this change for this PR. Reverted the change, the diff should be a bit smaller now too

}

DomainResponse handleOrders(DomainRequest request) {
Expand Down
2 changes: 2 additions & 0 deletions .../hhs/cdc/trustedintermediary/etor/ruleengine/transformation/TransformationRuleEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.ArrayList;
import java.util.List;
import javax.inject.Inject;
import org.apache.commons.io.IOUtils;

/** Implements the RuleEngine interface. It represents a rule engine for transformations. */
public class TransformationRuleEngine implements RuleEngine {
Expand Down Expand Up @@ -53,6 +54,7 @@ public void ensureRulesLoaded() throws RuleLoaderException {
ruleLoader.loadRules(resourceStream, new TypeReference<>() {});
rules.addAll(parsedRules);
rulesLoaded = true;
IOUtils.closeQuietly(resourceStream);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use this. Use the "try with resources" mechanism. I.e. the same thing that @basiliskus mentioned below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the catch, I would catch IOException | NullPointerException and throw a throw new RuleLoaderException("File not found: " + ruleDefinitionsFileName, new FileNotFoundException()); like above.

}
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...java/gov/hhs/cdc/trustedintermediary/etor/ruleengine/validation/ValidationRuleEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.ArrayList;
import java.util.List;
import javax.inject.Inject;
import org.apache.commons.io.IOUtils;

/** Implements the RuleEngine interface. It represents a rule engine for validations. */
public class ValidationRuleEngine implements RuleEngine {
Expand Down Expand Up @@ -53,6 +54,7 @@ public void ensureRulesLoaded() throws RuleLoaderException {
ruleLoader.loadRules(resourceStream, new TypeReference<>() {});
rules.addAll(parsedRules);
rulesLoaded = true;
IOUtils.closeQuietly(resourceStream);
}
}
}
Expand Down
14 changes: 8 additions & 6 deletions ...in/java/gov/hhs/cdc/trustedintermediary/external/openapi/OpenApiReaderImplementation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import gov.hhs.cdc.trustedintermediary.wrappers.OpenApiReader;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.charset.Charset;
import java.util.Objects;

/**
Expand All @@ -22,15 +22,17 @@ public static OpenApiReaderImplementation getInstance() {
* Loads the stream of an OpenApi file and returns the output as a string.
*
* @param fileName Name of the file to load
* @param charset Charset used for the output string
* @return File contents as string
* @throws UnableToReadOpenApiSpecificationException If there is an issue loading the Api file
*/
@Override
public String openAsString(String fileName) throws UnableToReadOpenApiSpecificationException {
InputStream stream = getClass().getClassLoader().getResourceAsStream(fileName);
try {
return new String(
Objects.requireNonNull(stream).readAllBytes(), StandardCharsets.UTF_8);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the Objects.requireNonNull(). stream will be not be null if we get to this point.

Copy link
Contributor Author

@luis-pabon-tf luis-pabon-tf Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

stream.close();
luis-pabon-tf marked this conversation as resolved.
Show resolved Hide resolved
luis-pabon-tf marked this conversation as resolved.
Show resolved Hide resolved
return out;
} catch (IOException | NullPointerException e) {
throw new UnableToReadOpenApiSpecificationException(
"Failed to open OpenAPI specification for " + fileName, e);
Expand Down
4 changes: 3 additions & 1 deletion shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/OpenApiReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package gov.hhs.cdc.trustedintermediary.wrappers;

import gov.hhs.cdc.trustedintermediary.domainconnector.UnableToReadOpenApiSpecificationException;
import java.nio.charset.Charset;

/** Wrapper for classes that load OpenApi documentation. */
public interface OpenApiReader {
String openAsString(String fileName) throws UnableToReadOpenApiSpecificationException;
String openAsString(String fileName, Charset charset)
throws UnableToReadOpenApiSpecificationException;
}