Add support for time-coupled RAO implementation#124
Draft
vbochetRTE wants to merge 14 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces comprehensive time-coupled RAO (Remedial Action Optimization) support across the rao-runner modules. Changes include new API resource classes for time-coupled requests/responses, abstract base classes to share common request fields, message handling infrastructure to route requests by type, service implementations for time-coupled RAO execution, synchronous and asynchronous client libraries, and supporting file I/O and configuration updates. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR introduces substantial new architecture across multiple modules with dense business logic (time-coupled RAO execution, network action application, redispatching), careful coordination of message routing and request delegation, and intricate file I/O operations. While many changes follow repetitive patterns (similar handler/client classes), the interconnected nature of the components and the new Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.java (1)
89-95:⚠️ Potential issue | 🟠 MajorReturn the time-coupled interrupted schema from this shared helper.
This base class now serves both handlers, but it always emits
RaoSuccessResponse. An interrupted time-coupled request will therefore be serialized with the standard success resource instead ofTimeCoupledRaoSuccessResponse, which breaks the new time-coupled response contract on interruption paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.java` around lines 89 - 95, sendRaoInterruptedResponse currently always builds a RaoSuccessResponse which breaks time-coupled contracts; modify sendRaoInterruptedResponse to detect time-coupled requests (e.g., raoRequest instanceof TimeCoupledRaoRequest or raoRequest.isTimeCoupled()) and when true build and send a TimeCoupledRaoSuccessResponse (set id and interrupted flag) instead of RaoSuccessResponse, otherwise keep the existing RaoSuccessResponse; use the same sendRaoResponse(…, replyTo, brokerCorrelationId) call to send the correct response type.
🧹 Nitpick comments (19)
rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.java (1)
147-180: Add coverage for the alternate export-unit branch.This update only locks down the
AMPEREpath. A companion test that exercises the other branch would keep the new unit-selection logic from regressing silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.java` around lines 147 - 180, The test currently only exercises the AMPERE branch (when fileExporter.saveRaoResult is called with Unit.AMPERE); add a companion test (e.g., checkSuccessfulCoreRaoRunWithAlternateUnit) that sets up the same RaoRequest/fixtures but forces the alternate export unit path and stubs/verifies fileExporter.saveRaoResult to expect that alternate Unit constant (instead of Unit.AMPERE) and return a URL; ensure you reference the same helpers used in the existing test (checkSuccessfulCoreRaoRun, fileImporter.importRefProg, fileImporter.importGlsk, fileImporter.importVirtualHubs, fileExporter.saveNetwork, fileExporter.saveRaoResult) so the alternate Unit branch is covered and assertions/verify calls check the alternate Unit was used.rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0230.xiidm (1)
2-2: Align the embeddedcaseDatewith the0230slot.Line 2 still carries a
2026-01-29T14:55:28.969+01:00case date even though this fixture is the0230variant. For time-coupled tests, keeping the internal timestamp consistent with the slot suffix and the paired CRAC makes the scenario much more representative and avoids masking timestamp-association bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0230.xiidm` at line 2, Update the caseDate attribute on the iidm:network element with id "12Nodes_0230" so its timestamp reflects the 0230 slot (i.e., the same date as the fixture but with time 02:30 and matching timezone/precision used across other time-coupled fixtures/paired CRAC files) to keep the internal timestamp consistent with the slot.rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/AbstractRaoRequest.java (1)
39-82: Unchecked casts in builder methods generate compiler warnings.The
(T) thiscasts in the builder methods are unchecked and will generate compiler warnings. Consider adding@SuppressWarnings("unchecked")to the class or methods to acknowledge this is intentional for the fluent builder pattern.♻️ Suggested fix
+ `@SuppressWarnings`("unchecked") public abstract static class AbstractRaoRequestBuilder<T extends AbstractRaoRequestBuilder> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/AbstractRaoRequest.java` around lines 39 - 82, The unchecked casts in AbstractRaoRequestBuilder's fluent methods (withId, withRunId, withRaoParametersFileUrl, withResultsDestination, withTargetEndInstant, withEventPrefix) cause compiler warnings; suppress them by adding `@SuppressWarnings`("unchecked") either to the AbstractRaoRequestBuilder class declaration or to each of these methods so the intentional (T) this casts are acknowledged and warnings are silenced.rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileImporterForTesting.java (1)
24-46: Consider extracting the URL normalization logic to reduce duplication.The same URL normalization pattern is repeated in all four override methods. A private helper would make the code more maintainable.
♻️ Suggested refactor
+ private static String normalizeToFileUrl(String url) { + return url.startsWith("file:") ? url : "file:" + url; + } + `@Override` RaoParameters importRaoParameters(String raoParametersFileUrl) throws FileImporterException { - final String url = raoParametersFileUrl.startsWith("file:") ? raoParametersFileUrl : "file:" + raoParametersFileUrl; - return super.importRaoParameters(url); + return super.importRaoParameters(normalizeToFileUrl(raoParametersFileUrl)); } `@Override` TimeCoupledConstraints importIcsFile(String icsFileUrl) throws FileImporterException { - final String url = icsFileUrl.startsWith("file:") ? icsFileUrl : "file:" + icsFileUrl; - return super.importIcsFile(url); + return super.importIcsFile(normalizeToFileUrl(icsFileUrl)); } `@Override` public Network importNetwork(final String networkFileUrl) throws FileImporterException { - final String url = networkFileUrl.startsWith("file:") ? networkFileUrl : "file:" + networkFileUrl; - return super.importNetwork(url); + return super.importNetwork(normalizeToFileUrl(networkFileUrl)); } `@Override` public Crac importCracWithContext(final String cracFileUrl, final Network network) throws FileImporterException { - final String url = cracFileUrl.startsWith("file:") ? cracFileUrl : "file:" + cracFileUrl; - return super.importCracWithContext(url, network); + return super.importCracWithContext(normalizeToFileUrl(cracFileUrl), network); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileImporterForTesting.java` around lines 24 - 46, Extract the repeated URL normalization into a private helper (e.g., a method named normalizeFileUrl) and replace the inline ternary in importRaoParameters, importIcsFile, importNetwork, and importCracWithContext with calls to that helper; the helper should accept the incoming String (e.g., raoParametersFileUrl, icsFileUrl, networkFileUrl, cracFileUrl) and return the url prefixed with "file:" only when necessary, preserving existing behavior and exceptions when passing the normalized value to the corresponding super.method calls.rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoSuccessResponse.java (1)
49-49: Minor formatting: Extra space beforeboolean.- private boolean interrupted; + private boolean interrupted;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoSuccessResponse.java` at line 49, In TimeCoupledRaoSuccessResponse adjust the field declaration for 'interrupted' to remove the extra space so the declaration reads with a single space between the access modifier and the type (i.e., change "private boolean interrupted;" to "private boolean interrupted;"); locate the field named interrupted in the TimeCoupledRaoSuccessResponse class and fix the spacing to match the project's formatting conventions.rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileImporter.java (1)
79-88: Consider logging instead of printing the creation report.
context.getCreationReport().printCreationReport()writes directly to stdout, which may not be appropriate for a service environment. Consider using a logger to capture the report or making this behavior configurable.♻️ Suggested refactor using logging
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory;+ private static final Logger LOGGER = LoggerFactory.getLogger(FileImporter.class); + public Crac importCracWithContext(final String cracFileUrl, final Network network) throws FileImporterException { try { final CracCreationContext context = Crac.readWithContext(getFileNameFromUrl(cracFileUrl), openUrlStream(cracFileUrl), network); - context.getCreationReport().printCreationReport(); + LOGGER.info("CRAC creation report: {}", context.getCreationReport()); return context.getCrac(); } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileImporter.java` around lines 79 - 88, Replace the direct stdout call in importCracWithContext where context.getCreationReport().printCreationReport() is used: inject or create a Logger in FileImporter (e.g., a private static final Logger) and log the creation report instead of printing it, either by using a provided string/format method on the CreationReport (e.g., toString()/toFormattedString) or by capturing the report output into a String (via a StringWriter/PrintWriter) and calling logger.info/debug with that text; make this behavior configurable if needed (flag or configuration) so importCracWithContext can skip logging in noisy environments.rao-runner-spring-boot-starter/src/test/resources/timeCoupledRaoResponseMessage.json (1)
5-8: Exercise the full response contract in this fixture.The starter tests built from this payload only cover the two URL attributes, so a regression in
computationStartInstant,computationEndInstant, orinterruptedmapping would slip through at the client layer.Proposed fixture update
"attributes": { "networksWithPraFileUrl": "networksWithPraFileUrl", - "raoResultsFileUrl": "raoResultsFileUrl" + "raoResultsFileUrl": "raoResultsFileUrl", + "computationStartInstant": "2025-12-31T12:34:00Z", + "computationEndInstant": "2025-12-31T12:35:00Z", + "interrupted": false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/test/resources/timeCoupledRaoResponseMessage.json` around lines 5 - 8, The fixture only includes the two URL attributes and misses other response-contract fields; update timeCoupledRaoResponseMessage.json to include the full response contract by adding computationStartInstant and computationEndInstant with ISO-8601 timestamps, interrupted as a boolean, plus any other expected top-level fields present in the response model so tests exercise mapping for those fields (keep existing networksWithPraFileUrl and raoResultsFileUrl values intact and add representative values for the new fields).rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.java (1)
44-63: Test method name is misleading.The test method is named
getRaoInputTestbut it actually tests therunRaoworkflow and verifies the response/result. Consider renaming to better reflect the test's purpose.📝 Proposed fix
`@Test` - void getRaoInputTest() throws FileExporterException { + void runTimeCoupledRaoTest() throws FileExporterException { final TimeCoupledRaoRequest simpleRaoRequest = TimeCoupledTestHelper.getValidTimeCoupledRaoRequest("");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.java` around lines 44 - 63, Rename the misleading test method getRaoInputTest in TimeCoupledRaoRunnerService2IT to a name that reflects it exercises the runRao workflow and verifies outputs, e.g. runRao_shouldReturnResponseAndSaveFiles or runRao_returnsExpectedResponseAndSavesResults; update the method declaration (keeping the `@Test` annotation) and any references to the method name so it clearly indicates it calls raoRunnerService.runRao(...) and asserts the AbstractRaoResponse and saved TimeCoupledRaoResult.rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.java (1)
36-46: Consider using the constantRaoRunnerConstants.TIME_COUPLED_ROUTING_KEYinstead of hardcoded string.The test hardcodes
"TIME-COUPLED"but the AI summary indicates a constantRaoRunnerConstants.TIME_COUPLED_ROUTING_KEYexists and is used elsewhere. Using the constant ensures the test stays in sync with the production code.♻️ Proposed fix
Add the import:
import static com.farao_community.farao.rao_runner.api.RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY;Then update the test:
`@Test` void genericLauncherTriggersTimeCoupledHandlerTest() { final MessageProperties messageProperties = new MessageProperties(); - messageProperties.setReceivedRoutingKey("TIME-COUPLED"); + messageProperties.setReceivedRoutingKey(TIME_COUPLED_ROUTING_KEY); final Message message = new Message("Test".getBytes(), messageProperties);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.java` around lines 36 - 46, The test genericLauncherTriggersTimeCoupledHandlerTest currently hardcodes "TIME-COUPLED"; replace that literal with the constant RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY (either via static import of com.farao_community.farao.rao_runner.api.RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY or by referencing RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY directly) so the test remains in sync with production code and avoid brittle string duplication; update the MessageProperties.setReceivedRoutingKey call to use the constant and add the corresponding static import if you prefer the shorter name.rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.java (1)
99-102: Hardcoded JSON string is fragile and hard to maintain.The expected JSON string on line 101 is very long and difficult to read or update. Consider using a multi-line text block or comparing parsed JSON trees instead.
💡 Suggestion
Option 1 - Use a text block for readability:
final String expectedJson = """ {"id":"8","runId":"1234-56-7890",...} """.replace("\n", "").replace(" ", "");Option 2 - Compare parsed JSON trees to be order-independent:
final JsonNode actualNode = objectMapper.readTree(serializedTimeCoupledRaoRequest); final JsonNode expectedNode = objectMapper.readTree(expectedJsonString); Assertions.assertThat(actualNode).isEqualTo(expectedNode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.java` around lines 99 - 102, The test TimeCoupledRaoRequestTest currently asserts against a long hardcoded JSON string (serializedTimeCoupledRaoRequest) which is fragile; replace it by building a readable expected JSON (e.g., a Java text block assigned to expectedJson) and either normalize it (strip whitespace) before comparing, or better parse both sides to JsonNode via objectMapper.readTree and assert equality of the trees (use objectMapper.readTree(serializedTimeCoupledRaoRequest) and objectMapper.readTree(expectedJson) and assertThat(actualNode).isEqualTo(expectedNode)); update the assertion in the test method accordingly to use these variables instead of the inline literal.rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClientTest.java (1)
60-80: Consider adding an explicit assertion on the exception cause.The conversion failure test verifies that a
RuntimeExceptionis thrown, but doesn't verify the underlying cause. This could help catch regressions if the error handling changes.💡 Suggestion
//When & Then final CompletableFuture<AbstractRaoResponse> resultFuture = client.runRaoAsynchronously(raoRequest); - assertThrows(RuntimeException.class, resultFuture::join); + final RuntimeException exception = assertThrows(RuntimeException.class, resultFuture::join); + // Optionally verify the cause type or message + assertNotNull(exception.getCause());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClientTest.java` around lines 60 - 80, Capture the RuntimeException thrown by resultFuture::join and add assertions that its cause is the expected conversion error: in AsynchronousTimeCoupledRaoRunnerClientTest, modify the testRunRaoAsynchronouslyConversionFailure to assign the thrown exception from assertThrows(RuntimeException.class, resultFuture::join) to a variable, then assert that thrown.getCause() is non-null and is the expected conversion exception type or contains the expected conversion message coming from AsynchronousTimeCoupledRaoRunnerClient.runRaoAsynchronously; this ensures you validate the underlying cause rather than only the top-level RuntimeException.rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledTestHelper.java (1)
27-43: Consider using an immutable list since the list is not modified after creation.The
ArrayListis only populated during construction and never modified afterward. UsingList.of()would make the immutability explicit.♻️ Suggested refactor
- final List<TimedInput> timedInputs = new ArrayList<>(); - timedInputs.add(new TimedInput( + final List<TimedInput> timedInputs = List.of( + new TimedInput( OffsetDateTime.parse("2019-01-08T00:30:00+01:00"), getResourceUrl("/timecoupled_rao_inputs/simple_case/initialNetwork_0030.xiidm", filePathPrefix), - getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0030.json", filePathPrefix))); - timedInputs.add(new TimedInput( + getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0030.json", filePathPrefix)), + new TimedInput( OffsetDateTime.parse("2019-01-08T01:30:00+01:00"), getResourceUrl("/timecoupled_rao_inputs/simple_case/initialNetwork_0130.xiidm", filePathPrefix), - getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0130.json", filePathPrefix))); - timedInputs.add(new TimedInput( + getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0130.json", filePathPrefix)), + new TimedInput( OffsetDateTime.parse("2019-01-08T02:30:00+01:00"), getResourceUrl("/timecoupled_rao_inputs/simple_case/initialNetwork_0230.xiidm", filePathPrefix), - getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0230.json", filePathPrefix))); - timedInputs.add(new TimedInput( + getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0230.json", filePathPrefix)), + new TimedInput( OffsetDateTime.parse("2019-01-08T03:30:00+01:00"), getResourceUrl("/timecoupled_rao_inputs/simple_case/initialNetwork_0330.xiidm", filePathPrefix), - getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0330.json", filePathPrefix))); + getResourceUrl("/timecoupled_rao_inputs/simple_case/crac_0330.json", filePathPrefix)) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledTestHelper.java` around lines 27 - 43, The list timedInputs is built once via new ArrayList<>() and add(...) calls but never modified; replace this mutable construction with an immutable factory such as List.of(...) (or List.copyOf(...) if you prefer) by inlining the TimedInput(...) instances into a single List.of(...) call (using the same OffsetDateTime.parse(...) and getResourceUrl(...) arguments), and update the declaration of timedInputs to List<TimedInput> timedInputs = List.of(...); ensure appropriate imports are present.rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/JsonApiConverterTest.java (1)
122-134: Typo in test method name:WhithNullInstantshould beWithNullInstant.The method name contains a typo that affects readability and discoverability.
✏️ Suggested fix
`@Test` - void checkRaoResponseJsonConversionWhithNullInstant() throws IOException { + void checkRaoResponseJsonConversionWithNullInstant() throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/JsonApiConverterTest.java` around lines 122 - 134, Rename the test method checkRaoResponseJsonConversionWhithNullInstant in JsonApiConverterTest to checkRaoResponseJsonConversionWithNullInstant (fix the "Whith"→"With" typo); update the method declaration and any references to that method (e.g., test runners or annotations) so the test name reflects the correct spelling while keeping the body and assertions unchanged.rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.java (1)
47-48: Unused autowired field.
TimeCoupledRaoRunnerServiceis autowired but never used in the test class. Consider removing it to avoid unnecessary Spring context loading overhead.Proposed fix
- `@Autowired` - private TimeCoupledRaoRunnerService timeCoupledRaoRunnerService; `@Autowired` private FileExporter fileExporter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.java` around lines 47 - 48, Remove the unused autowired field TimeCoupledRaoRunnerService from the FileExporterTest class to avoid unnecessary Spring context wiring; locate the field declaration "private TimeCoupledRaoRunnerService timeCoupledRaoRunnerService;" in FileExporterTest and delete it (and its `@Autowired` annotation) so the test no longer injects that service.rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerMessageHandler.java (1)
41-41: Consider logging only the request ID to avoid exposing sensitive URLs.Logging the entire
raoRequestobject viatoString()may expose sensitive file URLs (network, CRAC, ICS files) in logs. Consider logging only the request ID for security.Proposed fix
- LOGGER.info("Time-coupled RAO request received: {}", raoRequest); + LOGGER.info("Time-coupled RAO request received with id: {}", raoRequest.getId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerMessageHandler.java` at line 41, The current log call LOGGER.info("Time-coupled RAO request received: {}", raoRequest) risks leaking sensitive URLs in raoRequest.toString(); change it to log only a non-sensitive identifier: extract and log the request ID (e.g., raoRequest.getRequestId() or raoRequest.requestId) in the TimeCoupledRaoRunnerMessageHandler where LOGGER.info is invoked so the message becomes something like "Time-coupled RAO request received: id={}", and remove or redact any full-object logging of raoRequest to avoid exposing file/URL fields.rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java (1)
52-60: Missing priority parameter in async client.The synchronous
TimeCoupledRaoRunnerClientsupports a priority parameter, but this async client does not. Consider adding priority support for consistency if message prioritization is needed for async requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java` around lines 52 - 60, The async client is missing message priority support: update AsynchronousTimeCoupledRaoRunnerClient.buildMessageProperties() to read the priority from raoRunnerClientProperties.getAmqp() (or add a priority field there if absent) and set it on the MessagePropertiesBuilder (e.g., .setPriority(...)) analogous to the synchronous TimeCoupledRaoRunnerClient implementation so async messages can be prioritized consistently.rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequest.java (1)
27-31: Consider defensive copies for list immutability.The
timedInputslist is stored and returned by reference, allowing external code to modify the internal state. For a request object that should be immutable after construction, consider using defensive copies.Proposed fix
private TimeCoupledRaoRequest(RaoRequestBuilder builder) { super(builder); this.icsFileUrl = builder.icsFileUrl; - this.timedInputs = builder.timedInputs; + this.timedInputs = builder.timedInputs != null ? List.copyOf(builder.timedInputs) : null; }And for the getter:
public List<TimedInput> getTimedInputs() { - return timedInputs; + return timedInputs != null ? List.copyOf(timedInputs) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequest.java` around lines 27 - 31, The TimeCoupledRaoRequest stores and exposes the timedInputs list by reference, so make defensive copies to preserve immutability: in the TimeCoupledRaoRequest(RaoRequestBuilder builder) constructor copy builder.timedInputs into a new list (or Collections.unmodifiableList) assigned to this.timedInputs, and in the getter for timedInputs (e.g., getTimedInputs) return an unmodifiable view or a new copy rather than the internal list; also ensure RaoRequestBuilder populates timedInputs with a copy if it accepts external lists to avoid aliasing.rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClientTest.java (2)
38-38: Same potential NPE for response message resource.Apply the same
Objects.requireNonNull()pattern here.Proposed fix
- Mockito.when(responseMessage.getBody()).thenReturn(getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json").readAllBytes()); + Mockito.when(responseMessage.getBody()).thenReturn(Objects.requireNonNull(getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json"), "Test resource not found").readAllBytes());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClientTest.java` at line 38, The mock setup calling Mockito.when(responseMessage.getBody()).thenReturn(getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json").readAllBytes()) can NPE if the resource is missing; update the test to guard the InputStream with Objects.requireNonNull by obtaining the stream from getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json"), wrapping that call with Objects.requireNonNull(...) (or assign to a local InputStream and requireNonNull it) before calling readAllBytes(), and use that byte[] in the Mockito.when for responseMessage.getBody().
35-35: Potential NullPointerException if test resource is missing.
getClass().getResourceAsStream(...)returnsnullif the resource is not found, which would cause an NPE onreadAllBytes(). Consider usingObjects.requireNonNull()to fail fast with a clear error message.Proposed fix
- final TimeCoupledRaoRequest raoRequest = jsonConverter.fromJsonMessage(getClass().getResourceAsStream("/timeCoupledRaoRequestMessage.json").readAllBytes(), TimeCoupledRaoRequest.class); + final TimeCoupledRaoRequest raoRequest = jsonConverter.fromJsonMessage(Objects.requireNonNull(getClass().getResourceAsStream("/timeCoupledRaoRequestMessage.json"), "Test resource not found").readAllBytes(), TimeCoupledRaoRequest.class);And add the import:
import java.util.Objects;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClientTest.java` at line 35, The test in TimeCoupledRaoRunnerClientTest calls getClass().getResourceAsStream("/timeCoupledRaoRequestMessage.json") and directly invokes readAllBytes(), which will NPE if the resource is missing; wrap the InputStream with Objects.requireNonNull(...) providing a clear message (e.g., "test resource /timeCoupledRaoRequestMessage.json not found") before calling readAllBytes(), and add the import java.util.Objects; this will fail fast with a helpful error and locate the problem when jsonConverter.fromJsonMessage(...) is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Line 53: The build fails because the Maven property farao.dependencies.version
is set to an unresolved snapshot (1.47.0-SNAPSHOT); either publish that snapshot
to your configured snapshot repository before merging or change the property
value in pom.xml (farao.dependencies.version) to a known released version (e.g.,
1.46.2) temporarily so the pipeline can resolve dependencies; if choosing
publication, coordinate with the upstream to deploy the snapshot to your
repository, or if choosing the temporary pin, update the property and verify the
build succeeds.
In `@rao-runner-api/pom.xml`:
- Around line 45-48: The assertj-core dependency is missing its test scope so it
gets packaged as a compile dependency; update the dependency block for
artifactId "assertj-core" (groupId "org.assertj") to include <scope>test</scope>
so AssertJ is only used for tests and not included in the final artifact.
In
`@rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.java`:
- Around line 1-16: The file TimeCoupledRaoRequestTest.java is missing the
Mozilla Public License header; add the same MPL license header used across the
project to the top of the file (above the package declaration) so the file
matches other sources and CI checks; locate the test class
TimeCoupledRaoRequestTest and insert the standard project license comment block
exactly as used in other files in this PR.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.java`:
- Around line 70-75: The checkIsInterrupted method currently builds a
RestTemplate without timeouts; change it to create a RestTemplate from
restTemplateBuilder configured with explicit connect and read timeouts (e.g.,
via
restTemplateBuilder.setConnectTimeout(Duration.ofSeconds(...)).setReadTimeout(Duration.ofSeconds(...))).
Use that configured RestTemplate instance in
AbstractRaoRunnerMessageHandler.checkIsInterrupted when calling getForEntity, so
the listener thread will not block indefinitely on network hangs; pick
conservative timeout values appropriate for your environment.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java`:
- Around line 77-90: The code buffers the entire ZIP into a
ByteArrayOutputStream (outputStreamRaoResult) before calling
minioAdapter.uploadArtifact, which can OOM for large exports; change the
implementation of the ZIP creation to stream to disk (create a temp file via
Files.createTempFile and write the ZipOutputStream to a FileOutputStream) or
stream directly into MinIO if the adapter supports streaming, then upload using
a FileInputStream (or the adapter's streaming API) and delete the temp file in a
finally/block; update the same pattern found around
addNetworkToZip/ZipOutputStream and the corresponding path that uses
NETWORKS_ZIP, minioAdapter.uploadArtifact and minioAdapter.generatePreSignedUrl
(also apply the same fix to the similar block at lines ~131-143).
- Around line 97-115: The ZIP entry name currently uses only filenameNoExt (from
networkFilename) so duplicate basenames across timed inputs cause
ZipOutputStream.putNextEntry to fail; update the entry naming logic in
FileExporter where outputFilename is built (and the ZipEntry
creation/zipOutputStream.putNextEntry call) to append a timestamp or unique
identifier derived from the input's offsetDateTime (or another unique per-input
token) to filenameNoExt (e.g., filenameNoExt + "_" + formattedOffset +
"_afterPRA." + outputExtension), ensuring the timestamp is formatted safely for
filenames and used when constructing the ZipEntry to guarantee uniqueness.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java`:
- Around line 110-116: The map key is derived from crac.getTimestamp() but
should use the request timestamp on TimedInput (TimedInput.timestamp() or
timedInput.ts) to avoid overwriting when CRAC metadata changes; replace the
crac.getTimestamp().orElseThrow() in the timedInputMap.put(...) call with a
normalized request timestamp derived from TimedInput (convert/normalize to the
same Instant/UTC type you use for map keys), ensure you handle missing
timestamps (throw with clear message) and detect/resolve duplicates (e.g., fail
fast or apply a deterministic uniqueness transform) so
RaoInputWithNetworkPaths.build(timedInput.networkFileUrl(),
timedInput.networkFileUrl(), crac).build() is always keyed by the authoritative
normalized TimedInput timestamp.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.java`:
- Around line 129-133: The `@ValueSource` in
saveTimeCoupledNetworkWithNoResultDestinationTest is passing one comma-separated
string instead of three entries; update the annotation to provide separate
string literals (e.g. `@ValueSource`(strings = {"biidm", "jiidm", "xiidm"})) so
the parameterized test actually runs three times for the formats used by
checkTimeCoupledNetworkSaving and keep the test body and method signature
(saveTimeCoupledNetworkWithNoResultDestinationTest and call to
checkTimeCoupledNetworkSaving) unchanged.
- Around line 123-127: The `@ValueSource` on
saveTimeCoupledNetworkWithResultDestinationTest is passing a single
comma-separated string; update the annotation on FileExporterTest so it supplies
three separate string values (e.g. "biidm", "jiidm", "xiidm" without spaces)
instead of one combined string, so the parameterized test invoking
checkTimeCoupledNetworkSaving(timeCoupledRaoRequestWithResultDestination,
"postIcsNetworkPath." + format) runs three times with the correct formats.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.java`:
- Around line 44-45: Update the mock verification calls to use the corrected
field name: replace references to "stantardRaoRunnerMessageHandler" with the
correct "standardRaoRunnerMessageHandler" in the Mockito.verify statements (the
two shown that check times(0) and the similar ones at lines 56-57), so the
verifications reference the actual mock field name used elsewhere in the test
class.
- Around line 22-34: Rename the misspelled test variable
`stantardRaoRunnerMessageHandler` to `standardRaoRunnerMessageHandler` across
the test class (declaration, mock creation in setUp(), and where it is passed
into the GenericRaoRunnerListener constructor) to fix the typo and keep
consistency with the RaoRunnerMessageHandler and
TimeCoupledRaoRunnerMessageHandler symbols used by the GenericRaoRunnerListener
and genericListener setup.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceIT.java`:
- Around line 56-64: The test TimeCoupledRaoRunnerServiceIT mixes an
order-insensitive check (containsExactlyInAnyOrderElementsOf on timestamps) with
an order-dependent loop that assumes the local timestamps list index maps to
file suffixes; fix by either making the first assertion order-sensitive (use
containsExactlyElementsOf on raoInput.getTimestampsToRun() to guarantee matching
order) or, preferably, iterate over the actual timestamps returned by raoInput
(e.g., raoInput.getTimestampsToRun() or
raoInput.getRaoInputs().getDataPerTimestamp().keySet()) and derive the expected
file suffix from each timestamp when asserting
raoInputForTimestamp.getInitialNetworkPath() / getPostIcsImportNetworkPath().
In
`@rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/crac_0230.json`:
- Around line 87-103: Update the display name for the range action with id
"RO_RA_00002_RD" to a distinct, descriptive label that reflects the targeted
element "RO_RA_00002_DDE1AA1_GENERATOR" (e.g., "Generator DDE1AA1 Range Action")
so it no longer duplicates "Power Plant 001"; apply the same renaming pattern to
the corresponding sibling timed CRAC fixtures to keep logs/exports/assertions
unambiguous and ensure the change is made where "name" is set for the object
with id "RO_RA_00002_RD".
In
`@rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0130.xiidm`:
- Line 38: Replace the misspelled property name "nomimalPower" with the correct
"nominalPower" in the IIDM property element (the attribute name on the
<iidm:property> element currently set to name="nomimalPower"); update the XML
attribute to name="nominalPower" and scan for any other occurrences or consumers
that reference "nomimalPower" so they can be updated to the corrected symbol.
In
`@rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0330.xiidm`:
- Line 38: Fix the typo in the IIDM property name by renaming the attribute name
"nomimalPower" to the correct "nominalPower" in the <iidm:property> element (the
same change should be applied where the same typo appears, e.g., in
initialNetwork_0130.xiidm); ensure the attribute spelling matches "nominalPower"
so downstream parsers/readers pick up the value correctly.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java`:
- Around line 40-44: runRaoAsynchronously currently instantiates a new
MDCAwareForkJoinPool for every call (in thenApplyAsync(..., new
MDCAwareForkJoinPool())) which leaks threads; change this to reuse a shared
ForkJoinPool (e.g., a static final MDCAwareForkJoinPool instance or use
ForkJoinPool.commonPool()) and wire its lifecycle so it is not recreated per
invocation (or ensure proper shutdown on application stop), updating
runRaoAsynchronously to pass the shared pool to thenApplyAsync and remove
per-call instantiation.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClient.java`:
- Line 70: The call to
setReceivedRoutingKey(RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY) in
TimeCoupledRaoRunnerClient deviates from the parent RaoRunnerClient and sets a
broker-side property on the sender; either remove this call and rely on the
broker to populate receivedRoutingKey, or if the time-coupled flow truly
requires sender-side marking, replace it with a clear custom header (e.g.,
"X-Time-Coupled-Routing") and document the behavior. Locate the
setReceivedRoutingKey usage in TimeCoupledRaoRunnerClient and either delete it
to match RaoRunnerClient behavior or switch to setting a dedicated header on the
outgoing message and add a short comment explaining why the header is necessary
for time-coupled routing.
---
Outside diff comments:
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.java`:
- Around line 89-95: sendRaoInterruptedResponse currently always builds a
RaoSuccessResponse which breaks time-coupled contracts; modify
sendRaoInterruptedResponse to detect time-coupled requests (e.g., raoRequest
instanceof TimeCoupledRaoRequest or raoRequest.isTimeCoupled()) and when true
build and send a TimeCoupledRaoSuccessResponse (set id and interrupted flag)
instead of RaoSuccessResponse, otherwise keep the existing RaoSuccessResponse;
use the same sendRaoResponse(…, replyTo, brokerCorrelationId) call to send the
correct response type.
---
Nitpick comments:
In
`@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/AbstractRaoRequest.java`:
- Around line 39-82: The unchecked casts in AbstractRaoRequestBuilder's fluent
methods (withId, withRunId, withRaoParametersFileUrl, withResultsDestination,
withTargetEndInstant, withEventPrefix) cause compiler warnings; suppress them by
adding `@SuppressWarnings`("unchecked") either to the AbstractRaoRequestBuilder
class declaration or to each of these methods so the intentional (T) this casts
are acknowledged and warnings are silenced.
In
`@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequest.java`:
- Around line 27-31: The TimeCoupledRaoRequest stores and exposes the
timedInputs list by reference, so make defensive copies to preserve
immutability: in the TimeCoupledRaoRequest(RaoRequestBuilder builder)
constructor copy builder.timedInputs into a new list (or
Collections.unmodifiableList) assigned to this.timedInputs, and in the getter
for timedInputs (e.g., getTimedInputs) return an unmodifiable view or a new copy
rather than the internal list; also ensure RaoRequestBuilder populates
timedInputs with a copy if it accepts external lists to avoid aliasing.
In
`@rao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoSuccessResponse.java`:
- Line 49: In TimeCoupledRaoSuccessResponse adjust the field declaration for
'interrupted' to remove the extra space so the declaration reads with a single
space between the access modifier and the type (i.e., change "private boolean
interrupted;" to "private boolean interrupted;"); locate the field named
interrupted in the TimeCoupledRaoSuccessResponse class and fix the spacing to
match the project's formatting conventions.
In
`@rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/JsonApiConverterTest.java`:
- Around line 122-134: Rename the test method
checkRaoResponseJsonConversionWhithNullInstant in JsonApiConverterTest to
checkRaoResponseJsonConversionWithNullInstant (fix the "Whith"→"With" typo);
update the method declaration and any references to that method (e.g., test
runners or annotations) so the test name reflects the correct spelling while
keeping the body and assertions unchanged.
In
`@rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.java`:
- Around line 99-102: The test TimeCoupledRaoRequestTest currently asserts
against a long hardcoded JSON string (serializedTimeCoupledRaoRequest) which is
fragile; replace it by building a readable expected JSON (e.g., a Java text
block assigned to expectedJson) and either normalize it (strip whitespace)
before comparing, or better parse both sides to JsonNode via
objectMapper.readTree and assert equality of the trees (use
objectMapper.readTree(serializedTimeCoupledRaoRequest) and
objectMapper.readTree(expectedJson) and
assertThat(actualNode).isEqualTo(expectedNode)); update the assertion in the
test method accordingly to use these variables instead of the inline literal.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileImporter.java`:
- Around line 79-88: Replace the direct stdout call in importCracWithContext
where context.getCreationReport().printCreationReport() is used: inject or
create a Logger in FileImporter (e.g., a private static final Logger) and log
the creation report instead of printing it, either by using a provided
string/format method on the CreationReport (e.g., toString()/toFormattedString)
or by capturing the report output into a String (via a StringWriter/PrintWriter)
and calling logger.info/debug with that text; make this behavior configurable if
needed (flag or configuration) so importCracWithContext can skip logging in
noisy environments.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerMessageHandler.java`:
- Line 41: The current log call LOGGER.info("Time-coupled RAO request received:
{}", raoRequest) risks leaking sensitive URLs in raoRequest.toString(); change
it to log only a non-sensitive identifier: extract and log the request ID (e.g.,
raoRequest.getRequestId() or raoRequest.requestId) in the
TimeCoupledRaoRunnerMessageHandler where LOGGER.info is invoked so the message
becomes something like "Time-coupled RAO request received: id={}", and remove or
redact any full-object logging of raoRequest to avoid exposing file/URL fields.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.java`:
- Around line 47-48: Remove the unused autowired field
TimeCoupledRaoRunnerService from the FileExporterTest class to avoid unnecessary
Spring context wiring; locate the field declaration "private
TimeCoupledRaoRunnerService timeCoupledRaoRunnerService;" in FileExporterTest
and delete it (and its `@Autowired` annotation) so the test no longer injects that
service.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileImporterForTesting.java`:
- Around line 24-46: Extract the repeated URL normalization into a private
helper (e.g., a method named normalizeFileUrl) and replace the inline ternary in
importRaoParameters, importIcsFile, importNetwork, and importCracWithContext
with calls to that helper; the helper should accept the incoming String (e.g.,
raoParametersFileUrl, icsFileUrl, networkFileUrl, cracFileUrl) and return the
url prefixed with "file:" only when necessary, preserving existing behavior and
exceptions when passing the normalized value to the corresponding super.method
calls.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.java`:
- Around line 36-46: The test genericLauncherTriggersTimeCoupledHandlerTest
currently hardcodes "TIME-COUPLED"; replace that literal with the constant
RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY (either via static import of
com.farao_community.farao.rao_runner.api.RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY
or by referencing RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY directly) so the
test remains in sync with production code and avoid brittle string duplication;
update the MessageProperties.setReceivedRoutingKey call to use the constant and
add the corresponding static import if you prefer the shorter name.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.java`:
- Around line 147-180: The test currently only exercises the AMPERE branch (when
fileExporter.saveRaoResult is called with Unit.AMPERE); add a companion test
(e.g., checkSuccessfulCoreRaoRunWithAlternateUnit) that sets up the same
RaoRequest/fixtures but forces the alternate export unit path and stubs/verifies
fileExporter.saveRaoResult to expect that alternate Unit constant (instead of
Unit.AMPERE) and return a URL; ensure you reference the same helpers used in the
existing test (checkSuccessfulCoreRaoRun, fileImporter.importRefProg,
fileImporter.importGlsk, fileImporter.importVirtualHubs,
fileExporter.saveNetwork, fileExporter.saveRaoResult) so the alternate Unit
branch is covered and assertions/verify calls check the alternate Unit was used.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.java`:
- Around line 44-63: Rename the misleading test method getRaoInputTest in
TimeCoupledRaoRunnerService2IT to a name that reflects it exercises the runRao
workflow and verifies outputs, e.g. runRao_shouldReturnResponseAndSaveFiles or
runRao_returnsExpectedResponseAndSavesResults; update the method declaration
(keeping the `@Test` annotation) and any references to the method name so it
clearly indicates it calls raoRunnerService.runRao(...) and asserts the
AbstractRaoResponse and saved TimeCoupledRaoResult.
In
`@rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledTestHelper.java`:
- Around line 27-43: The list timedInputs is built once via new ArrayList<>()
and add(...) calls but never modified; replace this mutable construction with an
immutable factory such as List.of(...) (or List.copyOf(...) if you prefer) by
inlining the TimedInput(...) instances into a single List.of(...) call (using
the same OffsetDateTime.parse(...) and getResourceUrl(...) arguments), and
update the declaration of timedInputs to List<TimedInput> timedInputs =
List.of(...); ensure appropriate imports are present.
In
`@rao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0230.xiidm`:
- Line 2: Update the caseDate attribute on the iidm:network element with id
"12Nodes_0230" so its timestamp reflects the 0230 slot (i.e., the same date as
the fixture but with time 02:30 and matching timezone/precision used across
other time-coupled fixtures/paired CRAC files) to keep the internal timestamp
consistent with the slot.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java`:
- Around line 52-60: The async client is missing message priority support:
update AsynchronousTimeCoupledRaoRunnerClient.buildMessageProperties() to read
the priority from raoRunnerClientProperties.getAmqp() (or add a priority field
there if absent) and set it on the MessagePropertiesBuilder (e.g.,
.setPriority(...)) analogous to the synchronous TimeCoupledRaoRunnerClient
implementation so async messages can be prioritized consistently.
In
`@rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClientTest.java`:
- Around line 60-80: Capture the RuntimeException thrown by resultFuture::join
and add assertions that its cause is the expected conversion error: in
AsynchronousTimeCoupledRaoRunnerClientTest, modify the
testRunRaoAsynchronouslyConversionFailure to assign the thrown exception from
assertThrows(RuntimeException.class, resultFuture::join) to a variable, then
assert that thrown.getCause() is non-null and is the expected conversion
exception type or contains the expected conversion message coming from
AsynchronousTimeCoupledRaoRunnerClient.runRaoAsynchronously; this ensures you
validate the underlying cause rather than only the top-level RuntimeException.
In
`@rao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClientTest.java`:
- Line 38: The mock setup calling
Mockito.when(responseMessage.getBody()).thenReturn(getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json").readAllBytes())
can NPE if the resource is missing; update the test to guard the InputStream
with Objects.requireNonNull by obtaining the stream from
getClass().getResourceAsStream("/timeCoupledRaoResponseMessage.json"), wrapping
that call with Objects.requireNonNull(...) (or assign to a local InputStream and
requireNonNull it) before calling readAllBytes(), and use that byte[] in the
Mockito.when for responseMessage.getBody().
- Line 35: The test in TimeCoupledRaoRunnerClientTest calls
getClass().getResourceAsStream("/timeCoupledRaoRequestMessage.json") and
directly invokes readAllBytes(), which will NPE if the resource is missing; wrap
the InputStream with Objects.requireNonNull(...) providing a clear message
(e.g., "test resource /timeCoupledRaoRequestMessage.json not found") before
calling readAllBytes(), and add the import java.util.Objects; this will fail
fast with a helpful error and locate the problem when
jsonConverter.fromJsonMessage(...) is executed.
In
`@rao-runner-spring-boot-starter/src/test/resources/timeCoupledRaoResponseMessage.json`:
- Around line 5-8: The fixture only includes the two URL attributes and misses
other response-contract fields; update timeCoupledRaoResponseMessage.json to
include the full response contract by adding computationStartInstant and
computationEndInstant with ISO-8601 timestamps, interrupted as a boolean, plus
any other expected top-level fields present in the response model so tests
exercise mapping for those fields (keep existing networksWithPraFileUrl and
raoResultsFileUrl values intact and add representative values for the new
fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30828159-e57f-4096-9bfd-68a9df536667
📒 Files selected for processing (61)
pom.xmlrao-runner-api/pom.xmlrao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/JsonApiConverter.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/RaoRunnerConstants.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/AbstractRaoRequest.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/RaoRequest.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequest.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoSuccessResponse.javarao-runner-api/src/main/java/com/farao_community/farao/rao_runner/api/resource/TimedInput.javarao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/JsonApiConverterTest.javarao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.javarao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoSuccessResponseTest.javarao-runner-api/src/test/resources/timeCoupledRaoRequestMessage.jsonrao-runner-api/src/test/resources/timeCoupledRaoRequestMessageEmptyOptionals.jsonrao-runner-api/src/test/resources/timeCoupledRaoResponseMessage.jsonrao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerService.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileImporter.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/GeneralBeans.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListener.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerMessageHandler.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerService.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerMessageHandler.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/configuration/AmqpBeans.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/exceptions/FileExporterException.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/exceptions/FileImporterException.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileImporterForTesting.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileImporterTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerMessageHandlerTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerMessageHandlerTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceIT.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledTestHelper.javarao-runner-app/src/test/resources/rao_inputs/raoParameters.jsonrao-runner-app/src/test/resources/timeCoupledRaoRequestMessage.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/crac_0030.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/crac_0130.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/crac_0230.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/crac_0330.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0030.xiidmrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0130.xiidmrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0230.xiidmrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/initialNetwork_0330.xiidmrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/raoParameters.jsonrao-runner-app/src/test/resources/timecoupled_rao_inputs/simple_case/timeCoupledConstraints.jsonrao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.javarao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/RaoResponseConversionHelper.javarao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/RaoRunnerClient.javarao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/RaoRunnerClientAutoConfiguration.javarao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClient.javarao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClientTest.javarao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/RaoRunnerClientTest.javarao-runner-spring-boot-starter/src/test/java/com/farao_community/farao/rao_runner/starter/TimeCoupledRaoRunnerClientTest.javarao-runner-spring-boot-starter/src/test/resources/timeCoupledRaoRequestMessage.jsonrao-runner-spring-boot-starter/src/test/resources/timeCoupledRaoResponseMessage.json
💤 Files with no reviewable changes (1)
- rao-runner-app/src/test/resources/rao_inputs/raoParameters.json
Comment on lines
+77
to
+90
| final ByteArrayOutputStream outputStreamRaoResult = new ByteArrayOutputStream(); | ||
| try (ZipOutputStream zipOutputStream = new ZipOutputStream(outputStreamRaoResult)) { | ||
| for (final Map.Entry<OffsetDateTime, Network> entry : networksWithPrasMap.entrySet()) { | ||
| final OffsetDateTime offsetDateTime = entry.getKey(); | ||
| final Network network = entry.getValue(); | ||
| // Write network | ||
| addNetworkToZip(network, zipOutputStream, raoInput, offsetDateTime); | ||
| } | ||
| } catch (final IOException ioe) { | ||
| throw new FileExporterException("Error occurred while trying to export time coupled networks", ioe); | ||
| } | ||
| final String networksWithPraDestinationPath = makeTargetDirectoryPath(raoRequest) + File.separator + NETWORKS_ZIP; | ||
| minioAdapter.uploadArtifact(networksWithPraDestinationPath, new ByteArrayInputStream(outputStreamRaoResult.toByteArray())); | ||
| return minioAdapter.generatePreSignedUrl(networksWithPraDestinationPath); |
There was a problem hiding this comment.
Avoid materializing the whole ZIP in heap before uploading.
Both time-coupled export paths buffer the complete archive in a ByteArrayOutputStream and only upload afterward. A few IIDM snapshots or a large multi-period result can make this object very large, which is risky in a service that's already memory-sensitive. Stream the ZIP to MinIO directly, or spill to a temp file first.
Also applies to: 131-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java`
around lines 77 - 90, The code buffers the entire ZIP into a
ByteArrayOutputStream (outputStreamRaoResult) before calling
minioAdapter.uploadArtifact, which can OOM for large exports; change the
implementation of the ZIP creation to stream to disk (create a temp file via
Files.createTempFile and write the ZipOutputStream to a FileOutputStream) or
stream directly into MinIO if the adapter supports streaming, then upload using
a FileInputStream (or the adapter's streaming API) and delete the temp file in a
finally/block; update the same pattern found around
addNetworkToZip/ZipOutputStream and the corresponding path that uses
NETWORKS_ZIP, minioAdapter.uploadArtifact and minioAdapter.generatePreSignedUrl
(also apply the same fix to the similar block at lines ~131-143).
vbochetRTE
marked this pull request as draft
March 25, 2026 16:03
vbochetRTE
marked this pull request as ready for review
April 16, 2026 10:00
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerService.java (1)
78-82:⚠️ Potential issue | 🟠 MajorAdd a final fallback catch to preserve request-scoped failure IDs.
runRaoonly catches specific exception types. Any other exception escapes toAbstractRaoRunnerMessageHandler#createFailedResponse(rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/AbstractRaoRunnerMessageHandler.java, Lines 116-119), which replies withid="defaultId". That loses request correlation and can break client-side tracking.Suggested fix
} catch (OpenRaoException ore) { return buildRaoFailureResponse(raoRequest.getId(), "FARAO exception occurred when running rao: " + ore.getMessage()); } catch (FileImporterException fie) { return buildRaoFailureResponse(raoRequest.getId(), "Exception occurred in rao-runner: " + fie.getMessage()); + } catch (Exception e) { + LOGGER.error("Unhandled exception while running RAO for request {}", raoRequest.getId(), e); + return buildRaoFailureResponse(raoRequest.getId(), "Unhandled exception: " + e.getMessage()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerService.java` around lines 78 - 82, runRao currently only catches OpenRaoException and FileImporterException so any other exception bubbles up and triggers AbstractRaoRunnerMessageHandler#createFailedResponse which returns a generic id="defaultId"; add a final broad catch (Exception e) in runRao to preserve the original request id by calling buildRaoFailureResponse(raoRequest.getId(), "Unexpected error: " + e.getMessage()) (or similar) so all failures use the request-scoped id and include the error message; locate the runRao method and append the catch(Exception) after the existing catches and ensure it returns the buildRaoFailureResponse result.
♻️ Duplicate comments (4)
rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java (2)
74-89:⚠️ Potential issue | 🟠 MajorAvoid buffering the full ZIP in heap before upload.
Both time-coupled export paths still build the entire archive in a
ByteArrayOutputStreambefore calling MinIO. A large multi-period run can make these blobs big enough to pressure the worker heap or OOM the process. Stream to a temp file or directly to the adapter instead.Also applies to: 133-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java` around lines 74 - 89, The method saveNetworks builds the entire ZIP in a ByteArrayOutputStream causing OOM risk; change it to stream the ZipOutputStream directly to a destination stream instead of buffering in memory—either create a FileOutputStream to a temporary file and pass that File to minioAdapter.uploadArtifact or modify minioAdapter to accept an InputStream/OutputStream so you can write the zip directly via ZipOutputStream (refer to saveNetworks, addNetworkToZip and minioAdapter.uploadArtifact/generatePreSignedUrl); ensure proper try-with-resources and delete the temp file after successful upload and handle IOExceptions by wrapping them in FileExporterException as before.
114-116:⚠️ Potential issue | 🟠 MajorMake each ZIP entry name period-specific.
outputFilenamestill uses only the source basename, so two timed inputs like.../00/network.xiidmand.../01/network.xiidmboth becomenetwork_afterPRA.xiidm. The secondputNextEntry(...)will fail after a successful computation. Include the timestamp, or another unique per-period token, in the entry name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java` around lines 114 - 116, The ZIP entry name uses only filenameNoExt so entries from different periods collide; update the code that builds outputFilename in FileExporter to include a unique per-period token (e.g., timestamp or periodId) extracted from the input path/metadata (or pass-through the original period folder name) before creating new ZipEntry and calling putNextEntry; specifically change the outputFilename construction (currently using filenameNoExt and outputExtension) to append the period token (e.g., filenameNoExt + "_" + periodToken + "_afterPRA." + outputExtension) so each ZipEntry is unique.rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java (1)
114-115:⚠️ Potential issue | 🟠 MajorUse
TimedInput.timestamp()as the authoritative temporal key.Re-keying with
crac.getTimestamp()can normalize offsets, overwrite periods, and then breakFileExporter.addNetworkToZip(...), which matches exported networks back toTimedInput.timestamp(). The request timestamp should stay authoritative here, with explicit normalization and duplicate handling if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java` around lines 114 - 115, Replace the current use of crac.getTimestamp() as the map key with the authoritative TimedInput.timestamp() value: when putting into timedInputMap use the TimedInput produced by RaoInput.build(network, crac).build() to obtain its timestamp (TimedInput.timestamp()) and use that as the key, applying any needed normalization (e.g., to UTC/instant) and explicit duplicate handling (e.g., check containsKey() and decide overwrite/merge/log) before calling timedInputMap.put(...); this keeps keys consistent with FileExporter.addNetworkToZip(...) and prevents offset-normalization/overwrite bugs.rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java (1)
41-47:⚠️ Potential issue | 🔴 CriticalThis still shuts down the executor before the async conversion can use it.
Same issue as the earlier review:
forkJoinPoolis closed when the method returns, but thethenApplyAsync(...)stage runs later. That can turn valid replies into failed futures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java` around lines 41 - 47, The code closes MDCAwareForkJoinPool via try-with-resources before the asynchronous thenApplyAsync stage runs, causing the executor to be shut down while RaoResponseConversionHelper.convertTimeCoupledRaoResponse (used in thenApplyAsync) may still need it; fix by removing the try-with-resources and either reuse a long-lived/shared MDCAwareForkJoinPool instance (e.g., a field/bean) or switch to a synchronous thenApply or use ForkJoinPool.commonPool() for async execution so that the executor isn't closed while asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(...) is still pending; update usages around MDCAwareForkJoinPool, asyncAmqpTemplate.sendAndReceive, thenApplyAsync, and jsonConverter accordingly.
🧹 Nitpick comments (1)
rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java (1)
105-108: Preserve the chronological order you just computed.
sortedTimedInputsis ordered, buttimedInputMapis aHashMap, so that ordering is discarded before buildingTimeCoupledRaoInput. If downstream iteration order matters here, useLinkedHashMaporTreeMap; otherwise the sort can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java` around lines 105 - 108, timedInputMap is created as a HashMap which discards the chronological order computed into sortedTimedInputs; to preserve that order when building TimeCoupledRaoInput, change timedInputMap to a LinkedHashMap (or TreeMap if you want natural key ordering by OffsetDateTime) so insertion/chronological order is maintained when iterating, and ensure you populate it by iterating sortedTimedInputs; alternatively, if downstream ordering does not matter, remove the sorting step that produces sortedTimedInputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousRaoRunnerClient.java`:
- Around line 40-46: The MDCAwareForkJoinPool created in the try-with-resources
is closed before the thenApplyAsync continuation runs, causing sporadic
failures; instead, do not use try-with-resources—create the MDCAwareForkJoinPool
(used as the Executor in the thenApplyAsync call) without auto-closing and
ensure it is shut down after the returned CompletableFuture completes (for
example, call forkJoinPool.shutdown()/shutdownNow() in a whenComplete or handle
callback on the future returned by
asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(..., forkJoinPool)). Locate
the MDCAwareForkJoinPool instantiation and the
asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(...) chain in
AsynchronousRaoRunnerClient (e.g., runRaoAsynchronously) and implement the
deferred shutdown logic so the pool remains alive until continuations finish.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java`:
- Around line 56-64: The buildMessageProperties() method currently sets
receivedRoutingKey via MessagePropertiesBuilder.setReceivedRoutingKey—which is
ineffective on the producer side; remove that call and instead ensure the
TIME_COUPLED tag is conveyed as the actual routing key when sending (use
RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY as the routingKey parameter in
sendAndReceive) or place the tag in a custom message header the consumer reads;
update the send sites (where sendAndReceive is invoked) to pass
RaoRunnerConstants.TIME_COUPLED_ROUTING_KEY and remove setReceivedRoutingKey
from buildMessageProperties (also apply the same change for
TimeCoupledRaoRunnerClient.java) so GenericRaoRunnerListener will see the
correct routing key/header at the consumer.
---
Outside diff comments:
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerService.java`:
- Around line 78-82: runRao currently only catches OpenRaoException and
FileImporterException so any other exception bubbles up and triggers
AbstractRaoRunnerMessageHandler#createFailedResponse which returns a generic
id="defaultId"; add a final broad catch (Exception e) in runRao to preserve the
original request id by calling buildRaoFailureResponse(raoRequest.getId(),
"Unexpected error: " + e.getMessage()) (or similar) so all failures use the
request-scoped id and include the error message; locate the runRao method and
append the catch(Exception) after the existing catches and ensure it returns the
buildRaoFailureResponse result.
---
Duplicate comments:
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.java`:
- Around line 74-89: The method saveNetworks builds the entire ZIP in a
ByteArrayOutputStream causing OOM risk; change it to stream the ZipOutputStream
directly to a destination stream instead of buffering in memory—either create a
FileOutputStream to a temporary file and pass that File to
minioAdapter.uploadArtifact or modify minioAdapter to accept an
InputStream/OutputStream so you can write the zip directly via ZipOutputStream
(refer to saveNetworks, addNetworkToZip and
minioAdapter.uploadArtifact/generatePreSignedUrl); ensure proper
try-with-resources and delete the temp file after successful upload and handle
IOExceptions by wrapping them in FileExporterException as before.
- Around line 114-116: The ZIP entry name uses only filenameNoExt so entries
from different periods collide; update the code that builds outputFilename in
FileExporter to include a unique per-period token (e.g., timestamp or periodId)
extracted from the input path/metadata (or pass-through the original period
folder name) before creating new ZipEntry and calling putNextEntry; specifically
change the outputFilename construction (currently using filenameNoExt and
outputExtension) to append the period token (e.g., filenameNoExt + "_" +
periodToken + "_afterPRA." + outputExtension) so each ZipEntry is unique.
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java`:
- Around line 114-115: Replace the current use of crac.getTimestamp() as the map
key with the authoritative TimedInput.timestamp() value: when putting into
timedInputMap use the TimedInput produced by RaoInput.build(network,
crac).build() to obtain its timestamp (TimedInput.timestamp()) and use that as
the key, applying any needed normalization (e.g., to UTC/instant) and explicit
duplicate handling (e.g., check containsKey() and decide overwrite/merge/log)
before calling timedInputMap.put(...); this keeps keys consistent with
FileExporter.addNetworkToZip(...) and prevents offset-normalization/overwrite
bugs.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java`:
- Around line 41-47: The code closes MDCAwareForkJoinPool via try-with-resources
before the asynchronous thenApplyAsync stage runs, causing the executor to be
shut down while RaoResponseConversionHelper.convertTimeCoupledRaoResponse (used
in thenApplyAsync) may still need it; fix by removing the try-with-resources and
either reuse a long-lived/shared MDCAwareForkJoinPool instance (e.g., a
field/bean) or switch to a synchronous thenApply or use
ForkJoinPool.commonPool() for async execution so that the executor isn't closed
while asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(...) is still
pending; update usages around MDCAwareForkJoinPool,
asyncAmqpTemplate.sendAndReceive, thenApplyAsync, and jsonConverter accordingly.
---
Nitpick comments:
In
`@rao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.java`:
- Around line 105-108: timedInputMap is created as a HashMap which discards the
chronological order computed into sortedTimedInputs; to preserve that order when
building TimeCoupledRaoInput, change timedInputMap to a LinkedHashMap (or
TreeMap if you want natural key ordering by OffsetDateTime) so
insertion/chronological order is maintained when iterating, and ensure you
populate it by iterating sortedTimedInputs; alternatively, if downstream
ordering does not matter, remove the sorting step that produces
sortedTimedInputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2d96447-8a04-4e3c-9ce5-40db1cf009c8
📒 Files selected for processing (14)
rao-runner-api/pom.xmlrao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/FileExporter.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/RaoRunnerService.javarao-runner-app/src/main/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceIT.javarao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceTest.javarao-runner-spring-boot-starter/pom.xmlrao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousRaoRunnerClient.javarao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousTimeCoupledRaoRunnerClient.java
✅ Files skipped from review due to trivial changes (5)
- rao-runner-api/pom.xml
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/RaoRunnerServiceTest.java
- rao-runner-spring-boot-starter/pom.xml
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/GenericRaoRunnerListenerTest.java
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerService2IT.java
- rao-runner-api/src/test/java/com/farao_community/farao/rao_runner/api/resource/TimeCoupledRaoRequestTest.java
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/TimeCoupledRaoRunnerServiceIT.java
- rao-runner-app/src/test/java/com/farao_community/farao/rao_runner/app/FileExporterTest.java
Comment on lines
+40
to
+46
| try (final MDCAwareForkJoinPool forkJoinPool = new MDCAwareForkJoinPool()) { | ||
| return asyncAmqpTemplate.sendAndReceive(raoRunnerClientProperties.getAmqp().getQueueName(), buildMessage(raoRequest)) | ||
| .thenApplyAsync( | ||
| message -> RaoResponseConversionHelper.convertRaoResponse(message, jsonConverter), | ||
| forkJoinPool | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don’t close the executor before the continuation runs.
thenApplyAsync(..., forkJoinPool) schedules the conversion only after the AMQP future completes, but the try-with-resources closes forkJoinPool as soon as runRaoAsynchronously() returns. That can reject the continuation and make the returned future fail sporadically.
Suggested fix
- try (final MDCAwareForkJoinPool forkJoinPool = new MDCAwareForkJoinPool()) {
- return asyncAmqpTemplate.sendAndReceive(raoRunnerClientProperties.getAmqp().getQueueName(), buildMessage(raoRequest))
- .thenApplyAsync(
- message -> RaoResponseConversionHelper.convertRaoResponse(message, jsonConverter),
- forkJoinPool
- );
- }
+ return asyncAmqpTemplate.sendAndReceive(raoRunnerClientProperties.getAmqp().getQueueName(), buildMessage(raoRequest))
+ .thenApply(message -> RaoResponseConversionHelper.convertRaoResponse(message, jsonConverter));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try (final MDCAwareForkJoinPool forkJoinPool = new MDCAwareForkJoinPool()) { | |
| return asyncAmqpTemplate.sendAndReceive(raoRunnerClientProperties.getAmqp().getQueueName(), buildMessage(raoRequest)) | |
| .thenApplyAsync( | |
| message -> RaoResponseConversionHelper.convertRaoResponse(message, jsonConverter), | |
| forkJoinPool | |
| ); | |
| } | |
| return asyncAmqpTemplate.sendAndReceive(raoRunnerClientProperties.getAmqp().getQueueName(), buildMessage(raoRequest)) | |
| .thenApply(message -> RaoResponseConversionHelper.convertRaoResponse(message, jsonConverter)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rao-runner-spring-boot-starter/src/main/java/com/farao_community/farao/rao_runner/starter/AsynchronousRaoRunnerClient.java`
around lines 40 - 46, The MDCAwareForkJoinPool created in the try-with-resources
is closed before the thenApplyAsync continuation runs, causing sporadic
failures; instead, do not use try-with-resources—create the MDCAwareForkJoinPool
(used as the Executor in the thenApplyAsync call) without auto-closing and
ensure it is shut down after the returned CompletableFuture completes (for
example, call forkJoinPool.shutdown()/shutdownNow() in a whenComplete or handle
callback on the future returned by
asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(..., forkJoinPool)). Locate
the MDCAwareForkJoinPool instantiation and the
asyncAmqpTemplate.sendAndReceive(...).thenApplyAsync(...) chain in
AsynchronousRaoRunnerClient (e.g., runRaoAsynchronously) and implement the
deferred shutdown logic so the pool remains alive until continuations finish.
vbochetRTE
marked this pull request as draft
April 16, 2026 10:07
| private final UrlConfiguration urlConfiguration; | ||
| public abstract class AbstractRaoRunnerMessageHandler<RAO_RUNNER_SERVICE_TYPE> { | ||
| protected static final Logger LOGGER = LoggerFactory.getLogger(AbstractRaoRunnerMessageHandler.class); | ||
| protected static final String APPLICATION_ID = "rao-runner-server"; |
Contributor
There was a problem hiding this comment.
Maybe these three could go in the constants class you created
| private static final String RAO_RESULT = "raoResult.json"; | ||
| private static final String IIDM_EXPORT_FORMAT = "XIIDM"; | ||
| private static final String IIDM_EXTENSION = "xiidm"; | ||
| private static final Properties RAO_RESULT_EXPORT_PROPERTIES = new Properties(); |
Contributor
There was a problem hiding this comment.
Some of these could also go in the constants class (probably just XIIDM)
Contributor
Author
There was a problem hiding this comment.
From my point of view, the RaoRunnerConstants class is useful for constants that are shared across several classes (and even more when classes are in different modules). But here the "XIIDM" constant is only used in the FileExporter class, therefore it doesn't seem relevant to move it in the Constants class.
| final TimeCoupledRaoRequest raoRequest) throws FileExporterException { | ||
| final ByteArrayOutputStream outputStreamRaoResult = new ByteArrayOutputStream(); | ||
| try (ZipOutputStream zipOutputStream = new ZipOutputStream(outputStreamRaoResult)) { | ||
| for (final Map.Entry<OffsetDateTime, Network> entry : networksWithPrasMap.entrySet()) { |
Contributor
There was a problem hiding this comment.
a Map.forEach would be more readable since you immediately get key/value, but maybe it has to do with exception handling ?
Contributor
Author
There was a problem hiding this comment.
Absolutely. addNetworkToZip throws exceptions that are not instances of RuntimeException and that is why I wrote an old-style for loop here.
| final OffsetDateTime offsetDateTime) throws IOException, FileExporterException { | ||
| final String networkFilePath = raoRequest.getTimedInputs().stream() | ||
| .filter(input -> input.timestamp().isEqual(offsetDateTime)) | ||
| .findFirst().orElseThrow() |
Contributor
There was a problem hiding this comment.
Could be useful for debug to have a specific exception here
| outputExtension = matcher.group("extension"); | ||
| filenameNoExt = matcher.group("filenameNoExt"); | ||
| } else { | ||
| final int lastDotPosition = networkFilename.lastIndexOf("."); |
Contributor
There was a problem hiding this comment.
can't you just do string.split and get the last element ?
Contributor
Author
There was a problem hiding this comment.
No, because if the input filename has no extension, the outputExtension should be an empty string, not the full filename.
| final TimeCoupledRaoInput raoInput, | ||
| final TimeCoupledRaoRequest raoRequest) throws FileExporterException { | ||
| final ByteArrayOutputStream outputStreamRaoResult = new ByteArrayOutputStream(); | ||
| try (ZipOutputStream zipOutputStream = new ZipOutputStream(outputStreamRaoResult)) { |
Contributor
There was a problem hiding this comment.
I would put everything in the try block, but i'm not adamant about it
| return makeTargetDirectoryPath(raoRequest.getResultsDestination(), raoRequest.getId()); | ||
| } | ||
|
|
||
| private String makeTargetDirectoryPath(final TimeCoupledRaoRequest raoRequest) { |
Contributor
There was a problem hiding this comment.
why not have one function taking abstract as parameter
Contributor
Author
There was a problem hiding this comment.
Indeed you're right, I could do it. Maybe I wrote this part of the code before I extracted an abstract... or maybe not and I just forgot that I could use the abstract class here. 🤷♂️
| final String brokerCorrelationId = message.getMessageProperties().getCorrelationId(); | ||
|
|
||
| try { | ||
| final TimeCoupledRaoRequest raoRequest = jsonApiConverter.fromJsonMessage(message.getBody(), TimeCoupledRaoRequest.class); |
Contributor
There was a problem hiding this comment.
looks like some code could be factored in AbstractRaoRunnerMessageHandler
Contributor
Author
There was a problem hiding this comment.
OK, we will loose the precision of the log message but I guess that's an acceptable loss for a much greater gain.
| } | ||
|
|
||
| @Threadable | ||
| public AbstractRaoResponse runRao(final TimeCoupledRaoRequest raoRequest) { |
Contributor
There was a problem hiding this comment.
runTimeCoupledRao ?
| } | ||
| return saveResultsAndCreateRaoResponse(raoRequest, raoInput, raoResult, computationStartInstant); | ||
| } catch (OpenRaoException ore) { | ||
| return buildRaoFailureResponse(raoRequest.getId(), "FARAO exception occurred when running rao: " + ore.getMessage()); |
Contributor
There was a problem hiding this comment.
FARAO ? not RAO ?
Contributor
Author
There was a problem hiding this comment.
Probably a relic from ancient times
| private static Map<OffsetDateTime, Network> applyRemedialActions(final TimeCoupledRaoResult result, final TimeCoupledRaoInput timeCoupledRaoInput) { | ||
| final Map<OffsetDateTime, Network> networksWithPrasMap = new HashMap<>(); | ||
|
|
||
| for (final OffsetDateTime offsetDateTime : result.getTimestamps()) { |
Contributor
There was a problem hiding this comment.
timestamp rather than offsetDateTime ?
| .build(); | ||
| } | ||
|
|
||
| private static Map<OffsetDateTime, Network> applyRemedialActions(final TimeCoupledRaoResult result, final TimeCoupledRaoInput timeCoupledRaoInput) { |
Contributor
There was a problem hiding this comment.
result & input or both with full names
| Mockito.doNothing().when(minioAdapter).uploadArtifact(Mockito.any(), Mockito.any()); | ||
| } | ||
|
|
||
| private void checkNetworkSaving(final String filePath, final RaoRequest raoRequest) { |
Contributor
There was a problem hiding this comment.
assertNetworkSaved ? I find it more explicit
| checkNetworkSaving("base/path/id/networkWithPRA.xiidm", simpleRaoRequest); | ||
| } | ||
|
|
||
| private void checkTimeCoupledNetworkSaving(final String filePath, final TimeCoupledRaoRequest raoRequest) throws FileExporterException, IOException { |
Contributor
Author
There was a problem hiding this comment.
The problem is that, on line 143-144, we would have
Assertions.assertExceptionOfType(...)
.isThrownBy(() -> assertTimeCoupledNetworkSaved(...))which I find pretty horrible since it is not the assertion that throws the exception...
| } | ||
|
|
||
| private MessageProperties buildMessageProperties() { | ||
| return MessagePropertiesBuilder.newInstance() |
Contributor
There was a problem hiding this comment.
raoRunnerClientProperties.getAmqp() coul be extracted in a variable
|
|
||
| private MessageProperties buildMessageProperties(final int priority) { | ||
| return MessagePropertiesBuilder.newInstance() | ||
| .setAppId(raoRunnerClientProperties.getAmqp().getClientAppId()) |
|
|
||
| @Test | ||
| void checkRaoRequestNormalUsage() { | ||
| TimeCoupledRaoSuccessResponse raoResponse = new TimeCoupledRaoSuccessResponse.Builder() |
| for (final OffsetDateTime timestamp : timestamps) { | ||
| final RaoInput raoInputForTimestamp = raoInput.getRaoInputs().getData(timestamp).orElseThrow(); | ||
| Assertions.assertThat(raoInputForTimestamp.getCrac().getTimestamp()).isEqualTo(Optional.of(timestamp)); | ||
| // final int hour = timestamp.getHour(); |
Contributor
There was a problem hiding this comment.
commented code ? do you want to keep this or use softassertions ?
Contributor
Author
There was a problem hiding this comment.
In fact I would like to test that the network contained in rao input is OK but I don't know how I could do it... So for the moment I think I should remove the commented code until somebody has a brilliant idea to solve my problem.
kahyami
reviewed
Apr 17, 2026
| } else { | ||
| final AbstractRaoResponse raoResponse = raoThreadResult.result(); | ||
| businessLogger.info("RAO computation is finished"); | ||
| LOGGER.info("RAO response sent: {}", raoResponse); |
Contributor
There was a problem hiding this comment.
“RAO response sent” is logged before the send call, move it after to reflect reality
| .withRunId("runId") | ||
| .withEventPrefix("prefix") | ||
| .build(); | ||
| final MessageProperties properties = new MessageProperties(); |
Contributor
There was a problem hiding this comment.
Message and MessageProperties creation is duplicated across multiple tests. Consider extracting it into a private method to avoid duplication
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements (please use
'[x]'to check the checkboxes, or submit the PR and then click the checkboxes)Does this PR already have an issue describing the problem ? If so, link to this issue using
'#XXX'and skip the restWhat kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Add support for time-coupled RAO implementation
What is the current behavior? (You can also link to an open issue here)
Currently, gridcapa-rao-runner only supports SearchTreeRao implementation.
What is the new behavior (if this is a feature change)?
In addition to SearchTreeRao, gridcapa-rao-runner also supports Marmot implementation (time-coupled implementation of OpenRAO).
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
This PR shouldn't introduce a breaking change
Other information:
Summary by CodeRabbit
New Features
Chores