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

Bump PMD with fixes #25

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Bump PMD with fixes #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ tasks.named('test') {
}

pmd {
toolVersion = '6.20.0'
toolVersion = '7.5.0'
ruleSetConfig = rootProject.resources.text.fromFile('config/pmd/ruleset.xml')
ruleSets = []
ignoreFailures = false
Expand Down
12 changes: 3 additions & 9 deletions config/pmd/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,11 @@
<exclude-pattern>.*/test/.*</exclude-pattern>

<rule ref="category/java/errorprone.xml">
<exclude name="DataflowAnomalyAnalysis"/>
<exclude name="MissingSerialVersionUID"/>
<exclude name="BeanMembersShouldSerialize"/>
</rule>

<rule ref="category/java/multithreading.xml">
<!-- Because it complains for the Map interface in the Resource class.-->
<!-- Because it complains about the Map interface in the Resource class.-->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We share Pub-API's PMD spec, but this isn't relevant for us…weird. Probably need to decide what to do about "Standard" and local colour in another meeting.

<exclude name="UseConcurrentHashMap"/>
</rule>

Expand All @@ -42,12 +40,8 @@
<exclude name="UnnecessaryConstructor"/>
<!-- Conflicts with the rule category/java/codestyle.xml/AvoidProtectedMethodInFinalClassNotExtending -->
<exclude name="CommentDefaultAccessModifier"/>
<!-- Conflicts with the rule category/java/codestyle.xml/AvoidProtectedMethodInFinalClassNotExtending -->
<exclude name="DefaultPackage"/>


<!--<exclude name="BeanMembersShouldSerialize"/>-->

<!-- We use vars! -->
<exclude name="UseExplicitTypes"/>
<exclude name="ConfusingTernary"/>

<!-- printStackTrace is used in the Default Amazon handler in a static clause
Expand Down
30 changes: 17 additions & 13 deletions src/main/java/no/sikt/generator/ApiData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import io.swagger.v3.oas.models.OpenAPI;
import java.util.HashMap;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import nva.commons.core.JacocoGenerated;
import nva.commons.core.attempt.Failure;
Expand All @@ -24,10 +23,10 @@ public class ApiData {

private final RestApi awsRestApi;
private final OpenAPI openapiApiGateway;
private Optional<OpenAPI> openapiApiGithub;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional fields are a no-no, I replaced this with a private getter…it serves little real purpose aside hiding null checks. I suspect we simply have a suboptimal design here (typically, we see this kind of thing when the OO is off), which should probably be rectified.

private final String rawYaml;
private final Stage stage;
private static final Logger logger = LoggerFactory.getLogger(ApiData.class);
private OpenAPI openapiApiGithub;

public ApiData(RestApi awsRestApi, OpenAPI openapiApiGateway, String rawYaml, Stage stage) {
this.awsRestApi = awsRestApi;
Expand Down Expand Up @@ -64,6 +63,10 @@ public int getDashesInPath() {
return Try.attempt(this::getNumberOfDashesInBasePath).orElse(this::handleGetDashesFailure);
}

private Optional<OpenAPI> getOpenapiApiGithub() {
return Optional.ofNullable(openapiApiGithub);
}

public void setMatchingGithubOpenapi(List<Pair<S3Object, OpenAPI>> templateOpenapiDocs) {
var title = this.openapiApiGateway.getInfo().getTitle();
var matchingGithubOpenApi = templateOpenapiDocs.stream()
Expand All @@ -73,16 +76,15 @@ public void setMatchingGithubOpenapi(List<Pair<S3Object, OpenAPI>> templateOpena
.equals(title))
.findFirst();
if (matchingGithubOpenApi.isPresent()) {
logger.info("Using matching github openapi at " + matchingGithubOpenApi.get().getLeft().key()+ " for " + title);
this.openapiApiGithub = Optional.of(matchingGithubOpenApi.get().getRight());
logger.info("Using matching github openapi at "
+ matchingGithubOpenApi.get().getLeft().key()+ " for " + title);
this.openapiApiGithub = matchingGithubOpenApi.get().getRight();
} else {
logger.warn("No matching github openapi found for " + title);
this.openapiApiGithub = Optional.empty();
}
;
}

public ApiData setEmptySchemasIfNull() {
public ApiData applyEmptySchemasIfNull() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setters traditionally return void. I agree strongly with this pattern.

if (isNull(this.openapiApiGateway.getComponents())) {
this.openapiApiGateway.setComponents(new Components());
}
Expand All @@ -93,27 +95,29 @@ public ApiData setEmptySchemasIfNull() {
}

public ApiData overridePropsFromGithub() {
if (this.openapiApiGithub.isPresent()) {
var openApiGithub = getOpenapiApiGithub();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is here, it really adds nothing. Probably, there is a missing object.

if (openApiGithub.isPresent()) {
this.openapiApiGateway.getPaths().forEach((pathKey, pathItem) -> {
pathItem.readOperationsMap().forEach((httpMethod, operation) -> {
if (nonNull(operation.getParameters())) {
operation.getParameters().forEach(parameter -> {
var operationInGithub = Optional.ofNullable(openapiApiGithub.get().getPaths().get(pathKey).readOperationsMap()
var operationInGithub =
Optional.ofNullable(openApiGithub.get().getPaths().get(pathKey).readOperationsMap()
.get(httpMethod));
operationInGithub.ifPresent(value -> operation.setParameters(value.getParameters()));
});
}
});
});
if (nonNull(this.openapiApiGithub.get().getComponents())
&& nonNull(this.openapiApiGithub.get().getComponents().getSchemas())
if (nonNull(openApiGithub.get().getComponents())
&& nonNull(openApiGithub.get().getComponents().getSchemas())
) {
this.openapiApiGithub.get().getComponents().getSchemas().forEach(((key,value) -> {
openApiGithub.get().getComponents().getSchemas().forEach((key,value) -> {
if (isNull(this.openapiApiGateway.getComponents().getSchemas().get(key))) {
logger.info("Adding schema " + key + " from github");
this.openapiApiGateway.getComponents().getSchemas().put(key, value);
}
}));
});
}

}
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/no/sikt/generator/ApplicationConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ private static String readExternalBucketName() {
private static String readInternalBucketName() {
return ENVIRONMENT.readEnv("INTERNAL_BUCKET_NAME");
}
public static String readOpenApiBucketName() { return ENVIRONMENT.readEnv("OPEN_API_DOCS_BUCKET_NAME");}

public static String readOpenApiBucketName() {
return ENVIRONMENT.readEnv("OPEN_API_DOCS_BUCKET_NAME");
}

private static String readExternalCloudFrontDistributionId() {
return ENVIRONMENT.readEnv("EXTERNAL_CLOUD_FRONT_DISTRIBUTION");
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/no/sikt/generator/OpenApiCombiner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import io.swagger.v3.oas.models.PathItem;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.parameters.Parameter;
import io.swagger.v3.oas.models.security.SecurityRequirement;
import io.swagger.v3.oas.models.security.SecurityScheme;
import io.swagger.v3.oas.models.servers.Server;
import java.util.Collection;
Expand Down Expand Up @@ -118,8 +117,8 @@ private void removeTags(OpenAPI api) {

private void mergeSecurity(OpenAPI api) {
if (nonNull(api.getSecurity())) {
for (SecurityRequirement securityRequirement : api.getSecurity()) {
logger.info("adding security req {}", securityRequirement.toString());
for (var securityRequirement : api.getSecurity()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a cheap trick to avoid the prescription to use the interface, which here would have been quite harmful. It is not just us that makes things difficult on ourselves.

logger.info("adding security req {}", securityRequirement);
this.baseTemplate.addSecurityItem(securityRequirement);
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/no/sikt/generator/OpenApiExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import io.swagger.v3.oas.models.media.Schema;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import org.slf4j.Logger;
Expand All @@ -34,7 +33,7 @@ private void removeUnusedSchemas(OpenAPI openAPI) {
var usedSchemas = getUsedSchemas(openAPI);
var allSchemas = schemas.keySet();

Map<String, Schema> newSchemas = new HashMap();
var newSchemas = new HashMap<String, Schema>();

allSchemas.forEach(key -> {
var value = schemas.get(key);
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/no/sikt/generator/OpenApiUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import static java.util.Objects.nonNull;
import static java.util.stream.Collectors.toSet;
import static java.util.stream.Stream.concat;
import io.swagger.v3.oas.models.OpenAPI;
import io.swagger.v3.oas.models.Operation;
import io.swagger.v3.oas.models.PathItem;
Expand Down Expand Up @@ -96,12 +95,12 @@ public static List<Schema> getNestedSchemas(OpenAPI openAPI, Schema schema) {
public static List<Schema> getNestedSchemas(OpenAPI openAPI, List<Schema> visited, Schema schema) {
var nestedSchemas = Stream.of(
Stream.of(schema.getItems()),
OpenApiUtils.getReffedSchema(openAPI, schema),
OpenApiUtils.getNestedAllOfSchemas(schema),
OpenApiUtils.getNestedAnyOfSchemas(schema),
OpenApiUtils.getNestedOneOfSchemas(schema),
OpenApiUtils.getNestedPropertiesSchemas(schema),
OpenApiUtils.getAdditionalPropertiesSchemas(schema)
getReffedSchema(openAPI, schema),
getNestedAllOfSchemas(schema),
getNestedAnyOfSchemas(schema),
getNestedOneOfSchemas(schema),
getNestedPropertiesSchemas(schema),
getAdditionalPropertiesSchemas(schema)
).flatMap(stream -> stream).filter(Objects::nonNull).toList();

var newSchemas = nestedSchemas.stream().filter(ns -> !visited.contains(ns)).collect(toSet());
Expand All @@ -116,7 +115,8 @@ public static List<Schema> getNestedSchemas(OpenAPI openAPI, List<Schema> visite

private static Stream<Schema> getReffedSchema(OpenAPI openAPI, Schema schema) {
if (nonNull(schema.get$ref()) && schema.get$ref().startsWith(COMPONENTS_SCHEMAS)) {
return Stream.of(openAPI.getComponents().getSchemas().get(StringUtils.stripStart(schema.get$ref(), COMPONENTS_SCHEMAS)));
return Stream.of(openAPI.getComponents()
.getSchemas().get(StringUtils.stripStart(schema.get$ref(), COMPONENTS_SCHEMAS)));
} else {
return Stream.of();
}
Expand Down
17 changes: 10 additions & 7 deletions src/main/java/no/sikt/generator/handlers/GenerateDocsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,18 @@ public GenerateDocsHandler() {
.retryPolicy(retryPolicy)
.build();

var apiGatewayClient =
ApiGatewayAsyncClient.builder().overrideConfiguration(clientOverrideConfiguration).build();
var cloudFrontClient = CloudFrontClient.builder()
try (
var apiGatewayClient =
ApiGatewayAsyncClient.builder().overrideConfiguration(clientOverrideConfiguration).build()) {
this.apiGatewayHighLevelClient = new ApiGatewayHighLevelClient(apiGatewayClient);
}

try (var cloudFrontClient = CloudFrontClient.builder()
.httpClient(UrlConnectionHttpClient.builder().build())
.region(AWS_GLOBAL)
.build();

this.apiGatewayHighLevelClient = new ApiGatewayHighLevelClient(apiGatewayClient);
this.cloudFrontHighLevelClient = new CloudFrontHighLevelClient(cloudFrontClient);
.build()) {
this.cloudFrontHighLevelClient = new CloudFrontHighLevelClient(cloudFrontClient);
}
this.s3ClientInput = S3Driver.defaultS3Client().build();
this.s3ClientOutput = S3Driver.defaultS3Client().build();
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/no/sikt/generator/handlers/GenerateExternalDocsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,15 @@ public void handleRequest(InputStream input, OutputStream output, Context contex
cloudFrontHighLevelClient.invalidateAll(EXTERNAL_CLOUD_FRONT_DISTRIBUTION);
}

private Stream<ApiData> validateAndFilterApis(GetRestApisResponse apis, List<Pair<S3Object, OpenAPI>> templateOpenapiDocs) {
private Stream<ApiData> validateAndFilterApis(GetRestApisResponse apis,
List<Pair<S3Object, OpenAPI>> templateOpenapiDocs) {
return apis.items().stream()
.map(this::fetchProdApiData)
.filter(Objects::nonNull)
.filter(this::apiShouldBeIncluded)
.peek(apiData -> openApiValidator.validateOpenApi(apiData.getOpenapi()))
.peek(apiData -> apiData.setMatchingGithubOpenapi(templateOpenapiDocs))
.map(ApiData::setEmptySchemasIfNull)
.map(ApiData::applyEmptySchemasIfNull)
.map(ApiData::overridePropsFromGithub)
.sorted(ApiData::sortByDate)
.sorted(ApiData::sortByDashes)
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/no/sikt/generator/handlers/GenerateInternalDocsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,15 @@ public void handleRequest(InputStream input, OutputStream output, Context contex
cloudFrontHighLevelClient.invalidateAll(INTERNAL_CLOUD_FRONT_DISTRIBUTION);
}

private Stream<ApiData> validateAndFilterApis(GetRestApisResponse apis, List<Pair<S3Object, OpenAPI>> templateOpenapiDocs) {
private Stream<ApiData> validateAndFilterApis(GetRestApisResponse apis,
List<Pair<S3Object, OpenAPI>> templateOpenapiDocs) {
return apis.items().stream()
.map(this::fetchProdApiData)
.filter(Objects::nonNull)
.filter(this::apiShouldBeIncluded)
.peek(apiData -> openApiValidator.validateOpenApi(apiData.getOpenapi()))
.peek(apiData -> apiData.setMatchingGithubOpenapi(templateOpenapiDocs))
.map(ApiData::setEmptySchemasIfNull)
.map(ApiData::applyEmptySchemasIfNull)
.map(ApiData::overridePropsFromGithub)
.sorted(ApiData::sortByDate)
.sorted(ApiData::sortByDashes)
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/no/sikt/generator/handlers/PublishDocumentationsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ public PublishDocumentationsHandler() {
.retryPolicy(retryPolicy)
.build();

var apiGatewayClient =
ApiGatewayAsyncClient.builder().overrideConfiguration(clientOverrideConfiguration).build();
try (var apiGatewayClient =
ApiGatewayAsyncClient.builder().overrideConfiguration(clientOverrideConfiguration).build()) {

this.apiGatewayHighLevelClient = new ApiGatewayHighLevelClient(apiGatewayClient);
this.apiGatewayHighLevelClient = new ApiGatewayHighLevelClient(apiGatewayClient);
}
}

public PublishDocumentationsHandler(ApiGatewayHighLevelClient apiGatewayHighLevelClient) {
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/no/sikt/generator/handlers/GenerateInternalDocsHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ public void shouldOverrideStyleIfSetInGithubOpenapi() {
handler.handleRequest(null, null, null);
var openApi = readGeneratedOpenApi();
var parameter =
openApi.getPaths().get("/publication/{publicationIdentifier}").getGet().getParameters().stream().filter(param -> param.getName().equals("doNotRedirect")).findFirst().get();
openApi.getPaths().get("/publication/{publicationIdentifier}").getGet().getParameters().stream()
.filter(param -> param.getName().equals("doNotRedirect")).findFirst().get();

assertThat(parameter.getStyle(), is(equalTo(StyleEnum.FORM)));
assertThat(parameter.getExplode(), is(equalTo(false)));
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/no/sikt/generator/handlers/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ private static RestApi buildRestApiFromTestCase(TestCase testCase) {
public static void loadGithubOpenapiFile(S3Driver s3Driver, TestCase testCase) {
if (testCase.getContentGithub().isPresent()) {
Try.attempt(() ->
s3Driver.insertFile(UnixPath.of(String.valueOf(Path.of(testCase.getId() + ".yaml")))
, testCase.getContentGithub().get())
s3Driver.insertFile(UnixPath.of(String.valueOf(Path.of(testCase.getId() + ".yaml"))),
testCase.getContentGithub().get())
);
}
}
Expand All @@ -87,7 +87,8 @@ public static void setupTestcasesFromFiles(S3Client s3Client,
List<Pair<String, Optional<String>>> fileNames) {
var filePrefix = "openapi_docs/" + ((isNull(folder)) ? "" : folder + "/");
var testCases = fileNames.stream().map(fileName -> loadTestCase(filePrefix + fileName.getLeft(),
fileName.getRight().map(fn -> filePrefix + fn)) ).toList();
fileName.getRight()
.map(fn -> filePrefix + fn))).toList();

var s3driver = new S3Driver(s3Client, readOpenApiBucketName());
testCases.forEach(tc -> loadGithubOpenapiFile(s3driver, tc));
Expand Down
Loading