Conversation
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a debug mode flag across the execution stack, persists debug metadata to the database, and integrates S3-based export of compressed debug artifacts produced during worker steps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as MonitorController
participant Service as MonitorService
participant Notifier as NotificationService
participant Worker as Worker
participant S3Srv as S3Service
participant DB as Database
participant S3 as S3
Client->>Controller: POST /executeSecurityAnalysis (isDebug=true)
Controller->>Service: executeProcess(caseUuid, userId, config, true)
Service->>DB: persist ProcessExecutionEntity(isDebug=true)
Service->>Notifier: sendProcessRunMessage(caseUuid, config, true, executionId)
Notifier->>Worker: publish ProcessRunMessage(..., isDebug=true)
Worker->>Worker: run steps (checks isDebug)
alt isDebug == true
Worker->>S3Srv: exportCompressedToS3(debugPath, debugFileName, writer)
S3Srv->>S3: upload compressed artifact
end
Worker->>Service: update status (COMPLETED/FAILED)
Service->>DB: setDebugFileLocation(s3Path) when applicable
DB-->>Client: execution complete (with debugFileLocation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/SecurityAnalysisProcess.java (1)
43-50:⚠️ Potential issue | 🔴 CriticalCritical:
applyModificationsStepis listed twice — modifications will be applied twice.This appears to be a copy-paste error. Applying modifications twice will corrupt network state or produce incorrect results if the modifications are not idempotent.
Proposed fix
public List<ProcessStep<SecurityAnalysisConfig>> defineSteps() { return List.of( loadNetworkStep, applyModificationsStep, - applyModificationsStep, runComputationStep ); }
🤖 Fix all issues with AI agents
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java`:
- Around line 24-25: The test fails because ProcessRunMessage record now has a
fourth parameter (boolean isDebug); update the instantiation in
ConsumerServiceTest where ProcessRunMessage<ProcessConfig> runMessage is created
to pass an explicit boolean (e.g., false) as the fourth argument so the
constructor call matches ProcessRunMessage(UUID, UUID, T, boolean) and preserves
backward-compatible deserialization behavior.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 47-48: The controller method in MonitorController currently
declares two `@RequestBody` parameters (SecurityAnalysisConfig
securityAnalysisConfig and boolean isDebug) which is invalid; change the boolean
isDebug parameter to a `@RequestParam` (e.g., `@RequestParam` boolean isDebug or
`@RequestParam`(name="isDebug", required=false, defaultValue="false") boolean
isDebug) so only SecurityAnalysisConfig is read from the request body, and
update MonitorControllerTest to call the controller with the isDebug request
parameter (adjust mocked request/URL or controller invocation to supply the
boolean param) to match the new signature.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java`:
- Around line 58-59: Rename the boolean field isDebug in the
ProcessExecutionEntity class to debug to avoid Lombok-generated asymmetric
accessors; update the field declaration (private boolean debug) and, to preserve
DB column name, add `@Column`(name = "is_debug") on the field, then update any
references/usages (getters, setters, builder/DTO mapping, tests) that relied on
isDebug to use the new debug property or its generated isDebug()/setDebug(...)
methods as appropriate so Jackson/JPA/Lombok produce consistent JavaBean
accessors.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 82-84: The code currently calls
S3PathUtils.toDebugLocation(executionEnvName, ...) whenever execution.isDebug()
is true, which can produce an invalid path if executionEnvName is null and also
overwrites the debugFileLocation on every status update; modify
updateExecutionStatus so that you only compute and set
execution.setDebugFileLocation(...) once when executionEnvName is available
(e.g., on the transition to RUNNING) and/or when
execution.getDebugFileLocation() is null, and add a null-check for
executionEnvName before calling S3PathUtils.toDebugLocation to avoid passing
null.
In
`@monitor-server/src/main/resources/db/changelog/changesets/changelog_20260212T082157Z.xml`:
- Around line 8-11: Update the Liquibase changeset (id "1770884531328-2") that
adds column "is_debug" to table "process_execution" so the column is NOT NULL
and uses the boolean-specific default; replace the current column definition
with one that includes a NOT NULL constraint and uses
defaultValueBoolean="false" (or add an explicit <addNotNullConstraint> for
column "is_debug" after the <addColumn>) to keep the DB schema consistent with
the primitive boolean field in the entity.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`:
- Line 82: The test still calls and stubs monitorService.executeProcess with the
old 3-arg signature; update MonitorServiceTest so both the when(...) stub and
the verify(...) call use the new 4-argument signature
monitorService.executeProcess(UUID, String, ProcessConfig, boolean) by adding a
ProcessConfig matcher (e.g., any(ProcessConfig.class) or
any(SecurityAnalysisConfig.class) if compatible) and the boolean matcher (e.g.,
anyBoolean() or eq(false)) to the when(...) and verify(...) invocations for the
symbols monitorService.executeProcess, SecurityAnalysisConfig/ProcessConfig, and
anyBoolean()/eq(false).
In `@monitor-worker-server/pom.xml`:
- Around line 112-116: The pom currently declares
io.awspring.cloud:spring-cloud-aws-starter-s3 with version property
${spring-cloud-aws-starter-s3} which is set to 3.3.1 (incompatible with Spring
Boot 3.3.3); update that property (or the dependency version) to a 3.2.x release
compatible with Spring Boot 3.3.3 so the dependency block for
spring-cloud-aws-starter-s3 uses a 3.2.x version instead of 3.3.1.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java`:
- Line 48: The DEBUG_FILENAME constant in ApplyModificationsStep is declared as
an instance final field; change its declaration to a class-level constant by
making it private static final (i.e., update the DEBUG_FILENAME field to be
static final) so it is a true compile-time constant and follows Java convention.
- Around line 77-92: The exportUpdatedNetworkToS3 method currently writes the
gzip to a fixed DEBUG_FILENAME in CWD causing races and leaks; change it to
create a temporary gz file (e.g. Files.createTempFile) instead of
Paths.get(DEBUG_FILENAME), write the gzip into that temp, and ensure both the
.xiidm tmp (tmp) and the temp gz are deleted in a finally block (or
try-with-resources plus finally) regardless of success or exceptions from
s3Service.uploadFile or the GZIPOutputStream; keep using
getDebugFilePath(context, DEBUG_FILENAME) and s3Service.uploadFile but perform
upload from the temp gz file and always call Files.deleteIfExists on both tmp
and the temp gz to avoid leaks and races.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/S3Service.java`:
- Around line 37-38: The catch blocks in uploadFile and downloadFile currently
discard the original SdkException by only using e.getMessage(), losing the stack
trace; update both catch clauses to rethrow an IOException that chains the
original SdkException as the cause (e.g., new IOException("Error occurred while
uploading/downloading file to S3", e)) so the original exception and stack trace
are preserved for debugging.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/utils/FileUtils.java`:
- Around line 15-17: The loop that uses fileNames directly is vulnerable to path
traversal/zip-slip; in the FileUtils class sanitize each fileName before using
tempDir.resolve(fileName) and new ZipEntry(fileName): reject or normalize names
containing "..", absolute paths, or path separators that would escape tempDir,
compute Path resolved = tempDir.resolve(fileName).normalize() and ensure
resolved.startsWith(tempDir); only proceed if that check passes, and use a safe
entry name (e.g., fileName's normalized relative path or
Files.getFileName(resolved).toString()) when creating sourceFile,
zos.putNextEntry(new ZipEntry(...)) and when reading the file; otherwise throw
or skip the entry. Ensure you reference tempDir, fileNames, sourceFile, zos and
ZipEntry in the change.
🧹 Nitpick comments (13)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/utils/FileUtils.java (1)
11-11: Add a private constructor to prevent instantiation of this utility class.
FileUtilsis a stateless utility class with only static methods. A private constructor signals intent and prevents accidental instantiation.Proposed fix
public class FileUtils { + private FileUtils() { + // Utility class + } + public static Path createZipFile(Path tempDir, String fileOrNetworkName, Set<String> fileNames) throws IOException {monitor-worker-server/pom.xml (1)
17-17: Property name should follow Maven.versionconvention.The property name
spring-cloud-aws-starter-s3should bespring-cloud-aws-starter-s3.versionto follow the standard Maven convention for version properties. This improves clarity and consistency with the existingnetwork-modification.versionproperty on Line 16.Proposed fix
- <spring-cloud-aws-starter-s3>3.3.1</spring-cloud-aws-starter-s3> + <spring-cloud-aws-starter-s3.version>3.3.1</spring-cloud-aws-starter-s3.version>And update the dependency reference accordingly:
- <version>${spring-cloud-aws-starter-s3}</version> + <version>${spring-cloud-aws-starter-s3.version}</version>monitor-server/src/test/java/org/gridsuite/monitor/server/services/NotificationServiceTest.java (1)
56-68: Consider adding a test case withisDebug = true.The test only verifies the
falsepath. A complementary test withisDebug = truewould ensure the flag is correctly propagated through the message.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/config/S3Configuration.java (1)
27-32: Consider renaming the bean method to follow Java conventions.The
@SuppressWarnings("checkstyle:MethodName")suppression exists solely because the method is namedS3Service(uppercase). Renaming tos3Servicewould eliminate the need for the suppression and follow standard Java method naming. Spring will still wire the bean correctly by type.♻️ Proposed fix
- `@SuppressWarnings`("checkstyle:MethodName") `@Bean` - public S3Service S3Service(S3Client s3Client) { + public S3Service s3Service(S3Client s3Client) { LOGGER.info("Configuring S3Service with bucket: {}", bucketName); return new S3Service(s3Client, bucketName); }monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContext.java (1)
70-72: Naming inconsistency:getExecutionEnvironment()vs parent'sgetExecutionEnvName().Other delegating accessors in this class (e.g.,
getProcessExecutionId()→getExecutionId()) already have slight naming differences, butgetExecutionEnvironment()vsgetExecutionEnvName()diverges more noticeably ("Environment" vs "EnvName"). Consider aligning for discoverability.monitor-commons/src/main/java/org/gridsuite/monitor/commons/utils/S3PathUtils.java (1)
14-27: Consider adding a private constructor instead of suppressing the checkstyle warning.A private constructor is the idiomatic way to prevent instantiation of utility classes and would eliminate the suppression.
♻️ Proposed fix
-@SuppressWarnings("checkstyle:HideUtilityClassConstructor") public class S3PathUtils { + private S3PathUtils() { + // utility class + } + public static final String S3_DELIMITER = "/";monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java (1)
34-47: Missing assertion and coverage for theisDebugflag.
shouldInitializeCorrectlydoesn't assert the value ofisDebug()after construction, and no test exercisesisDebug=true. Consider adding an assertion here and a test case (or parameterize) withtrueto ensure the flag is correctly stored and retrievable.♻️ Suggested addition in shouldInitializeCorrectly
assertThat(processContext.getExecutionEnvName()).isEqualTo(envName); assertThat(processContext.getNetwork()).isNull(); + assertThat(processContext.isDebug()).isFalse();monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/dto/S3InputStreamInfos.java (1)
1-14: Missing copyright header and fields should beprivate.
- This file lacks the MPL copyright header present in all other files in this PR.
- The fields are package-private; prefer
privatefor proper encapsulation (Lombok@Getterwill still generate public accessors).♻️ Proposed fix
+/** + * Copyright (c) 2026, RTE (http://www.rte-france.com) + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ package org.gridsuite.monitor.worker.server.dto; import lombok.Builder; import lombok.Getter; import java.io.InputStream; `@Builder` `@Getter` public class S3InputStreamInfos { - InputStream inputStream; - String fileName; - Long fileLength; + private InputStream inputStream; + private String fileName; + private Long fileLength; }monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
72-100: Test only coversisDebug=false— consider adding atruecase.The
executeProcessCreateExecutionAndSendNotificationtest only exercises the non-debug path. TheisDebug=truepath triggers distinct behavior inMonitorService.updateExecutionStatus(debug file location viaS3PathUtils). A complementary test withisDebug=truewould increase confidence.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java (2)
53-54: Minor: trailing whitespace before semicolon.- private S3Service s3Service ; + private S3Service s3Service;
79-94: No test coverage for the debug export path.The test only exercises the non-debug flow (Mockito defaults
isDebug()tofalse). TheexportUpdatedNetworkToS3method performs file I/O and S3 upload — critical logic worth covering with at least one test verifyings3Service.uploadFileis called whencontext.isDebug()returnstrue.Would you like me to draft a test case for the debug export path?
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java (1)
118-123: Consider assertingisDebugon the execution context.The context verification checks
executionId,caseUuid,config, andexecutionEnvNamebut notisDebug. Adding!context.isDebug()would confirm the flag propagates correctly throughProcessExecutionService.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/S3Service.java (1)
17-27: Unused@Serviceimport — class is bean-managed viaS3Configuration.The
org.springframework.stereotype.Serviceimport on line 4 is present but@Serviceis not applied to the class. This is presumably intentional since the bean is created inS3Configuration, but the import is vestigial.
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/resources/db/changelog/changesets/changelog_20260212T082157Z.xml
Outdated
Show resolved
Hide resolved
...java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java
Outdated
Show resolved
Hide resolved
...java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java
Show resolved
Hide resolved
| } catch (SdkException e) { | ||
| throw new IOException("Error occurred while uploading file to S3: " + e.getMessage()); |
There was a problem hiding this comment.
Original exception is not chained — stack trace will be lost.
Both uploadFile (line 38) and downloadFile (line 55) discard the original SdkException by only appending its message. This loses the root-cause stack trace, making S3 failures hard to diagnose in production.
Proposed fix
- throw new IOException("Error occurred while uploading file to S3: " + e.getMessage());
+ throw new IOException("Error occurred while uploading file to S3: " + e.getMessage(), e);- throw new IOException("Error occurred while downloading file from S3: " + e.getMessage());
+ throw new IOException("Error occurred while downloading file from S3: " + e.getMessage(), e);🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/S3Service.java`
around lines 37 - 38, The catch blocks in uploadFile and downloadFile currently
discard the original SdkException by only using e.getMessage(), losing the stack
trace; update both catch clauses to rethrow an IOException that chains the
original SdkException as the cause (e.g., new IOException("Error occurred while
uploading/downloading file to S3", e)) so the original exception and stack trace
are preserved for debugging.
| for (String fileName : fileNames) { | ||
| Path sourceFile = tempDir.resolve(fileName); | ||
| zos.putNextEntry(new ZipEntry(fileName)); |
There was a problem hiding this comment.
Sanitize fileName to guard against path traversal.
fileName values from the Set<String> are used directly in tempDir.resolve(fileName) (reading arbitrary files) and new ZipEntry(fileName) (producing a zip-slip archive). If the caller ever passes untrusted or user-influenced file names, this is exploitable. Even for internal callers, a defensive check is cheap insurance.
Proposed fix
for (String fileName : fileNames) {
Path sourceFile = tempDir.resolve(fileName);
+ if (!sourceFile.normalize().startsWith(tempDir.normalize())) {
+ throw new IOException("Invalid file name (path traversal detected): " + fileName);
+ }
zos.putNextEntry(new ZipEntry(fileName));🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/utils/FileUtils.java`
around lines 15 - 17, The loop that uses fileNames directly is vulnerable to
path traversal/zip-slip; in the FileUtils class sanitize each fileName before
using tempDir.resolve(fileName) and new ZipEntry(fileName): reject or normalize
names containing "..", absolute paths, or path separators that would escape
tempDir, compute Path resolved = tempDir.resolve(fileName).normalize() and
ensure resolved.startsWith(tempDir); only proceed if that check passes, and use
a safe entry name (e.g., fileName's normalized relative path or
Files.getFileName(resolved).toString()) when creating sourceFile,
zos.putNextEntry(new ZipEntry(...)) and when reading the file; otherwise throw
or skip the entry. Ensure you reference tempDir, fileNames, sourceFile, zos and
ZipEntry in the change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java`:
- Around line 68-74: The debug export in ApplyModificationsStep currently
rethrows a PowsyblException when exportUpdatedNetworkToS3(context) fails, which
aborts the successful step; change the catch block so that IOException is logged
as a warning (include the exception and a clear message) instead of throwing, so
debug export failures don't stop execution when context.isDebug() is true.
Locate the try/catch around exportUpdatedNetworkToS3 in ApplyModificationsStep
and replace the throw new PowsyblException(...) with a warning-level log call
that includes e (stacktrace) and a short message about debug export failure.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/S3Service.java`:
- Around line 48-52: S3InputStreamInfos.builder() currently uses
inputStream.response().metadata().get(METADATA_FILE_NAME) without null checks,
which can produce a null fileName; update the single place building the DTO in
S3Service to defensively validate that metadata value and provide a fallback
(e.g., use the S3 object key from inputStream.response() or a safe default like
an empty string) before calling .fileName(...), and ensure the DTO always
receives a non-null value; reference METADATA_FILE_NAME and the
inputStream.response().metadata().get(...) call when making the change.
🧹 Nitpick comments (1)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java (1)
79-94: No test coverage for the debug export path.The
executeApplyModificationstest only exercises the non-debug flow. The newexportUpdatedNetworkToS3logic (gzip, upload to S3) introduced inApplyModificationsStepis untested. Consider adding a test wherecontext.isDebug()returnstrueto verify the S3 upload interaction and temp-file handling.
| if (context.isDebug()) { | ||
| try { | ||
| exportUpdatedNetworkToS3(context); | ||
| } catch (IOException e) { | ||
| throw new PowsyblException("An error occurred while saving debug file", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Debug export failure aborts the entire step execution.
If the S3 upload fails, the PowsyblException propagates and the step fails — even though the actual modifications were already applied successfully. Consider whether debug export failures should be logged as warnings rather than aborting the step, since debug is a non-essential side effect.
🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java`
around lines 68 - 74, The debug export in ApplyModificationsStep currently
rethrows a PowsyblException when exportUpdatedNetworkToS3(context) fails, which
aborts the successful step; change the catch block so that IOException is logged
as a warning (include the exception and a clear message) instead of throwing, so
debug export failures don't stop execution when context.isDebug() is true.
Locate the try/catch around exportUpdatedNetworkToS3 in ApplyModificationsStep
and replace the throw new PowsyblException(...) with a warning-level log call
that includes e (stacktrace) and a short message about debug export failure.
There was a problem hiding this comment.
I think it's the expected behaviour, since all the point of running on debug mode is to generate those debug files
There was a problem hiding this comment.
@klesaulnier That's a fair point! If a user explicitly enables debug mode, they're expecting those debug artifacts to be generated. Failing fast when the export fails makes sense in that context — it's better to surface the issue immediately rather than silently continue without the debug information. Thank you for clarifying the intent! 👍
| return S3InputStreamInfos.builder() | ||
| .inputStream(inputStream) | ||
| .fileName(inputStream.response().metadata().get(METADATA_FILE_NAME)) | ||
| .fileLength(inputStream.response().contentLength()) | ||
| .build(); |
There was a problem hiding this comment.
metadata().get(METADATA_FILE_NAME) may return null — no defensive check.
If the S3 object was uploaded without the file-name metadata key (e.g., uploaded outside this service), fileName will silently be null in the returned DTO. Consider validating or providing a fallback.
🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/S3Service.java`
around lines 48 - 52, S3InputStreamInfos.builder() currently uses
inputStream.response().metadata().get(METADATA_FILE_NAME) without null checks,
which can produce a null fileName; update the single place building the DTO in
S3Service to defensively validate that metadata value and provide a fallback
(e.g., use the S3 object key from inputStream.response() or a safe default like
an empty string) before calling .fileName(...), and ensure the DTO always
receives a non-null value; reference METADATA_FILE_NAME and the
inputStream.response().metadata().get(...) call when making the change.
There was a problem hiding this comment.
The code was inspired by computation lib
There was a problem hiding this comment.
@klesaulnier, thank you for the clarification. If this implementation matches the computation lib, maintaining consistency with that codebase makes sense. The null-handling behavior would be shared across both libraries in that case.
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
FranckLecuyer
left a comment
FranckLecuyer
left a comment
There was a problem hiding this comment.
Tests: OK
Code review: OK (just a few remarks)
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| try (ZipOutputStream zipOutputStream = new ZipOutputStream(outputStream)) { | ||
| for (String fileS3Key : filesS3Keys) { | ||
| String zipEntryName = fileS3Key.substring(directoryKey.length()); |
There was a problem hiding this comment.
directoryKey.length() + 1 ?
to remove the begining '/' character for the zip entry name in the zip file generated
There was a problem hiding this comment.
nice catch, I cheated in tests by adding "/" to directory, which is not actually the case
| @SpringBootTest | ||
| @TestPropertySource(properties = { | ||
| "spring.autoconfigure.exclude=io.awspring.cloud.autoconfigure.s3.S3AutoConfiguration", | ||
| "spring.autoconfigure.exclude=io.awspring.cloud.autoconfigure.s3.S3AutoConfiguration" |
|
|
||
| @Transactional(readOnly = true) | ||
| public Optional<byte[]> getDebugInfos(UUID executionId) { | ||
| return executionRepository.findById(executionId).map(execution -> { |
There was a problem hiding this comment.
add a check to avoid NPE if exection not found/debug file location is empty?
There was a problem hiding this comment.
if execution is not found, it will return optional.empty with the map()
I agree with the debuglocation empty, adding a unit test to cover this case
| @@ -0,0 +1,74 @@ | |||
| package org.gridsuite.monitor.worker.server.services; | |||
There was a problem hiding this comment.
some new files miss copyrights/licences
| */ | ||
| @Builder | ||
| @Getter | ||
| public class S3InputStreamInfos { |
| private String userId; | ||
|
|
||
| @Column | ||
| private boolean isDebug; |
There was a problem hiding this comment.
do we need this or only debugFileLocation? assuming null = not debug?
| if (completedAt != null) { | ||
| execution.setCompletedAt(completedAt); | ||
| } | ||
| if (execution.isDebug() && executionEnvName != null |
There was a problem hiding this comment.
Not sure why it's on update and not on process execution creation that we set that?
| * @author Kevin Le Saulnier <kevin.le-saulnier at rte-france.com> | ||
| */ | ||
| public class S3RestService { | ||
| public static final String METADATA_FILE_NAME = "file-name"; |
|
|
||
| <properties> | ||
| <liquibase-hibernate-package>org.gridsuite.monitor.server</liquibase-hibernate-package> | ||
| <spring-cloud-aws-starter-s3>3.3.1</spring-cloud-aws-starter-s3> |
There was a problem hiding this comment.
we don't already get that from gridsuite-dependencies?
There was a problem hiding this comment.
not yet, I used the version from case-server
| } | ||
|
|
||
| public String getExecutionEnvironment() { | ||
| return processContext.getExecutionEnvName(); |
There was a problem hiding this comment.
I think you will have issues if you rely on this to construct the S3 path. executionEnvName is a parameter to expose to the users to make it clear which env ran the analysis.
For S3 we usually use the environment prefix defined in the deployment?
|
|
||
| @Test | ||
| void testPathIsCompressedBeforeUploading() throws Exception { | ||
| // --- Inputs --- |
There was a problem hiding this comment.
do we want these comments?
There was a problem hiding this comment.
When initiation and assertions are difficult to read, i think it makes the test easier to read
But I can remove it if it's disturbing
|
|
||
| <properties> | ||
| <network-modification.version>0.61.0</network-modification.version> | ||
| <spring-cloud-aws-starter-s3>3.3.1</spring-cloud-aws-starter-s3> |
There was a problem hiding this comment.
We need it to download debug files from S3
| return outputStream.toByteArray(); | ||
| } | ||
|
|
||
| List<String> getFilesKeysInDirectory(String directoryKey) { |
There was a problem hiding this comment.
does this handle correctly nested folders?
There was a problem hiding this comment.
in s3, there is not any folder, it will return all documents with keys prefixed by directoryKey + "/"
but i think it's easier to understand to name the method this way
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(config)); | ||
|
|
||
| if (isDebug != null) { |
There was a problem hiding this comment.
not necessary?
not sure why we need a parametrized test here
There was a problem hiding this comment.
as discussed, we'll keep it here only
| void executeProcessCreateExecutionAndSendNotification() { | ||
| @ParameterizedTest | ||
| @ValueSource(booleans = {true, false}) | ||
| void executeProcessCreateExecutionAndSendNotification(boolean isDebug) { |
There was a problem hiding this comment.
I'm not convinced of these parametrized test that DO NOT test the debug behavior?
There was a problem hiding this comment.
removed as discussed
| @Test | ||
| void sendProcessRunMessage() { | ||
| notificationService.sendProcessRunMessage(caseUuid, securityAnalysisConfig, executionId); | ||
| @ParameterizedTest |
There was a problem hiding this comment.
instead of parametrized just use the not default no?
There was a problem hiding this comment.
removed as discussed
| * @author Antoine Bouhours <antoine.bouhours at rte-france.com> | ||
| */ | ||
| @SpringBootTest(classes = {MonitorServerApplication.class, TestChannelBinderConfiguration.class}) | ||
| @DisableS3ConfigurationTest |
There was a problem hiding this comment.
what happens if you enable it?
Can't we mock it instead of disabling?
There was a problem hiding this comment.
Replaced with application.yaml test config
|
|
||
| public void exportCompressedToS3(String s3Key, String fileName, ThrowingConsumer<Path> writer) throws IOException { | ||
| FileAttribute<Set<PosixFilePermission>> attrs = | ||
| PosixFilePermissions.asFileAttribute( |
There was a problem hiding this comment.
this is very tricky and very easy to make something wrong that crash the server if tmp files accumulate.
Did you check what's done in case server for temp fileS?
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
3 participants
PR Summary
A param "isDebug" has been added to process execution endpoint to do so