-
Notifications
You must be signed in to change notification settings - Fork 50
Add formatting check and fix formatting #1734
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
781d634
to
717cb57
Compare
@@ -210,8 +200,7 @@ static Process start(String username, | |||
} else if (redirects[1] == ProcessBuilderForWin32.Redirect.INHERIT) { | |||
stdHandles[1] = getHandle(FileDescriptor.out); | |||
} else { | |||
f1 = newFileOutputStream(redirects[1].file(), | |||
redirects[1].append()); | |||
f1 = newFileOutputStream(redirects[1].file(), redirects[1].append()); | |||
stdHandles[1] = getHandle(f1.getFD()); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Similar code fragments were detected in the same file at the following lines: 198:205, 207:214.
Refactoring can help improve code maintainability. Consider reducing duplicate code by extracting it into a separate method. You can then replace duplicated code with calls to this new method.
GreengrassService, ServiceLoadException> locateFunction) throws ServiceLoadException { | ||
@SuppressWarnings({"UseSpecificCatch", "PMD.AvoidCatchingThrowable", "PMD.AvoidDeeplyNestedIfStmts", | ||
"PMD.ConfusingTernary"}) | ||
private GreengrassService createGreengrassServiceInstance(Context.Value v, String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The cyclomatic complexity of this method is 24. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.
We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 561-615 into a separate method.
voidCompletableFuture.thenAccept(result -> {connection.closeConnection(0); this.close();}); | ||
CompletableFuture<Void> voidCompletableFuture = connection.sendProtocolMessage(responseHeaders, | ||
responsePayload.getBytes(StandardCharsets.UTF_8), responseMessageType, responseMessageFlag); | ||
voidCompletableFuture.thenAccept(result -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenAccept
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
@@ -198,17 +203,17 @@ private CompletableFuture<Integer> subscribe(String topic, QualityOfService qos) | |||
|
|||
@Override | |||
public CompletableFuture<SubscribeResponse> subscribe(Subscribe subscribe) { | |||
return subscribe(subscribe.getTopic(), | |||
QualityOfService.getEnumValueFromInteger(subscribe.getQos().getValue())) | |||
return subscribe(subscribe.getTopic(), QualityOfService.getEnumValueFromInteger(subscribe.getQos().getValue())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenApply
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
ex.getMessage()); | ||
} | ||
}); | ||
return continuation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method whenComplete
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
@@ -371,8 +370,8 @@ public CompletableFuture<UnsubscribeResponse> unsubscribe(String topic) { | |||
try (LockScope ls1 = LockScope.lock(lock)) { | |||
return connect().thenCompose((client) -> { | |||
logger.atDebug().kv(TOPIC_KEY, topic).log("Unsubscribing from topic"); | |||
return client.unsubscribe( | |||
new UnsubscribePacket.UnsubscribePacketBuilder().withSubscription(topic).build()) | |||
return client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenApply
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
.thenApply((i) -> new SubscribeResponse(null, subscribe.getQos().getValue(), null)); | ||
} | ||
|
||
@Override | ||
public CompletableFuture<PubAck> publish(Publish publish) { | ||
return publish(new MqttMessage(publish.getTopic(), publish.getPayload(), | ||
return publish( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenApply
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
"Error subscribing. Reason: " + rcString); | ||
}) | ||
.get(getMqttOperationTimeoutMillis(), TimeUnit.MILLISECONDS); | ||
subscribe(newReq).thenApply((v) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenApply
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
@@ -638,28 +674,33 @@ public void unsubscribe(UnsubscribeRequest request) | |||
if (subReq == null) { | |||
return; | |||
} | |||
unsubscribe(Unsubscribe.builder().subscriptionCallback(subReq.getCallback()) | |||
.topic(request.getTopic()).build()).thenAccept((m) -> cbMapping.remove(lookup)) | |||
unsubscribe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenAccept
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
return connection.publish(request) | ||
.whenComplete((response, throwable) -> { | ||
if (throwable == null && (response == null || response.isSuccessful())) { | ||
return connection.publish(request).whenComplete((response, throwable) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method whenComplete
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
@@ -277,8 +284,8 @@ public CompletableFuture<Boolean> connect() { | |||
}); | |||
} | |||
|
|||
connectionFuture = | |||
voidCompletableFuture.thenCompose((b) -> establishConnection(false)).thenApply((sessionPresent) -> { | |||
connectionFuture = voidCompletableFuture.thenCompose((b) -> establishConnection(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is using non-async method thenCompose
on CompletableFuture
(or CompletionStage
). Please be aware that task subscribed to CompletableFuture
(or CompletionStage
) through non-async method may be executed by the thread that completes the current CompletableFuture
(or CompletionStage
), or the thread that calls this non-async method to add subscription. In other words, the thread that executes the subscription is non-deterministic. If you prefer having fully control over threads, you may consider using the async variants instead. In addition, please make sure tasks subscribed to CompletableFuture
(or CompletionStage
) are short lived and non-blocking to avoid deadlock. Learn more: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/concurrent/CompletableFuture.html
CoralClient user: Please consider doing so to align with Coral Java Documentation.Work chained to CoralClient CompletableFuture
(or CompletionStage
) may be inadvertently scheduled on the internal pool of the library. Since the internal pool has limited number of threads, this may cause the pool to run out of available threads and eventually lead to a deadlock.
if (ARCH_ARM.equals(arch) || ARCH_AARCH64.equals(arch)) { | ||
String archDetail = com.aws.greengrass.util.platforms.Platform.getInstance() | ||
.createNewProcessRunner().sh("uname -m").toLowerCase(); | ||
String archDetail = com.aws.greengrass.util.platforms.Platform.getInstance().createNewProcessRunner() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
toLowerCase
The parameterless versions of String.toLowerCase()
and String.toUpperCase()
use the default locale of the JVM when transforming strings. This can have unintended results, including difficult-to-debug transient failures because case mapping differs based on locale.
Even if you aren’t writing code for a user experience that’s displayed to a customer, system internal code often uses case folding to normalize strings for comparison.
Not doing so can missing locale lead to errors when running on systems that are configured to use a different system locale. This can include developer desktops or build servers (not just your fleet hosts): many hours have been spent debugging errors that only occur on a specific machine due to its locale configuration.
Recommended solutions:
Always pass in a Locale
.
Language or locale-dependent processing:
Get the locale from the platform, use the language of the content, or configure it in your code and pass that to the method.
String firstName = "Ichabod";
Locale locale = // Where you obtain the locale from is platform dependent
String lowerCaseFirstName = firstName.toLowerCase(locale);
Internal processing that isn’t locale-dependent:
Example: S3 bucket name
Always specify the language/country-neutral locale Locale.ROOT
.
String imgTag = "IMG";
if ("img".equals(imgTag.toLowerCase(Locale.ROOT)) {
// do something
}
String rcString = SubAckPacket.SubAckReasonCode.UNSPECIFIED_ERROR.name(); | ||
try { | ||
rcString = SubAckPacket.SubAckReasonCode.getEnumValueFromInteger(v.getReasonCode()).name(); | ||
} catch (RuntimeException ignored) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Swallowing exceptions without logging can make it difficult to identify and troubleshoot issues, potentially leading to application instability, hidden bugs, or unexpected behavior. To avoid this, ensure that all exceptions are logged using appropriate logging levels. When logging exceptions, include the exception object or its stack trace to capture detailed information. For critical exceptions, rethrow them when necessary to provide sufficient context for effective troubleshooting.
Suggested remediation:
Handle all exceptions by creating a log for each exception using the message thrown.
@@ -554,2 +554,3 @@
} catch (RuntimeException ignored) {
+ log.error("RuntimeException is caught.", ignored);
}
* "loggerName": "Metrics-SystemMetrics", "timestamp": 1600127641506, "cause": | ||
* null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 98.
try { | ||
configuration = SerializerFactory.getFailSafeJsonObjectMapper() | ||
.readValue(fleetConfigStr, Configuration.class); | ||
configuration = SerializerFactory.getFailSafeJsonObjectMapper().readValue(fleetConfigStr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
bootstrapTaskStatusList.clear(); | ||
bootstrapTaskStatusList.addAll(SerializerFactory.getFailSafeJsonObjectMapper() | ||
.readValue(in, new TypeReference<List<BootstrapTaskStatus>>(){})); | ||
bootstrapTaskStatusList.addAll(SerializerFactory.getFailSafeJsonObjectMapper().readValue(in, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
* "loggerName": "Metrics-SystemMetrics", "timestamp": 1600127641506, "cause": | ||
* null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 193.
// start creating files and directories which may not be desired | ||
outStream.println(String.format(SHOW_VERSION_RESPONSE, | ||
DeviceConfiguration.getVersionFromBuildRecipeFile())); | ||
outStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
It appears that you are using println()
rather than a dedicated logging facility makes it difficult to monitor the program behavior. We recommend to use a Java logging facility rather than System.out
or System.err
.
case KERNEL_ACTIVATION: | ||
case KERNEL_ROLLBACK: | ||
case KERNEL_ACTIVATION : | ||
case KERNEL_ROLLBACK : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Problem: The 'switch-case' context does not contain a terminating statement and allows execution to fall through to the next 'switch-case'.
Learn more
Fix: Consider terminating the switch case with 'break'/'return'/'throw' as suitable
.request(innerRequestBuilder.build()).build()); | ||
ExecutableHttpRequest request = connManager.getClient() | ||
.prepareRequest(HttpExecuteRequest.builder() | ||
.contentStreamProvider(body == null ? null : () -> new ByteArrayInputStream(body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
} else { | ||
FileDescriptor stdinFd = new FileDescriptor(); | ||
setHandle(stdinFd, stdHandles[0]); | ||
stdinStream = new BufferedOutputStream(new FileOutputStream(stdinFd)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
Similar issue at line numbers 533 and 541.
return AccessController.doPrivileged(new PrivilegedAction<FileOutputStream>() { | ||
@Override | ||
public FileOutputStream run() { | ||
return new FileOutputStream(fd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
} | ||
LOGGER.info(String.format("%s authenticated identity: %s", serviceHandler.getServiceName(), authenticationData.getIdentityLabel())); | ||
LOGGER.info(String.format("%s authenticated identity: %s", serviceHandler.getServiceName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Using String.format(), string concatenation, or StringBuilder in logging statements can lead to unnecessary string formatting, even when the log level is disabled, potentially impacting application performance. SLF4J offers parameterized logging, which defers string formatting until the log message is actually needed. This approach can significantly improve efficiency, especially in high-throughput scenarios or when certain log levels are frequently disabled. Learn more - https://cwe.mitre.org/data/definitions/1176.html
continuation.close(); | ||
}); | ||
if (ex != null) { | ||
LOGGER.error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Using String.format(), string concatenation, or StringBuilder in logging statements can lead to unnecessary string formatting, even when the log level is disabled, potentially impacting application performance. SLF4J offers parameterized logging, which defers string formatting until the log message is actually needed. This approach can significantly improve efficiency, especially in high-throughput scenarios or when certain log levels are frequently disabled. Learn more - https://cwe.mitre.org/data/definitions/1176.html
logger.atInfo().kv(RUN_WITH_TOPIC + "." + RUN_WITH_DEFAULT_POSIX_USER, | ||
runWithDefault.get(RUN_WITH_DEFAULT_POSIX_USER)) | ||
.log(RESTART_REQUIRED_MESSAGE); | ||
runWithDefault.get(RUN_WITH_DEFAULT_POSIX_USER)).log(RESTART_REQUIRED_MESSAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Untrusted input is passed to a method that executes arbitrary code, creating a code injection vulnerability. This allows attackers to execute malicious code within the application, potentially leading to data theft or system compromise. Validate input using allowlists with Arrays.asList().contains()
, use regex matching with String.matches()
, or parse input safely with Integer.parseInt()
or Boolean.parseBoolean()
. Learn more
logger.atInfo().kv(RUN_WITH_TOPIC + "." + RUN_WITH_DEFAULT_POSIX_SHELL, | ||
runWithDefault.get(RUN_WITH_DEFAULT_POSIX_SHELL)) | ||
.log(RESTART_REQUIRED_MESSAGE); | ||
runWithDefault.get(RUN_WITH_DEFAULT_POSIX_SHELL)).log(RESTART_REQUIRED_MESSAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Untrusted input is passed to a method that executes arbitrary code, creating a code injection vulnerability. This allows attackers to execute malicious code within the application, potentially leading to data theft or system compromise. Validate input using allowlists with Arrays.asList().contains()
, use regex matching with String.matches()
, or parse input safely with Integer.parseInt()
or Boolean.parseBoolean()
. Learn more
return param.startsWith(FILE_URL_PREFIX) ? new String(Files.readAllBytes(Paths.get(new URI(param))), | ||
StandardCharsets.UTF_8) : param; | ||
return param.startsWith(FILE_URL_PREFIX) | ||
? new String(Files.readAllBytes(Paths.get(new URI(param))), StandardCharsets.UTF_8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Server-Side Request Forgery (SSRF) vulnerability detected. User-controlled input is being used to construct HTTP requests without proper validation, which could allow attackers to make requests to internal services or arbitrary external URLs. Validate and sanitize all user inputs before using them in HTTP requests, implement allowlists for permitted destinations, and avoid direct user input in URL construction. Learn more: https://owasp.org/www-community/attacks/Server_Side_Request_Forgery.
for (Topics topics : sortedByTimestamp) { | ||
boolean allConsumersUpdated = consumers.stream() | ||
.allMatch(consumer -> consumer.apply(topics.toPOJO())); | ||
boolean allConsumersUpdated = consumers.stream().allMatch(consumer -> consumer.apply(topics.toPOJO())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
When the stream is empty the Stream::allMatch operator in the Java Stream API, returns true by default, as there are no elements to violate the predicate. Which can lead to wrong results and potential security vulnerabilities. Therefore always verify that the stream contains elements by using conditions like !nameOfStream.isEmpty(), !nameOfStream.noneMatch(x -> true), !nameOfStream.findAny().isEmpty() to avoid unintended results or consider alternative approaches which will explicitly handle empty streams to ensure robust and secure code. Learn More https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/Stream.html
} | ||
|
||
private static TrustManager[] createTrustManagers(String rootCAPath) throws TLSAuthException { | ||
try { | ||
List<X509Certificate> trustCertificates = EncryptionUtils.loadX509Certificates(Paths.get(rootCAPath)); | ||
List<X509Certificate> trustCertificates = | ||
EncryptionUtils.loadX509Certificates(Paths.get(rootCAPath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Path traversal vulnerability detected. User-controlled input in file paths
can allow attackers to access files outside intended directories using
../
sequences. Secure your code by using framework functions like
getCanonicalPath()
, normalize()
, or validating paths with
.startsWith()
checks. Learn more:
https://cwe.mitre.org/data/definitions/22.html
private void interpolateServiceTemplate( | ||
Path src, Path dst, KernelAlternatives kernelAlternatives) throws IOException { | ||
try (BufferedReader r = Files.newBufferedReader(src); | ||
BufferedWriter w = Files.newBufferedWriter(dst)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Path traversal vulnerability detected. User-controlled input in file paths
can allow attackers to access files outside intended directories using
../
sequences. Secure your code by using framework functions like
getCanonicalPath()
, normalize()
, or validating paths with
.startsWith()
checks. Learn more:
https://cwe.mitre.org/data/definitions/22.html
@@ -158,7 +160,8 @@ public int size() { | |||
* @param map map to merge | |||
*/ | |||
public void mergeMap(long timestamp, Map<String, Object> map) { | |||
this.updateMap(map, new UpdateBehaviorTree(UpdateBehaviorTree.UpdateBehavior.MERGE, timestamp)); | |||
this.updateMap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential SQL Injection detected. Untrusted input is being directly included in an SQL query without proper parameterization. This can allow attackers to modify the query structure and execute arbitrary SQL commands. Use PreparedStatement with parameterized queries instead. Always validate and sanitize inputs before using them in queries. Learn more https://cwe.mitre.org/data/definitions/89.html
format-with-existing-config.sh
Outdated
# Colors for output | ||
RED='\033[0;31m' | ||
GREEN='\033[0;32m' | ||
YELLOW='\033[1;33m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
YELLOW appears unused. Verify use (or export if used externally).
6df1535
to
95f31be
Compare
subscribed = iotJobsClientWrapper | ||
.SubscribeToDescribeJobExecutionAccepted(describeJobExecutionSubscriptionRequest, | ||
QualityOfService.AT_LEAST_ONCE, consumerAccept); | ||
subscribed = iotJobsClientWrapper.SubscribeToDescribeJobExecutionAccepted( |
Check notice
Code scanning / CodeQL
Javadoc has impossible 'throws' tag Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To resolve the issue, the @throws TimeoutException
Javadoc tag needs to be removed from the Javadoc comment above subscribeToGetNextJobDescription
in IotJobsHelper.java
. No changes are necessary to the method signature, since TimeoutException
is not thrown and only InterruptedException
is declared. Only the documentation should be updated. The edit should be applied precisely to the lines spanning the Javadoc for this method.
@@ -524,7 +524,6 @@ | ||
* @param consumerReject Consumer for when the job is rejected | ||
* @throws ExecutionException if subscribing fails | ||
* @throws InterruptedException if the thread gets interrupted | ||
* @throws TimeoutException if the operation does not complete within the given time | ||
*/ | ||
@SuppressWarnings("PMD.AvoidCatchingThrowable") | ||
protected void subscribeToGetNextJobDescription(Consumer<DescribeJobExecutionResponse> consumerAccept, |
8895556
to
eb264b2
Compare
GreengrassService, ServiceLoadException> locateFunction) throws ServiceLoadException { | ||
@SuppressWarnings({"UseSpecificCatch", "PMD.AvoidCatchingThrowable", "PMD.AvoidDeeplyNestedIfStmts", | ||
"PMD.ConfusingTernary"}) | ||
private GreengrassService createGreengrassServiceInstance(Context.Value v, String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The cyclomatic complexity of this method is 24. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.
We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 535-588 into a separate method.
case ENV_STAGE_ARG_SHORT: | ||
kernelArgs.add(arg); | ||
this.environmentStage = getArg(); | ||
kernelArgs.add(environmentStage.toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
toLowerCase
The parameterless versions of String.toLowerCase()
and String.toUpperCase()
use the default locale of the JVM when transforming strings. This can have unintended results, including difficult-to-debug transient failures because case mapping differs based on locale.
Even if you aren’t writing code for a user experience that’s displayed to a customer, system internal code often uses case folding to normalize strings for comparison.
Not doing so can missing locale lead to errors when running on systems that are configured to use a different system locale. This can include developer desktops or build servers (not just your fleet hosts): many hours have been spent debugging errors that only occur on a specific machine due to its locale configuration.
Recommended solutions:
Always pass in a Locale
.
Language or locale-dependent processing:
Get the locale from the platform, use the language of the content, or configure it in your code and pass that to the method.
String firstName = "Ichabod";
Locale locale = // Where you obtain the locale from is platform dependent
String lowerCaseFirstName = firstName.toLowerCase(locale);
Internal processing that isn’t locale-dependent:
Example: S3 bucket name
Always specify the language/country-neutral locale Locale.ROOT
.
String imgTag = "IMG";
if ("img".equals(imgTag.toLowerCase(Locale.ROOT)) {
// do something
}
String rcString = SubAckPacket.SubAckReasonCode.UNSPECIFIED_ERROR.name(); | ||
try { | ||
rcString = SubAckPacket.SubAckReasonCode.getEnumValueFromInteger(v.getReasonCode()).name(); | ||
} catch (RuntimeException ignored) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Swallowing exceptions without logging can make it difficult to identify and troubleshoot issues, potentially leading to application instability, hidden bugs, or unexpected behavior. To avoid this, ensure that all exceptions are logged using appropriate logging levels. When logging exceptions, include the exception object or its stack trace to capture detailed information. For critical exceptions, rethrow them when necessary to provide sufficient context for effective troubleshooting.
Suggested remediation:
Handle all exceptions by creating a log for each exception using the message thrown.
@@ -520,2 +520,3 @@
} catch (RuntimeException ignored) {
+ log.error("RuntimeException is caught.", ignored);
}
* 4583, "TS": 1600127641506 }, "contexts": {}, "loggerName": "Metrics-SystemMetrics", | ||
* "timestamp": 1600127641506, "cause": null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 91.
* 1600127641506 }, "contexts": {}, "loggerName": "Metrics-SystemMetrics", "timestamp": | ||
* 1600127641506, "cause": null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 178.
} else { | ||
FileDescriptor stdinFd = new FileDescriptor(); | ||
setHandle(stdinFd, stdHandles[0]); | ||
stdinStream = new BufferedOutputStream(new FileOutputStream(stdinFd)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
Similar issue at line numbers 525 and 533.
eb264b2
to
3a3cdab
Compare
src/test/java/com/aws/greengrass/easysetup/DeviceProvisioningHelperTest.java
Fixed
Show fixed
Hide fixed
src/test/java/com/aws/greengrass/easysetup/DeviceProvisioningHelperTest.java
Fixed
Show fixed
Hide fixed
@SuppressWarnings({ | ||
"UseSpecificCatch", "PMD.AvoidCatchingThrowable", "PMD.AvoidDeeplyNestedIfStmts", "PMD.ConfusingTernary" | ||
}) | ||
private GreengrassService createGreengrassServiceInstance(Context.Value v, String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The cyclomatic complexity of this method is 24. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.
We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 536-589 into a separate method.
String rcString = SubAckPacket.SubAckReasonCode.UNSPECIFIED_ERROR.name(); | ||
try { | ||
rcString = SubAckPacket.SubAckReasonCode.getEnumValueFromInteger(v.getReasonCode()).name(); | ||
} catch (RuntimeException ignored) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Swallowing exceptions without logging can make it difficult to identify and troubleshoot issues, potentially leading to application instability, hidden bugs, or unexpected behavior. To avoid this, ensure that all exceptions are logged using appropriate logging levels. When logging exceptions, include the exception object or its stack trace to capture detailed information. For critical exceptions, rethrow them when necessary to provide sufficient context for effective troubleshooting.
Suggested remediation:
Handle all exceptions by creating a log for each exception using the message thrown.
@@ -522,2 +522,3 @@
} catch (RuntimeException ignored) {
+ log.error("RuntimeException is caught.", ignored);
}
* 1600127641506 }, "contexts": {}, "loggerName": "Metrics-SystemMetrics", "timestamp": | ||
* 1600127641506, "cause": null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 179.
} else { | ||
FileDescriptor stdinFd = new FileDescriptor(); | ||
setHandle(stdinFd, stdHandles[0]); | ||
stdinStream = new BufferedOutputStream(new FileOutputStream(stdinFd)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The code is not properly closing resources, which can lead to resource leaks. Unclosed resources may cause memory leaks or other system resource issues, potentially degrading application performance over time. If an exception occurs before the explicit close() call, the resource will not be closed, increasing the risk of leaks. To remediate this, use the try-with-resources statement to automatically close AutoCloseable resources, ensuring proper resource management even when exceptions occur.
Similar issue at line numbers 536 and 544.
3a3cdab
to
22a7337
Compare
22a7337
to
b15e9e6
Compare
.httpClientBuilder(ProxyUtils.getSdkHttpClientBuilder()) | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()).build(); | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Builder.retryPolicy
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the problem, move the retry configuration out of ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()
and instead supply the RetryPolicy
directly to the IamClient.builder().retryPolicy(retryPolicy)
as per AWS SDK v2.x best practices. Remove the .overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build())
line entirely or use .overrideConfiguration(ClientOverrideConfiguration.create())
if an override configuration is still required for other reasons, but in this snippet, it appears the only reason for using ClientOverrideConfiguration
is to configure retries.
Therefore:
- Remove
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build())
from the builder chain in thegetIamClient
method. - Insert
.retryPolicy(retryPolicy)
in theIamClient.builder()
chain, before.build()
. - No external dependencies need to be added, and the change is constrained to the shown method.
-
Copy modified line R54
@@ -51,7 +51,7 @@ | ||
return IamClient.builder() | ||
.region(globalRegionByPartition) | ||
.httpClientBuilder(ProxyUtils.getSdkHttpClientBuilder()) | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()) | ||
.retryPolicy(retryPolicy) | ||
.build(); | ||
} | ||
} |
.httpClientBuilder(ProxyUtils.getSdkHttpClientBuilder()) | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()).build(); | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Builder.retryPolicy
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the problem, we need to avoid setting the retry policy through a deprecated method on ClientOverrideConfiguration.Builder
. Instead, we should set the retryPolicy
directly on the StsClient.builder()
using its non-deprecated retryPolicy
method, as recommended by the AWS SDK v2 documentation. This means:
- Remove
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build())
from the client builder call. - Add
.retryPolicy(retryPolicy)
directly to theStsClient.builder()
chain. - No additional imports or method changes are needed; simply update the builder chain to use the preferred API.
All changes are localized within the getStsClient
method of StsSdkClientFactory.java
.
-
Copy modified line R53
@@ -50,7 +50,7 @@ | ||
return StsClient.builder() | ||
.region(Region.of(awsRegion)) | ||
.httpClientBuilder(ProxyUtils.getSdkHttpClientBuilder()) | ||
.overrideConfiguration(ClientOverrideConfiguration.builder().retryPolicy(retryPolicy).build()) | ||
.retryPolicy(retryPolicy) | ||
.build(); | ||
} | ||
} |
@@ -265,16 +279,17 @@ | |||
|
|||
private Set<Integer> pidsInComponentCgroup(Cgroup cgroup, String component) throws IOException { | |||
return Files.readAllLines(cgroup.getCgroupProcsPath(component)) | |||
.stream().map(Integer::parseInt).collect(Collectors.toSet()); | |||
.stream() | |||
.map(Integer::parseInt) |
Check notice
Code scanning / CodeQL
Missing catch of NumberFormatException Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
The best way to fix this problem is to catch NumberFormatException
when parsing each line as an integer. This prevents a bad line from crashing the operation and lets the code skip malformed values gracefully. This can be done by replacing the direct .map(Integer::parseInt)
with a .flatMap(...)
or similar, which tries to parse each line, includes it in the result if successful, and skips it if not, possibly logging the parse error.
Edit only lines 282-284 in pidsInComponentCgroup
in src/main/java/com/aws/greengrass/util/platforms/unix/linux/LinuxSystemResourceController.java
.
Add a logger statement if a malformed line is skipped—for observability.
No new imports are required, as logger
is already available.
-
Copy modified lines R283-R291
@@ -280,7 +280,15 @@ | ||
private Set<Integer> pidsInComponentCgroup(Cgroup cgroup, String component) throws IOException { | ||
return Files.readAllLines(cgroup.getCgroupProcsPath(component)) | ||
.stream() | ||
.map(Integer::parseInt) | ||
.flatMap(line -> { | ||
try { | ||
return Arrays.stream(new Integer[]{Integer.parseInt(line)}); | ||
} catch (NumberFormatException ex) { | ||
logger.warn("Malformed PID '{}' in {}: {}", line, | ||
cgroup.getCgroupProcsPath(component), ex.getMessage()); | ||
return Arrays.stream(new Integer[0]); | ||
} | ||
}) | ||
.collect(Collectors.toSet()); | ||
} | ||
|
.componentName("MonitoringService") | ||
.componentVersion(new Semver("1.0.0")) | ||
.componentDescription("a monitor service") | ||
.build(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
FileUtils.writeStringToFile
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the deprecated usage of FileUtils.writeStringToFile(File, String)
, replace the call with FileUtils.writeStringToFile(File, String, Charset)
, supplying an explicit encoding, preferably StandardCharsets.UTF_8
since this is already imported and used elsewhere in the code. The change should only be made on line 183 of src/test/java/com/aws/greengrass/componentmanager/ComponentStoreTest.java
, substituting the old call with one specifying the encoding. No additional imports are necessary as StandardCharsets
is already present.
-
Copy modified line R183
@@ -180,7 +180,7 @@ | ||
|
||
assertThat(expectedRecipeFile, not(anExistingFile())); | ||
String oldContent = "old content that will be replaced"; | ||
FileUtils.writeStringToFile(expectedRecipeFile, oldContent); | ||
FileUtils.writeStringToFile(expectedRecipeFile, oldContent, StandardCharsets.UTF_8); | ||
|
||
assertThat(expectedRecipeFile, is(anExistingFile())); | ||
String fileContent = new String(Files.readAllBytes(expectedRecipeFile.toPath())); |
.componentVersion(new Semver("1.0.0")).componentDescription("a monitor service").build(); | ||
.recipeFormatVersion(RecipeFormatVersion.JAN_25_2020) | ||
.componentName("MonitoringService") | ||
.componentVersion(new Semver("1.0.0")) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
FileUtils.writeStringToFile
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the deprecated invocation, update the call on line 221 from FileUtils.writeStringToFile(expectedRecipeFile, recipeString)
to FileUtils.writeStringToFile(expectedRecipeFile, recipeString, StandardCharsets.UTF_8)
. This change makes the encoding explicit (UTF-8), addressing the reason why the previous overload was deprecated. Since StandardCharsets
is already imported, there is no need to add new imports. The update does not change functionality, only prevents encoding ambiguity.
Modify only this line in src/test/java/com/aws/greengrass/componentmanager/ComponentStoreTest.java
, with several lines of pre/post context for clarity.
-
Copy modified line R221
@@ -218,7 +218,7 @@ | ||
File expectedRecipeFile = getExpectedRecipeFile(componentIdentifier); | ||
|
||
assertThat(expectedRecipeFile, not(anExistingFile())); | ||
FileUtils.writeStringToFile(expectedRecipeFile, recipeString); | ||
FileUtils.writeStringToFile(expectedRecipeFile, recipeString, StandardCharsets.UTF_8); | ||
|
||
assertThat(expectedRecipeFile, is(anExistingFile())); | ||
long modifiedTime = expectedRecipeFile.lastModified(); |
.responseBody(AbortableInputStream.create(IOUtils.toInputStream("random"))).build()); | ||
when(request.call()).thenReturn(HttpExecuteResponse.builder() | ||
.response(SdkHttpResponse.builder().statusCode(HTTP_OK).build()) | ||
.responseBody(AbortableInputStream.create(IOUtils.toInputStream("random"))) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
IOUtils.toInputStream
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we should replace the use of the deprecated method IOUtils.toInputStream(String)
on line 365 with a non-deprecated alternative for creating an InputStream from a String. The current recommended approach is to use new ByteArrayInputStream(string.getBytes(StandardCharsets.UTF_8))
, which ensures the string is properly encoded and converted to an input stream. This approach does not require any external dependency beyond standard Java libraries, so only an additional import for java.nio.charset.StandardCharsets
may be needed if it is not already present in the file.
The fix affects only line 365 in src/test/java/com/aws/greengrass/deployment/DeploymentDocumentDownloaderTest.java
. The replacement block should convert "random"
to an InputStream as described, maintaining the encoding consistency.
-
Copy modified line R365
@@ -362,7 +362,7 @@ | ||
|
||
when(request.call()).thenReturn(HttpExecuteResponse.builder() | ||
.response(SdkHttpResponse.builder().statusCode(HTTP_OK).build()) | ||
.responseBody(AbortableInputStream.create(IOUtils.toInputStream("random"))) | ||
.responseBody(AbortableInputStream.create(new java.io.ByteArrayInputStream("random".getBytes(java.nio.charset.StandardCharsets.UTF_8)))) | ||
.build()); | ||
|
||
RetryableDeploymentDocumentDownloadException exception = |
Long id = 1L; | ||
Publish request = Publish.builder().topic("spool") | ||
MqttClient client = spy(new MqttClient(deviceConfiguration, spool, false, (c) -> builder, executorService)); | ||
Long id = 1L; |
Check warning
Code scanning / CodeQL
Boxed variable is never null Warning test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the problem, change the declaration of id
at line 993 from the boxed type Long
to the primitive type long
. Assign the primitive value as before. Use the primitive type throughout that method where id
is used. This does not change functionality, as the variable is not intended to be null
and is only used as an ID value. No additional imports or method changes are necessary, assuming that SpoolMessage.builder().id(...)
and spool.getMessageById(...)
accept either primitive longs or their boxed equivalent (Java will auto-box if needed). All edits are confined to the method shown.
-
Copy modified line R993
@@ -990,7 +990,7 @@ | ||
|
||
// The mqttClient is initiated when connectivity is offline | ||
MqttClient client = spy(new MqttClient(deviceConfiguration, spool, false, (c) -> builder, executorService)); | ||
Long id = 1L; | ||
long id = 1L; | ||
Publish request = Publish.builder() | ||
.topic("spool") | ||
.payload("What's up".getBytes(StandardCharsets.UTF_8)) |
@SuppressWarnings({ | ||
"UseSpecificCatch", "PMD.AvoidCatchingThrowable", "PMD.AvoidDeeplyNestedIfStmts", "PMD.ConfusingTernary" | ||
}) | ||
private GreengrassService createGreengrassServiceInstance(Context.Value v, String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The cyclomatic complexity of this method is 24. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.
We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 541-595 into a separate method.
* 1600127641506 }, "contexts": {}, "loggerName": "Metrics-SystemMetrics", "timestamp": | ||
* 1600127641506, "cause": null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 184.
* 4583, "TS": 1600127641506 }, "contexts": {}, "loggerName": "Metrics-SystemMetrics", | ||
* "timestamp": 1600127641506, "cause": null } | ||
*/ | ||
GreengrassLogMessage egLog = objectMapper.readValue(log, GreengrassLogMessage.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
The ObjectMapper.readValue()
method can throw a JsonProcessingException
, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException
and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions
Similar issue at line number 92.
Binary incompatibility detected for commit b15e9e6. com.aws.greengrass.componentmanager.KernelConfigResolver is binary incompatible and is source incompatible because of METHOD_REMOVED Produced by binaryCompatability.py |
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against b15e9e6 |
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against b15e9e6 |
Issue #, if available:
Description of changes:
Remove codestyle.
Replaced it with spotless which is much more strict.
Following a merge of this PR, we will create one more PR which will ignore this change from git blame so as not to point all the changes to me.
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.