POC : use existing servers to run cyclic chain#47
Open
FranckLecuyer wants to merge 4 commits intomainfrom
Open
POC : use existing servers to run cyclic chain#47FranckLecuyer wants to merge 4 commits intomainfrom
FranckLecuyer wants to merge 4 commits intomainfrom
Conversation
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded end-to-end "security analysis using servers" flow: new DTOs, REST services, messaging consumers, controller endpoint, service orchestration, DB schema and entity changes, and stream binding configuration to run a two-phase process (network modifications → security analysis) and propagate results. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client\n(API caller)"
participant Controller as "MonitorController\n(/execute/security-analysis-using-servers)"
participant MonitorSvc as "MonitorService"
participant Notification as "NotificationService\n(publishes to broker)"
participant Broker as "Message Broker\n(cloud-stream)"
rect rgba(135,206,235,0.5)
participant Consumer as "ConsumerServiceUsingServers\nconsumeRunUsingServers"
end
participant NetRest as "NetworkModificationRestService"
participant NetServer as "Network-Modification-Server"
participant SAstore as "SecurityAnalysisRestService"
participant SAServer as "Security-Analysis-Server"
participant ConsumerNet as "ConsumerServiceUsingServers\nconsumeNetworkModifications"
participant ConsumerSA as "ConsumerServiceUsingServers\nconsumeSecurityAnalysis"
participant MonitorUpd as "Monitor (DB updates)"
Client->>Controller: POST /execute/security-analysis-using-servers(caseUuid, config)
Controller->>MonitorSvc: executeProcessUsingServers(caseUuid,userId,config)
MonitorSvc->>MonitorSvc: create ProcessExecutionEntity (SCHEDULED)
MonitorSvc->>Notification: sendProcessRunMessage(..., bindingName)
Notification->>Broker: publish run message (monitor.process.securityanalysis.using.servers.run)
Broker->>Consumer: deliver ProcessRunMessage
Consumer->>MonitorUpd: set execution status RUNNING, create steps
Consumer->>NetRest: applyModifications(caseUuid, executionId, modificationUuids)
NetRest->>NetServer: POST /v1/cases/{caseUuid}/network-composite-modifications?executionUuid=...
NetServer-->>ConsumerNet: emit CaseResultInfos (network phase result)
ConsumerNet->>MonitorUpd: update network step status, attach resultCaseId
alt network succeeded
ConsumerNet->>SAstore: runSecurityAnalysis(caseUuid, executionId, contingencies, parametersUuid)
SAstore->>SAServer: POST /v1/cases/{caseUuid}/run-and-save?executionUuid=...
SAServer-->>ConsumerSA: emit CaseResultInfos (security analysis result)
ConsumerSA->>MonitorUpd: update security step + overall execution status, persist report/result UUIDs
else network failed
ConsumerNet->>MonitorUpd: mark execution failed and cleanup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 6
🧹 Nitpick comments (4)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java (1)
34-37: Add connect/read timeouts on the external call.Prevents a stuck call from blocking the caller thread indefinitely.
⏱️ Suggested change
+import java.time.Duration; @@ - this.securityAnalysisServerRest = restTemplateBuilder.build(); + this.securityAnalysisServerRest = restTemplateBuilder + .setConnectTimeout(Duration.ofSeconds(5)) + .setReadTimeout(Duration.ofSeconds(60)) + .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java` around lines 34 - 37, The RestTemplate built in the SecurityAnalysisRestService constructor currently has no connect/read timeouts; update the constructor to configure timeouts on the RestTemplateBuilder before building (e.g. use setConnectTimeout and setReadTimeout with appropriate Durations) so securityAnalysisServerRest has both connect and read timeouts to avoid indefinite blocking. Locate the SecurityAnalysisRestService constructor and modify the restTemplateBuilder chain to apply the timeouts prior to calling build().monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (1)
222-240: Consider extracting the execution environment name to a constant.Avoids a magic string and makes future reuse/changes safer.
♻️ Suggested refactor
public class MonitorService { + private static final String EXECUTION_ENV_USING_SERVERS = "using-servers-env"; @@ - .executionEnvName("using-servers-env") + .executionEnvName(EXECUTION_ENV_USING_SERVERS) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java` around lines 222 - 240, The method executeProcessUsingServers in MonitorService uses a hard-coded executionEnvName "using-servers-env"; extract this magic string to a private static final constant (e.g., EXEC_ENV_USING_SERVERS) and replace the literal in ProcessExecutionEntity.builder().executionEnvName(...) with that constant so the environment name is centralized for reuse and safer refactoring across MonitorService and any other callers.monitor-server/src/main/java/org/gridsuite/monitor/server/services/NetworkModificationRestService.java (1)
34-37: Add connect/read timeouts for this external call.Keeps message consumer threads from hanging if the server is unresponsive.
⏱️ Suggested change
+import java.time.Duration; @@ - this.networkModificationServerRest = restTemplateBuilder.build(); + this.networkModificationServerRest = restTemplateBuilder + .setConnectTimeout(Duration.ofSeconds(5)) + .setReadTimeout(Duration.ofSeconds(60)) + .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NetworkModificationRestService.java` around lines 34 - 37, The RestTemplate created in the NetworkModificationRestService constructor is missing connect/read timeouts; update the constructor to configure timeouts on the RestTemplateBuilder before building (use RestTemplateBuilder.setConnectTimeout(Duration) and setReadTimeout(Duration)) so networkModificationServerRest is created with sensible timeouts (e.g., configurable via properties or reasonable defaults) while leaving networkModificationServerBaseUri unchanged.monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java (1)
50-50: Add an eviction/timeout strategy for execution contexts.Entries are only removed on explicit completion/failure paths, so a lost message or hung external server can leave the map growing without bound. Consider expiring contexts after a max duration or on execution timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java` at line 50, The processExecutionContexts map in ConsumerServiceUsingServers can grow unbounded because entries are only removed on completion/failure; add an eviction/timeout strategy by associating an expiration time with each ProcessExecutionContext (or storing a small wrapper with expiry) and scheduling periodic cleanup or per-entry scheduled removal using a ScheduledExecutorService. On context creation (where you currently put into processExecutionContexts) schedule a removal task that removes the UUID key after the max allowed duration (or use a timed cache like Caffeine/Guava with expireAfterWrite), and ensure you cancel that scheduled removal and remove the entry immediately in the existing completion/failure paths to avoid races. Include configuration for the timeout value and unit so it can be tuned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`:
- Around line 76-83: The switch over processConfig.processType() only handles
SECURITY_ANALYSIS and leaves contingencies and parametersUuid null for other
types; add an explicit default branch in that switch (inside
ConsumerServiceUsingServers -> the block where contingencies and parametersUuid
are set) that marks the execution as failed and returns early instead of
continuing to schedule a security analysis, e.g. call the failure-path used
elsewhere in this class (or set the appropriate execution status/emit error and
return) so unsupported process types cannot proceed with null
contingencies/parametersUuid.
- Around line 129-134: In ConsumerServiceUsingServers, when handling
non-COMPLETED stepStatus (the else branch that currently only does
processExecutionContexts.remove(executionId)), update the corresponding
execution record to a terminal state (FAILED or CANCELLED) and set its end time
before removing the context; locate the execution by executionId (or via any
Execution/ExecutionRepository/ExecutionService used elsewhere in this class) and
call its update/persist method so the execution status and endTimestamp are
saved, then remove the processExecutionContexts entry and skip calling
securityAnalysisRestService.runSecurityAnalysis.
- Around line 85-101: The code currently creates new ProcessExecutionContext
entries and always calls networkModificationRestService.applyModifications for
every incoming message, which can cause duplicate modifications; add a
deduplication guard that checks whether this executionId already exists or is
past the start state before creating a new ProcessExecutionContext and updating
step statuses (e.g., check processExecutionContexts.containsKey(executionId) or
consult monitorService for an existing ProcessStatus for executionId and return
early if already RUNNING/COMPLETED), and ensure the external call
networkModificationRestService.applyModifications is invoked only when the guard
allows (also verify applyModifications treats executionId idempotently on the
network-modification-server side).
- Around line 148-149: The call StepStatus.valueOf(caseResultInfos.getStatus())
can throw for unexpected server values; in both consumeNetworkModifications()
and consumeSecurityAnalysis() replace direct valueOf usage with safe parsing:
catch IllegalArgumentException/NullPointerException around the valueOf call (or
use StepStatus.fromString helper) and fall back to a default safe status (e.g.,
StepStatus.UNKNOWN or StepStatus.FAILED) while logging the raw status and
context; ensure the surrounding logic uses the fallback status so consumers
don't crash or loop indefinitely and add a unit-friendly message to logs
identifying the method (consumeNetworkModifications / consumeSecurityAnalysis)
and the offending status string.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java`:
- Around line 40-42: In runSecurityAnalysis, the UriComponentsBuilder.fromPath
call builds a path with a redundant slash causing "/v1//cases/…"; fix by
removing the extra "/" before "cases" so the path concatenation uses DELIMITER +
SECURITY_ANALYSIS_SERVER_API_VERSION + DELIMITER +
"cases/{caseUuid}/run-and-save" (or otherwise ensure only one DELIMITER is
inserted), updating the UriComponentsBuilder.fromPath invocation accordingly.
In `@monitor-server/src/main/resources/config/application.yaml`:
- Around line 14-21: The three consumer bindings consumeRunUsingServers-in-0,
consumeNetworkModifications-in-0, and consumeSecurityAnalysis-in-0 should be
given explicit consumer group names to enable competing-consumer behavior;
update each binding in application.yaml to add a unique group property (for
example runUsingServersGroup, networkModificationsGroup, securityAnalysisGroup)
so only one monitor-server instance in the group processes each message and
prevents duplicate APPLY_MODIFICATIONS/SECURITY_ANALYSIS executions.
---
Nitpick comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`:
- Line 50: The processExecutionContexts map in ConsumerServiceUsingServers can
grow unbounded because entries are only removed on completion/failure; add an
eviction/timeout strategy by associating an expiration time with each
ProcessExecutionContext (or storing a small wrapper with expiry) and scheduling
periodic cleanup or per-entry scheduled removal using a
ScheduledExecutorService. On context creation (where you currently put into
processExecutionContexts) schedule a removal task that removes the UUID key
after the max allowed duration (or use a timed cache like Caffeine/Guava with
expireAfterWrite), and ensure you cancel that scheduled removal and remove the
entry immediately in the existing completion/failure paths to avoid races.
Include configuration for the timeout value and unit so it can be tuned.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 222-240: The method executeProcessUsingServers in MonitorService
uses a hard-coded executionEnvName "using-servers-env"; extract this magic
string to a private static final constant (e.g., EXEC_ENV_USING_SERVERS) and
replace the literal in ProcessExecutionEntity.builder().executionEnvName(...)
with that constant so the environment name is centralized for reuse and safer
refactoring across MonitorService and any other callers.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NetworkModificationRestService.java`:
- Around line 34-37: The RestTemplate created in the
NetworkModificationRestService constructor is missing connect/read timeouts;
update the constructor to configure timeouts on the RestTemplateBuilder before
building (use RestTemplateBuilder.setConnectTimeout(Duration) and
setReadTimeout(Duration)) so networkModificationServerRest is created with
sensible timeouts (e.g., configurable via properties or reasonable defaults)
while leaving networkModificationServerBaseUri unchanged.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java`:
- Around line 34-37: The RestTemplate built in the SecurityAnalysisRestService
constructor currently has no connect/read timeouts; update the constructor to
configure timeouts on the RestTemplateBuilder before building (e.g. use
setConnectTimeout and setReadTimeout with appropriate Durations) so
securityAnalysisServerRest has both connect and read timeouts to avoid
indefinite blocking. Locate the SecurityAnalysisRestService constructor and
modify the restTemplateBuilder chain to apply the timeouts prior to calling
build().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/CaseResultInfos.javamonitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/NetworkModificationRestService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.javamonitor-server/src/main/resources/application-local.yamlmonitor-server/src/main/resources/config/application.yaml
Comment on lines
+76
to
+83
| List<String> contingencies = null; | ||
| UUID parametersUuid = null; | ||
| switch (processConfig.processType()) { | ||
| case SECURITY_ANALYSIS -> { | ||
| contingencies = ((SecurityAnalysisConfig) processConfig).contingencies(); | ||
| parametersUuid = ((SecurityAnalysisConfig) processConfig).parametersUuid(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast for unsupported process types.
Only SECURITY_ANALYSIS is handled; other types leave contingencies/parametersUuid null but still schedule security analysis and call the SA server. Add a default branch that marks the execution failed and returns.
✅ Add an explicit default branch
List<String> contingencies = null;
UUID parametersUuid = null;
switch (processConfig.processType()) {
case SECURITY_ANALYSIS -> {
- contingencies = ((SecurityAnalysisConfig) processConfig).contingencies();
- parametersUuid = ((SecurityAnalysisConfig) processConfig).parametersUuid();
+ SecurityAnalysisConfig saConfig = (SecurityAnalysisConfig) processConfig;
+ contingencies = saConfig.contingencies();
+ parametersUuid = saConfig.parametersUuid();
}
+ default -> {
+ LOGGER.error("Unsupported process type {} for executionId: {}", processConfig.processType(), executionId);
+ monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now());
+ return;
+ }
}📝 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
| List<String> contingencies = null; | |
| UUID parametersUuid = null; | |
| switch (processConfig.processType()) { | |
| case SECURITY_ANALYSIS -> { | |
| contingencies = ((SecurityAnalysisConfig) processConfig).contingencies(); | |
| parametersUuid = ((SecurityAnalysisConfig) processConfig).parametersUuid(); | |
| } | |
| } | |
| List<String> contingencies = null; | |
| UUID parametersUuid = null; | |
| switch (processConfig.processType()) { | |
| case SECURITY_ANALYSIS -> { | |
| SecurityAnalysisConfig saConfig = (SecurityAnalysisConfig) processConfig; | |
| contingencies = saConfig.contingencies(); | |
| parametersUuid = saConfig.parametersUuid(); | |
| } | |
| default -> { | |
| LOGGER.error("Unsupported process type {} for executionId: {}", processConfig.processType(), executionId); | |
| monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`
around lines 76 - 83, The switch over processConfig.processType() only handles
SECURITY_ANALYSIS and leaves contingencies and parametersUuid null for other
types; add an explicit default branch in that switch (inside
ConsumerServiceUsingServers -> the block where contingencies and parametersUuid
are set) that marks the execution as failed and returns early instead of
continuing to schedule a security analysis, e.g. call the failure-path used
elsewhere in this class (or set the appropriate execution status/emit error and
return) so unsupported process types cannot proceed with null
contingencies/parametersUuid.
Comment on lines
85
to
101
| processExecutionContexts.put(executionId, new ProcessExecutionContext( | ||
| applyModificationsStepId, | ||
| securityAnalysisStepId, | ||
| caseUuid, | ||
| contingencies, | ||
| parametersUuid | ||
| )); | ||
|
|
||
| monitorService.updateExecutionStatus(executionId, ProcessStatus.RUNNING, null, Instant.now(), null); | ||
| monitorService.updateStepStatus(executionId, | ||
| new ProcessExecutionStep(applyModificationsStepId, "APPLY_MODIFICATIONS", 0, StepStatus.RUNNING, null, null, null, Instant.now(), null)); | ||
| monitorService.updateStepStatus(executionId, | ||
| new ProcessExecutionStep(securityAnalysisStepId, "SECURITY_ANALYSIS", 1, StepStatus.SCHEDULED, null, null, null, null, null)); | ||
|
|
||
| // call network-modification-server to apply modifications | ||
| networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids); | ||
| }; |
There was a problem hiding this comment.
Guard against duplicate run messages to avoid double modifications.
Message delivery is typically at-least-once; this code generates new step IDs and calls applyModifications on every delivery, which can duplicate steps and apply changes twice. Add a deduplication guard (and keep the external call idempotent by executionId).
🛡️ Proposed deduplication guard
- processExecutionContexts.put(executionId, new ProcessExecutionContext(
- applyModificationsStepId,
- securityAnalysisStepId,
- caseUuid,
- contingencies,
- parametersUuid
- ));
+ ProcessExecutionContext newContext = new ProcessExecutionContext(
+ applyModificationsStepId,
+ securityAnalysisStepId,
+ caseUuid,
+ contingencies,
+ parametersUuid
+ );
+ ProcessExecutionContext existing = processExecutionContexts.putIfAbsent(executionId, newContext);
+ if (existing != null) {
+ LOGGER.warn("Duplicate run message ignored for executionId: {}", executionId);
+ return;
+ }📝 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
| processExecutionContexts.put(executionId, new ProcessExecutionContext( | |
| applyModificationsStepId, | |
| securityAnalysisStepId, | |
| caseUuid, | |
| contingencies, | |
| parametersUuid | |
| )); | |
| monitorService.updateExecutionStatus(executionId, ProcessStatus.RUNNING, null, Instant.now(), null); | |
| monitorService.updateStepStatus(executionId, | |
| new ProcessExecutionStep(applyModificationsStepId, "APPLY_MODIFICATIONS", 0, StepStatus.RUNNING, null, null, null, Instant.now(), null)); | |
| monitorService.updateStepStatus(executionId, | |
| new ProcessExecutionStep(securityAnalysisStepId, "SECURITY_ANALYSIS", 1, StepStatus.SCHEDULED, null, null, null, null, null)); | |
| // call network-modification-server to apply modifications | |
| networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids); | |
| }; | |
| ProcessExecutionContext newContext = new ProcessExecutionContext( | |
| applyModificationsStepId, | |
| securityAnalysisStepId, | |
| caseUuid, | |
| contingencies, | |
| parametersUuid | |
| ); | |
| ProcessExecutionContext existing = processExecutionContexts.putIfAbsent(executionId, newContext); | |
| if (existing != null) { | |
| LOGGER.warn("Duplicate run message ignored for executionId: {}", executionId); | |
| return; | |
| } | |
| monitorService.updateExecutionStatus(executionId, ProcessStatus.RUNNING, null, Instant.now(), null); | |
| monitorService.updateStepStatus(executionId, | |
| new ProcessExecutionStep(applyModificationsStepId, "APPLY_MODIFICATIONS", 0, StepStatus.RUNNING, null, null, null, Instant.now(), null)); | |
| monitorService.updateStepStatus(executionId, | |
| new ProcessExecutionStep(securityAnalysisStepId, "SECURITY_ANALYSIS", 1, StepStatus.SCHEDULED, null, null, null, null, null)); | |
| // call network-modification-server to apply modifications | |
| networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`
around lines 85 - 101, The code currently creates new ProcessExecutionContext
entries and always calls networkModificationRestService.applyModifications for
every incoming message, which can cause duplicate modifications; add a
deduplication guard that checks whether this executionId already exists or is
past the start state before creating a new ProcessExecutionContext and updating
step statuses (e.g., check processExecutionContexts.containsKey(executionId) or
consult monitorService for an existing ProcessStatus for executionId and return
early if already RUNNING/COMPLETED), and ensure the external call
networkModificationRestService.applyModifications is invoked only when the guard
allows (also verify applyModifications treats executionId idempotently on the
network-modification-server side).
Comment on lines
+129
to
+134
| if (stepStatus == StepStatus.COMPLETED) { | ||
| // call security-analysis-server to run security analysis | ||
| securityAnalysisRestService.runSecurityAnalysis(caseResultUuid, executionId, processExecutionContext.contingencies(), processExecutionContext.parametersUuid()); | ||
| } else { | ||
| processExecutionContexts.remove(executionId); | ||
| } |
There was a problem hiding this comment.
Update execution status when network modifications fail.
On non-COMPLETED statuses you skip security analysis and remove the context, but the execution remains RUNNING. Mark the execution as FAILED (or CANCELLED) and set the end time.
🧭 Propagate failure to the execution status
if (stepStatus == StepStatus.COMPLETED) {
// call security-analysis-server to run security analysis
securityAnalysisRestService.runSecurityAnalysis(caseResultUuid, executionId, processExecutionContext.contingencies(), processExecutionContext.parametersUuid());
} else {
+ monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now());
processExecutionContexts.remove(executionId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`
around lines 129 - 134, In ConsumerServiceUsingServers, when handling
non-COMPLETED stepStatus (the else branch that currently only does
processExecutionContexts.remove(executionId)), update the corresponding
execution record to a terminal state (FAILED or CANCELLED) and set its end time
before removing the context; locate the execution by executionId (or via any
Execution/ExecutionRepository/ExecutionService used elsewhere in this class) and
call its update/persist method so the execution status and endTimestamp are
saved, then remove the processExecutionContexts entry and skip calling
securityAnalysisRestService.runSecurityAnalysis.
Comment on lines
148
to
149
| StepStatus stepStatus = StepStatus.valueOf(caseResultInfos.getStatus()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the current state around lines 148-149
find . -type f -name "ConsumerServiceUsingServers.java" -exec wc -l {} \;Repository: gridsuite/monitor-core
Length of output: 171
🏁 Script executed:
# Check the file structure and content around the specified lines
find . -type f -name "ConsumerServiceUsingServers.java" -exec sed -n '140,160p' {} +Repository: gridsuite/monitor-core
Length of output: 1332
🏁 Script executed:
# Find CaseResultInfos class definition
fd 'CaseResultInfos.java' -x cat -n {}Repository: gridsuite/monitor-core
Length of output: 1386
🏁 Script executed:
# Search for all StepStatus.valueOf usages in the codebase
rg -n "StepStatus\.valueOf" -g '*.java' -B2 -A2Repository: gridsuite/monitor-core
Length of output: 1740
🏁 Script executed:
# Find consumeNetworkModifications method
rg -n "consumeNetworkModifications" -g '*.java' -B2 -A10Repository: gridsuite/monitor-core
Length of output: 2030
🏁 Script executed:
# Check StepStatus enum definition to see valid values
fd 'StepStatus.java' -x cat -n {}Repository: gridsuite/monitor-core
Length of output: 663
🏁 Script executed:
# Check the method name for line 148
sed -n '130,150p' ./monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.javaRepository: gridsuite/monitor-core
Length of output: 1133
🏁 Script executed:
# Search for any other StepStatus usages or related parsing
rg -n "StepStatus|getStatus" ./monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java -B1 -A1Repository: gridsuite/monitor-core
Length of output: 2294
🏁 Script executed:
# Check if there's any validation or documentation about status values from external servers
rg -n "status|Status" ./monitor-server/src/main/java/org/gridsuite/monitor/commons/CaseResultInfos.java -B2 -A2Repository: gridsuite/monitor-core
Length of output: 187
Harden status parsing from external servers in two locations.
StepStatus.valueOf will throw if the server sends an unexpected or case-mismatched status, causing the consumer to crash and potentially retry indefinitely. Unsafe parsing exists at:
- Line 114 in
consumeNetworkModifications() - Line 148 in
consumeSecurityAnalysis()
Use safe parsing with error handling and fallback in both methods.
🛡️ Safer status parsing pattern
- StepStatus stepStatus = StepStatus.valueOf(caseResultInfos.getStatus());
+ StepStatus stepStatus;
+ try {
+ stepStatus = StepStatus.valueOf(caseResultInfos.getStatus());
+ } catch (IllegalArgumentException ex) {
+ LOGGER.error("Unsupported step status '{}' for executionId: {}", caseResultInfos.getStatus(), executionId, ex);
+ monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now());
+ processExecutionContexts.remove(executionId);
+ return;
+ }📝 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
| StepStatus stepStatus = StepStatus.valueOf(caseResultInfos.getStatus()); | |
| StepStatus stepStatus; | |
| try { | |
| stepStatus = StepStatus.valueOf(caseResultInfos.getStatus()); | |
| } catch (IllegalArgumentException ex) { | |
| LOGGER.error("Unsupported step status '{}' for executionId: {}", caseResultInfos.getStatus(), executionId, ex); | |
| monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); | |
| processExecutionContexts.remove(executionId); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`
around lines 148 - 149, The call StepStatus.valueOf(caseResultInfos.getStatus())
can throw for unexpected server values; in both consumeNetworkModifications()
and consumeSecurityAnalysis() replace direct valueOf usage with safe parsing:
catch IllegalArgumentException/NullPointerException around the valueOf call (or
use StepStatus.fromString helper) and fall back to a default safe status (e.g.,
StepStatus.UNKNOWN or StepStatus.FAILED) while logging the raw status and
context; ensure the surrounding logic uses the fallback status so consumers
don't crash or loop indefinitely and add a unit-friendly message to logs
identifying the method (consumeNetworkModifications / consumeSecurityAnalysis)
and the offending status string.
Comment on lines
+40
to
+42
| public void runSecurityAnalysis(UUID caseUuid, UUID executionId, List<String> contingencies, UUID parametersUuid) { | ||
| var uriComponentsBuilder = UriComponentsBuilder.fromPath(DELIMITER + SECURITY_ANALYSIS_SERVER_API_VERSION + DELIMITER + "/cases/{caseUuid}/run-and-save"); | ||
| var path = uriComponentsBuilder |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SecurityAnalysisRestService.java" -type fRepository: gridsuite/monitor-core
Length of output: 167
🏁 Script executed:
cat -n ./monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java | head -100Repository: gridsuite/monitor-core
Length of output: 2915
Remove the redundant leading slash in the path construction.
The current path concatenation produces /v1//cases/... due to the extra / before "cases". This double-slash can cause issues with some proxies or stricter routing rules.
Suggested fix
- var uriComponentsBuilder = UriComponentsBuilder.fromPath(DELIMITER + SECURITY_ANALYSIS_SERVER_API_VERSION + DELIMITER + "/cases/{caseUuid}/run-and-save");
+ var uriComponentsBuilder = UriComponentsBuilder.fromPath(DELIMITER + SECURITY_ANALYSIS_SERVER_API_VERSION + DELIMITER + "cases/{caseUuid}/run-and-save");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisRestService.java`
around lines 40 - 42, In runSecurityAnalysis, the UriComponentsBuilder.fromPath
call builds a path with a redundant slash causing "/v1//cases/…"; fix by
removing the extra "/" before "cases" so the path concatenation uses DELIMITER +
SECURITY_ANALYSIS_SERVER_API_VERSION + DELIMITER +
"cases/{caseUuid}/run-and-save" (or otherwise ensure only one DELIMITER is
inserted), updating the UriComponentsBuilder.fromPath invocation accordingly.
Comment on lines
+14
to
+21
| publishRunSecurityAnalysisUsingServers-out-0: | ||
| destination: ${powsybl-ws.rabbitmq.destination.prefix:}monitor.process.securityanalysis.using.servers.run | ||
| consumeRunUsingServers-in-0: | ||
| destination: ${powsybl-ws.rabbitmq.destination.prefix:}monitor.process.securityanalysis.using.servers.run | ||
| consumeNetworkModifications-in-0: | ||
| destination: ${powsybl-ws.rabbitmq.destination.prefix:}modifications.case.result | ||
| consumeSecurityAnalysis-in-0: | ||
| destination: ${powsybl-ws.rabbitmq.destination.prefix:}sa.case.result |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "application\.(yaml|yml|properties)" | head -20Repository: gridsuite/monitor-core
Length of output: 297
🏁 Script executed:
find . -path "*/monitor-server/src/main/resources/config/application.yaml" -o -path "*/application.yaml" | head -20Repository: gridsuite/monitor-core
Length of output: 305
🏁 Script executed:
fd "application\.(yaml|yml)" monitor-server/Repository: gridsuite/monitor-core
Length of output: 174
🏁 Script executed:
cat -n monitor-server/src/main/resources/config/application.yaml | head -50Repository: gridsuite/monitor-core
Length of output: 1591
🏁 Script executed:
rg "consumer.*group|duplicate.*processing" monitor-server/ -i -A 2 -B 2Repository: gridsuite/monitor-core
Length of output: 48
🏁 Script executed:
rg "spring.*cloud.*stream" . --type yaml --type yml -lRepository: gridsuite/monitor-core
Length of output: 93
🏁 Script executed:
fd "pom.xml|build.gradle" monitor-server/ | xargs cat | grep -A 5 -B 5 "spring-cloud-stream"Repository: gridsuite/monitor-core
Length of output: 1369
🏁 Script executed:
find . -name "docker-compose*.yml" -o -name "docker-compose*.yaml" | xargs cat 2>/dev/null | grep -A 5 "monitor-server"Repository: gridsuite/monitor-core
Length of output: 48
🏁 Script executed:
rg "replicas|instances|scale" --type yaml -A 2 -B 2 | head -30Repository: gridsuite/monitor-core
Length of output: 48
🏁 Script executed:
find . -path "*/src/main/java/*" -name "*Consume*" -o -name "*Consumer*" | head -10Repository: gridsuite/monitor-core
Length of output: 563
🏁 Script executed:
find . -path "*/src/main/java/org/gridsuite/monitor/server/services/ConsumerService.java" -exec cat -n {} \; | head -100Repository: gridsuite/monitor-core
Length of output: 4912
🏁 Script executed:
rg "consumeNetworkModifications|consumeSecurityAnalysis|consumeRunUsingServers" monitor-server/src/main/java -A 15 -B 2Repository: gridsuite/monitor-core
Length of output: 8032
🏁 Script executed:
rg "idempotent|deduplicat|duplicate|lock|mutex" monitor-server/src/main/java -i -A 2Repository: gridsuite/monitor-core
Length of output: 48
🏁 Script executed:
rg "processExecutionContexts" monitor-server/src/main/java -B 5 -A 5 | head -80Repository: gridsuite/monitor-core
Length of output: 9479
Add consumer groups to prevent duplicate process execution across multiple instances.
Spring Cloud Stream creates a broadcast queue (anonymous group) when no consumer group is configured. With multiple monitor-server instances, all instances will independently receive and process consumeRunUsingServers, consumeNetworkModifications, and consumeSecurityAnalysis messages, causing duplicate APPLY_MODIFICATIONS and SECURITY_ANALYSIS operations. The in-memory processExecutionContexts map cannot synchronize state across instances. Configure explicit consumer groups to enable competing consumer pattern, so only one instance in the group processes each message.
Note: consumeMonitorUpdate-in-0 already has group: monitorUpdateGroup, confirming this pattern is established in your configuration.
💡 Suggested configuration
consumeRunUsingServers-in-0:
destination: ${powsybl-ws.rabbitmq.destination.prefix:}monitor.process.securityanalysis.using.servers.run
+ group: monitorRunUsingServersGroup
consumeNetworkModifications-in-0:
destination: ${powsybl-ws.rabbitmq.destination.prefix:}modifications.case.result
+ group: monitorNetworkModificationsGroup
consumeSecurityAnalysis-in-0:
destination: ${powsybl-ws.rabbitmq.destination.prefix:}sa.case.result
+ group: monitorSecurityAnalysisGroup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monitor-server/src/main/resources/config/application.yaml` around lines 14 -
21, The three consumer bindings consumeRunUsingServers-in-0,
consumeNetworkModifications-in-0, and consumeSecurityAnalysis-in-0 should be
given explicit consumer group names to enable competing-consumer behavior;
update each binding in application.yaml to add a unique group property (for
example runUsingServersGroup, networkModificationsGroup, securityAnalysisGroup)
so only one monitor-server instance in the group processes each message and
prevents duplicate APPLY_MODIFICATIONS/SECURITY_ANALYSIS executions.
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
159-196:⚠️ Potential issue | 🟡 MinorAdd
resultCaseIdassertions to all three step-status update test methods.The three test methods—
updateStepStatusShouldAddNewStep,updateStepStatusShouldUpdateExistingStep, andupdateStepsStatusesShouldUpdateExistingSteps—do not set or assertresultCaseIdon the DTO or resulting entity. AlthoughMonitorService.updateStepStatuscorrectly copiesresultCaseIdfrom DTO to entity, this persistence path is not verified by tests.Add a
resultCaseIdUUID to each test'sProcessExecutionStepbuilder and assertaddedStep.getResultCaseId()/updatedStep.getResultCaseId()respectively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java` around lines 159 - 196, Add a UUID for resultCaseId to each ProcessExecutionStep built in the three tests (updateStepStatusShouldAddNewStep, updateStepStatusShouldUpdateExistingStep, updateStepsStatusesShouldUpdateExistingSteps) and include assertions that the resulting entity's getResultCaseId() equals that UUID after MonitorService.updateStepStatus/updateStepsStatuses runs; locate the ProcessExecutionStep builder usages in each test, set .resultCaseId(resultCaseId) on the DTO, and add assertThat(addedStep.getResultCaseId()).isEqualTo(resultCaseId) or assertThat(updatedStep.getResultCaseId()).isEqualTo(resultCaseId) as appropriate, keeping the existing verify(executionRepository).save(execution) checks.
♻️ Duplicate comments (4)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java (4)
115-115:⚠️ Potential issue | 🟠 MajorHarden enum parsing for external status payloads.
Lines 115 and 149 use
StepStatus.valueOf(...)directly. Unexpected/null/case-mismatched status from external servers will throw and can cause repeated consumer failure loops.🛡️ Proposed fix
- StepStatus stepStatus = StepStatus.valueOf(caseResultInfos.getStatus()); + StepStatus stepStatus = parseStepStatus(caseResultInfos.getStatus(), executionId, "consumeNetworkModifications"); + if (stepStatus == null) { + monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); + processExecutionContexts.remove(executionId); + return; + } ... - StepStatus stepStatus = StepStatus.valueOf(caseResultInfos.getStatus()); + StepStatus stepStatus = parseStepStatus(caseResultInfos.getStatus(), executionId, "consumeSecurityAnalysis"); + if (stepStatus == null) { + monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); + processExecutionContexts.remove(executionId); + return; + }// Add helper in the same class (outside bean methods) private StepStatus parseStepStatus(String rawStatus, UUID executionId, String consumerName) { if (rawStatus == null) { LOGGER.error("{} received null status for executionId: {}", consumerName, executionId); return null; } try { return StepStatus.valueOf(rawStatus); } catch (IllegalArgumentException ex) { LOGGER.error("{} received unsupported status '{}' for executionId: {}", consumerName, rawStatus, executionId, ex); return null; } }#!/bin/bash # Verify enum parsing risk and call sites. # Expected: StepStatus enum constants are strict names; direct valueOf calls exist on raw external status strings. fd 'StepStatus.java' -x sed -n '1,220p' {} fd 'CaseResultInfos.java' -x sed -n '1,220p' {} rg -n -C2 'StepStatus\.valueOf\(caseResultInfos\.getStatus\(\)\)' --type=javaAlso applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java` at line 115, The code calls StepStatus.valueOf(...) directly (e.g., StepStatus.valueOf(caseResultInfos.getStatus())) which will throw on null/unknown/case-mismatched external values; add a private helper method parseStepStatus(String rawStatus, UUID executionId, String consumerName) in ConsumerServiceUsingServers that returns null on null/unknown values and logs a descriptive error (including rawStatus, executionId and consumerName) instead of throwing, then replace direct valueOf(...) calls with parseStepStatus(caseResultInfos.getStatus(), executionId, consumerName) at the existing call sites (the occurrences of StepStatus.valueOf(caseResultInfos.getStatus()) around the consumer processing logic).
78-83:⚠️ Potential issue | 🟠 MajorFail fast for unsupported process types and unsafe config cast.
Line 78 only handles
SECURITY_ANALYSIS; other types continue with nullcontingencies/parametersUuid. Also, Lines 80-81 perform an unchecked cast. Add a guarded branch and return early on unsupported or mismatched config types.✅ Proposed fix
List<String> contingencies = null; UUID parametersUuid = null; switch (processConfig.processType()) { case SECURITY_ANALYSIS -> { - contingencies = ((SecurityAnalysisConfig) processConfig).contingencies(); - parametersUuid = ((SecurityAnalysisConfig) processConfig).parametersUuid(); + if (!(processConfig instanceof SecurityAnalysisConfig saConfig)) { + LOGGER.error("Invalid config type {} for process type {} and executionId: {}", + processConfig.getClass().getName(), processConfig.processType(), executionId); + monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); + return; + } + contingencies = saConfig.contingencies(); + parametersUuid = saConfig.parametersUuid(); } + default -> { + LOGGER.error("Unsupported process type {} for executionId: {}", processConfig.processType(), executionId); + monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); + return; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java` around lines 78 - 83, The switch in ConsumerServiceUsingServers.java that sets contingencies and parametersUuid only handles SECURITY_ANALYSIS and performs an unchecked cast; update the switch in the method containing processConfig.processType() so that the SECURITY_ANALYSIS branch first checks that processConfig is an instance of SecurityAnalysisConfig before casting (e.g., if (!(processConfig instanceof SecurityAnalysisConfig)) throw new IllegalArgumentException(...)); assign contingencies and parametersUuid from the safely-cast SecurityAnalysisConfig, and add a default branch that fails fast (throw IllegalArgumentException or a DomainSpecificException) for unsupported process types to avoid null values and unsafe casts.
85-101:⚠️ Potential issue | 🔴 CriticalAdd deduplication guard before scheduling external work.
Lines 85-101 accept duplicate run messages for the same
executionId, create new step IDs, and callapplyModificationsagain. This can trigger duplicate side effects.🛡️ Proposed fix
- processExecutionContexts.put(executionId, new ProcessExecutionContext( + ProcessExecutionContext newContext = new ProcessExecutionContext( applyModificationsStepId, securityAnalysisStepId, caseUuid, contingencies, parametersUuid - )); + ); + ProcessExecutionContext existing = processExecutionContexts.putIfAbsent(executionId, newContext); + if (existing != null) { + LOGGER.warn("Duplicate run message ignored for executionId: {}", executionId); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java` around lines 85 - 101, The code currently accepts duplicate run messages and may schedule external work twice; before creating a new ProcessExecutionContext and calling networkModificationRestService.applyModifications, add a deduplication guard that checks the existing processExecutionContexts for executionId (e.g., use processExecutionContexts.putIfAbsent(executionId, new ProcessExecutionContext(...)) or check containsKey and return early), and only proceed with monitorService.updateExecutionStatus, updateStepStatus calls, and networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids) when the context was actually inserted (i.e., this is the first run for that executionId) to prevent duplicate side effects.
133-135:⚠️ Potential issue | 🟠 MajorPropagate network-modification failure to execution status.
Line 134 removes context for non-
COMPLETEDnetwork-modification outcomes, but execution status is not transitioned to a terminal failed state.🧭 Proposed fix
if (stepStatus == StepStatus.COMPLETED) { // call security-analysis-server to run security analysis securityAnalysisRestService.runSecurityAnalysis(caseResultUuid, executionId, processExecutionContext.contingencies(), processExecutionContext.parametersUuid()); } else { + monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now()); processExecutionContexts.remove(executionId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java` around lines 133 - 135, The current branch in ConsumerServiceUsingServers that calls processExecutionContexts.remove(executionId) for non-COMPLETED network-modification outcomes discards the context without marking the execution as failed; update this branch so that before removing the context you transition the execution's status to a terminal failed state (e.g., set the Execution/ExecutionContext status to FAILED or invoke the existing failure-handling API), include the network-modification error/reason when setting the status, and then remove the context; reference the network-modification outcome handling block and replace the bare processExecutionContexts.remove(executionId) with a call that updates the execution status (or calls markExecutionFailed/handleExecutionFailure if such helper exists) then removes the context.
🧹 Nitpick comments (4)
monitor-server/src/main/resources/db/changelog/changesets/changelog_20260225T073450Z.xml (1)
3-7: Consider adding an explicit<rollback>block.Without a
<rollback>tag, Liquibase's auto-rollback foraddColumnis database-dependent and not always reliable. An explicit rollback makes the migration fully reversible:♻️ Proposed rollback addition
<changeSet author="lecuyerfra (generated)" id="1772004899864-1"> <addColumn tableName="process_execution_step"> <column name="result_case_id" type="uuid"/> </addColumn> + <rollback> + <dropColumn tableName="process_execution_step" columnName="result_case_id"/> + </rollback> </changeSet>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/resources/db/changelog/changesets/changelog_20260225T073450Z.xml` around lines 3 - 7, Add an explicit <rollback> block to the changeSet id "1772004899864-1" that drops the added column to ensure deterministic reversibility: in the changeSet that adds column "result_case_id" to table "process_execution_step", include a rollback that executes a dropColumn (or equivalent SQL) for "result_case_id" so Liquibase will reliably revert the migration across databases.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java (1)
66-66: Consider using the builder instead of the 10-arg positional constructor.Six trailing
nullarguments is error-prone; adding any new field toProcessExecutionStepshifts the argument positions silently. The@Builderis already available.♻️ Proposed refactor
- .mapToObj(i -> new ProcessExecutionStep(steps.get(i).getId(), steps.get(i).getType().getName(), i, StepStatus.SCHEDULED, null, null, null, null, null, null)) + .mapToObj(i -> ProcessExecutionStep.builder() + .id(steps.get(i).getId()) + .stepType(steps.get(i).getType().getName()) + .stepOrder(i) + .status(StepStatus.SCHEDULED) + .build())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java` at line 66, Replace the positional 10-arg constructor call in the mapping lambda with the ProcessExecutionStep.builder() fluent API: set id, typeName, index and status via .id(...).typeName(...).index(...).status(StepStatus.SCHEDULED) and omit or explicitly set the remaining optional fields via their builder methods as needed, then call .build(); update the lambda that currently creates new ProcessExecutionStep(steps.get(i).getId(), steps.get(i).getType().getName(), i, StepStatus.SCHEDULED, null, null, null, null, null, null) to use the builder to avoid the fragile multi-null argument list.monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java (1)
161-163: Consider using the builder instead of the 10-arg positional constructor.Same concern as in
ProcessExecutionServiceandMonitorServiceTest: six positionalnullarguments create brittle call sites.♻️ Proposed refactor
- ProcessExecutionStep processExecutionStep1 = new ProcessExecutionStep(UUID.randomUUID(), "loadNetwork", 0, StepStatus.RUNNING, null, null, UUID.randomUUID(), Instant.now(), null, null); - ProcessExecutionStep processExecutionStep2 = new ProcessExecutionStep(UUID.randomUUID(), "applyModifs", 1, StepStatus.SCHEDULED, null, null, UUID.randomUUID(), null, null, null); - ProcessExecutionStep processExecutionStep3 = new ProcessExecutionStep(UUID.randomUUID(), "runSA", 2, StepStatus.SCHEDULED, null, null, UUID.randomUUID(), null, null, null); + ProcessExecutionStep processExecutionStep1 = ProcessExecutionStep.builder() + .id(UUID.randomUUID()).stepType("loadNetwork").stepOrder(0).status(StepStatus.RUNNING) + .reportId(UUID.randomUUID()).startedAt(Instant.now()).build(); + ProcessExecutionStep processExecutionStep2 = ProcessExecutionStep.builder() + .id(UUID.randomUUID()).stepType("applyModifs").stepOrder(1).status(StepStatus.SCHEDULED) + .reportId(UUID.randomUUID()).build(); + ProcessExecutionStep processExecutionStep3 = ProcessExecutionStep.builder() + .id(UUID.randomUUID()).stepType("runSA").stepOrder(2).status(StepStatus.SCHEDULED) + .reportId(UUID.randomUUID()).build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java` around lines 161 - 163, Replace the brittle 10-arg positional constructor calls for ProcessExecutionStep with the builder API: use ProcessExecutionStep.builder() and set named properties (id, name, order, status, parentExecutionId, startedAt, etc.) instead of passing multiple nulls; update the three test instances (processExecutionStep1, processExecutionStep2, processExecutionStep3) to call builder().id(...).name("...").order(...).status(StepStatus....).parentExecutionId(...).startedAt(...)/scheduledAt(...) as appropriate and then build() to make the test resilient and readable.monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
466-467: Prefer builder over the 10-arg positional constructor in tests.Consistent with the builder usage elsewhere in this same test file, replacing positional constructors reduces the risk of silent argument misalignment on future field additions.
♻️ Proposed refactor
- ProcessExecutionStep processExecutionStep1 = new ProcessExecutionStep(stepId1, "loadNetwork", 0, StepStatus.RUNNING, null, null, null, startedAt1, null, null); - ProcessExecutionStep processExecutionStep2 = new ProcessExecutionStep(stepId2, "applyModifs", 1, StepStatus.SCHEDULED, null, null, null, null, null, null); + ProcessExecutionStep processExecutionStep1 = ProcessExecutionStep.builder() + .id(stepId1).stepType("loadNetwork").stepOrder(0).status(StepStatus.RUNNING).startedAt(startedAt1).build(); + ProcessExecutionStep processExecutionStep2 = ProcessExecutionStep.builder() + .id(stepId2).stepType("applyModifs").stepOrder(1).status(StepStatus.SCHEDULED).build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java` around lines 466 - 467, Replace the two 10-argument positional constructors for ProcessExecutionStep (processExecutionStep1 and processExecutionStep2) with the class's builder to avoid positional-arg mistakes; locate the instantiations of ProcessExecutionStep and use ProcessExecutionStep.builder() to set id, name, order/index, status (e.g., StepStatus.RUNNING / StepStatus.SCHEDULED), startedAt and any other nullable fields explicitly via builder methods, then call build()—mirror the builder style used elsewhere in this test file to keep construction consistent and explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`:
- Around line 99-100: Wrap external REST calls (notably
networkModificationRestService.applyModifications(caseUuid, executionId,
modificationUuids) and the other REST call in the same method) in try/catch to
prevent uncaught exceptions from leaving executions stuck in RUNNING; on failure
catch Exception, log the error with context
(caseUuid/executionId/modificationUuids), transition the execution to a terminal
failure state via the existing execution state updater (e.g.,
executionRepository or executionService method used elsewhere in this class),
persist that state change, and ensure any execution context cleanup/rollback
runs in a finally block so no context leaks remain.
---
Outside diff comments:
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`:
- Around line 159-196: Add a UUID for resultCaseId to each ProcessExecutionStep
built in the three tests (updateStepStatusShouldAddNewStep,
updateStepStatusShouldUpdateExistingStep,
updateStepsStatusesShouldUpdateExistingSteps) and include assertions that the
resulting entity's getResultCaseId() equals that UUID after
MonitorService.updateStepStatus/updateStepsStatuses runs; locate the
ProcessExecutionStep builder usages in each test, set
.resultCaseId(resultCaseId) on the DTO, and add
assertThat(addedStep.getResultCaseId()).isEqualTo(resultCaseId) or
assertThat(updatedStep.getResultCaseId()).isEqualTo(resultCaseId) as
appropriate, keeping the existing verify(executionRepository).save(execution)
checks.
---
Duplicate comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`:
- Line 115: The code calls StepStatus.valueOf(...) directly (e.g.,
StepStatus.valueOf(caseResultInfos.getStatus())) which will throw on
null/unknown/case-mismatched external values; add a private helper method
parseStepStatus(String rawStatus, UUID executionId, String consumerName) in
ConsumerServiceUsingServers that returns null on null/unknown values and logs a
descriptive error (including rawStatus, executionId and consumerName) instead of
throwing, then replace direct valueOf(...) calls with
parseStepStatus(caseResultInfos.getStatus(), executionId, consumerName) at the
existing call sites (the occurrences of
StepStatus.valueOf(caseResultInfos.getStatus()) around the consumer processing
logic).
- Around line 78-83: The switch in ConsumerServiceUsingServers.java that sets
contingencies and parametersUuid only handles SECURITY_ANALYSIS and performs an
unchecked cast; update the switch in the method containing
processConfig.processType() so that the SECURITY_ANALYSIS branch first checks
that processConfig is an instance of SecurityAnalysisConfig before casting
(e.g., if (!(processConfig instanceof SecurityAnalysisConfig)) throw new
IllegalArgumentException(...)); assign contingencies and parametersUuid from the
safely-cast SecurityAnalysisConfig, and add a default branch that fails fast
(throw IllegalArgumentException or a DomainSpecificException) for unsupported
process types to avoid null values and unsafe casts.
- Around line 85-101: The code currently accepts duplicate run messages and may
schedule external work twice; before creating a new ProcessExecutionContext and
calling networkModificationRestService.applyModifications, add a deduplication
guard that checks the existing processExecutionContexts for executionId (e.g.,
use processExecutionContexts.putIfAbsent(executionId, new
ProcessExecutionContext(...)) or check containsKey and return early), and only
proceed with monitorService.updateExecutionStatus, updateStepStatus calls, and
networkModificationRestService.applyModifications(caseUuid, executionId,
modificationUuids) when the context was actually inserted (i.e., this is the
first run for that executionId) to prevent duplicate side effects.
- Around line 133-135: The current branch in ConsumerServiceUsingServers that
calls processExecutionContexts.remove(executionId) for non-COMPLETED
network-modification outcomes discards the context without marking the execution
as failed; update this branch so that before removing the context you transition
the execution's status to a terminal failed state (e.g., set the
Execution/ExecutionContext status to FAILED or invoke the existing
failure-handling API), include the network-modification error/reason when
setting the status, and then remove the context; reference the
network-modification outcome handling block and replace the bare
processExecutionContexts.remove(executionId) with a call that updates the
execution status (or calls markExecutionFailed/handleExecutionFailure if such
helper exists) then removes the context.
---
Nitpick comments:
In
`@monitor-server/src/main/resources/db/changelog/changesets/changelog_20260225T073450Z.xml`:
- Around line 3-7: Add an explicit <rollback> block to the changeSet id
"1772004899864-1" that drops the added column to ensure deterministic
reversibility: in the changeSet that adds column "result_case_id" to table
"process_execution_step", include a rollback that executes a dropColumn (or
equivalent SQL) for "result_case_id" so Liquibase will reliably revert the
migration across databases.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java`:
- Around line 161-163: Replace the brittle 10-arg positional constructor calls
for ProcessExecutionStep with the builder API: use
ProcessExecutionStep.builder() and set named properties (id, name, order,
status, parentExecutionId, startedAt, etc.) instead of passing multiple nulls;
update the three test instances (processExecutionStep1, processExecutionStep2,
processExecutionStep3) to call
builder().id(...).name("...").order(...).status(StepStatus....).parentExecutionId(...).startedAt(...)/scheduledAt(...)
as appropriate and then build() to make the test resilient and readable.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`:
- Around line 466-467: Replace the two 10-argument positional constructors for
ProcessExecutionStep (processExecutionStep1 and processExecutionStep2) with the
class's builder to avoid positional-arg mistakes; locate the instantiations of
ProcessExecutionStep and use ProcessExecutionStep.builder() to set id, name,
order/index, status (e.g., StepStatus.RUNNING / StepStatus.SCHEDULED), startedAt
and any other nullable fields explicitly via builder methods, then call
build()—mirror the builder style used elsewhere in this test file to keep
construction consistent and explicit.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java`:
- Line 66: Replace the positional 10-arg constructor call in the mapping lambda
with the ProcessExecutionStep.builder() fluent API: set id, typeName, index and
status via .id(...).typeName(...).index(...).status(StepStatus.SCHEDULED) and
omit or explicitly set the remaining optional fields via their builder methods
as needed, then call .build(); update the lambda that currently creates new
ProcessExecutionStep(steps.get(i).getId(), steps.get(i).getType().getName(), i,
StepStatus.SCHEDULED, null, null, null, null, null, null) to use the builder to
avoid the fragile multi-null argument list.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessExecutionStep.javamonitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionStepEntity.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionStepMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.javamonitor-server/src/main/resources/db/changelog/changesets/changelog_20260225T073450Z.xmlmonitor-server/src/main/resources/db/changelog/db.changelog-master.yamlmonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Comment on lines
+99
to
+100
| // call network-modification-server to apply modifications | ||
| networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids); |
There was a problem hiding this comment.
Handle REST client failures to avoid stuck RUNNING executions.
Lines 99-100 and 132 call external services without exception handling. If either call throws, execution state can remain inconsistent and context may leak.
🧯 Proposed fix
- networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids);
+ try {
+ networkModificationRestService.applyModifications(caseUuid, executionId, modificationUuids);
+ } catch (RuntimeException ex) {
+ LOGGER.error("applyModifications failed for executionId: {}", executionId, ex);
+ monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now());
+ processExecutionContexts.remove(executionId);
+ }
...
if (stepStatus == StepStatus.COMPLETED) {
// call security-analysis-server to run security analysis
- securityAnalysisRestService.runSecurityAnalysis(caseResultUuid, executionId, processExecutionContext.contingencies(), processExecutionContext.parametersUuid());
+ try {
+ securityAnalysisRestService.runSecurityAnalysis(caseResultUuid, executionId, processExecutionContext.contingencies(), processExecutionContext.parametersUuid());
+ } catch (RuntimeException ex) {
+ LOGGER.error("runSecurityAnalysis failed for executionId: {}", executionId, ex);
+ monitorService.updateExecutionStatus(executionId, ProcessStatus.FAILED, null, null, Instant.now());
+ processExecutionContexts.remove(executionId);
+ }
} else {
processExecutionContexts.remove(executionId);
}Also applies to: 130-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerServiceUsingServers.java`
around lines 99 - 100, Wrap external REST calls (notably
networkModificationRestService.applyModifications(caseUuid, executionId,
modificationUuids) and the other REST call in the same method) in try/catch to
prevent uncaught exceptions from leaving executions stuck in RUNNING; on failure
catch Exception, log the error with context
(caseUuid/executionId/modificationUuids), transition the execution to a terminal
failure state via the existing execution state updater (e.g.,
executionRepository or executionService method used elsewhere in this class),
persist that state change, and ensure any execution context cleanup/rollback
runs in a finally block so no context leaks remain.
…rs_to_run_cyclic_chain Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com> # Conflicts: # monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java # monitor-server/src/main/resources/db/changelog/db.changelog-master.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java (1)
67-67: Consider a builder or named-constructor to replace the 6 trailingnullarguments.With 10 positional parameters (6 of which are
null), the call site is opaque — it's impossible to tell whichnullmaps to which field without consulting the constructor signature. It also makes it easy to silently swap twoUUID-typed nullable fields in the future.If
ProcessExecutionStepis a Java record (as is typical for a DTO), a small static factory or an overloadedscheduled(...)factory method on the record itself would make this readable:♻️ Example: static factory on the record
- .mapToObj(i -> new ProcessExecutionStep(steps.get(i).getId(), steps.get(i).getType().getName(), i, StepStatus.SCHEDULED, null, null, null, null, null, null)) + .mapToObj(i -> ProcessExecutionStep.scheduled(steps.get(i).getId(), steps.get(i).getType().getName(), i))In
ProcessExecutionStep(commons module):// static factory — documents intent and hides the null-filled fields public static ProcessExecutionStep scheduled(UUID id, String typeName, int order) { return new ProcessExecutionStep(id, typeName, order, StepStatus.SCHEDULED, null, null, null, null, null, null); }Alternatively, if a builder is available or can be added (e.g., via Lombok
@Builder), it would achieve the same goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java` at line 67, Replace the opaque constructor call that passes 6 trailing nulls in the mapToObj lambda with a clear factory or builder: add a static factory method on ProcessExecutionStep (e.g., scheduled(UUID id, String typeName, int order) or similar named-constructor) that returns the record/instance with StepStatus.SCHEDULED and the nullable fields set to null, or alternatively add a builder (Lombok `@Builder` or manual) and use it here; then update the lambda (.mapToObj(...)) to call ProcessExecutionStep.scheduled(steps.get(i).getId(), steps.get(i).getType().getName(), i) (or the builder variant) so the call site no longer passes six positional nulls.monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java (1)
33-33: Two adjacentStringparameters (bindingName,debugFileLocation) are silently swappable.Java provides no compile-time protection against swapping
bindingNameanddebugFileLocationat call sites. A misorder would route messages to a wrong/nonexistent binding or send the binding name as a file location without any error.Consider wrapping them in distinct types or a small value object, or at minimum ensure call sites in
MonitorServicehave a test asserting the correct binding name is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java` at line 33, The method sendProcessRunMessage(UUID caseUuid, ProcessConfig processConfig, UUID executionId, String bindingName, String debugFileLocation) accepts two adjacent Strings that can be silently swapped; change the API to use distinct types (e.g., introduce a BindingName value class and a DebugFileLocation value class, or a small ProcessRunDestination/BindingInfo DTO containing explicit fields) and update all call sites (notably in MonitorService) to pass the new typed objects instead of raw Strings; also add a unit test in MonitorService verifying the binding name is passed through correctly to sendProcessRunMessage to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java`:
- Around line 33-36: The MonitorService call to
NotificationService.sendProcessRunMessage passes the two String arguments in the
wrong order; update the call site in MonitorService where
notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), null, bindingName) is invoked to either swap the last two
args to notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), bindingName, null) so bindingName is used for publisher.send,
or replace the call with the 4-parameter overload
(notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), bindingName)) to centralize routing logic; ensure you modify
the call that references execution.getId() and bindingName accordingly.
---
Nitpick comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java`:
- Line 33: The method sendProcessRunMessage(UUID caseUuid, ProcessConfig
processConfig, UUID executionId, String bindingName, String debugFileLocation)
accepts two adjacent Strings that can be silently swapped; change the API to use
distinct types (e.g., introduce a BindingName value class and a
DebugFileLocation value class, or a small ProcessRunDestination/BindingInfo DTO
containing explicit fields) and update all call sites (notably in
MonitorService) to pass the new typed objects instead of raw Strings; also add a
unit test in MonitorService verifying the binding name is passed through
correctly to sendProcessRunMessage to prevent regressions.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java`:
- Line 67: Replace the opaque constructor call that passes 6 trailing nulls in
the mapToObj lambda with a clear factory or builder: add a static factory method
on ProcessExecutionStep (e.g., scheduled(UUID id, String typeName, int order) or
similar named-constructor) that returns the record/instance with
StepStatus.SCHEDULED and the nullable fields set to null, or alternatively add a
builder (Lombok `@Builder` or manual) and use it here; then update the lambda
(.mapToObj(...)) to call ProcessExecutionStep.scheduled(steps.get(i).getId(),
steps.get(i).getType().getName(), i) (or the builder variant) so the call site
no longer passes six positional nulls.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.javamonitor-server/src/main/resources/application-local.yamlmonitor-server/src/main/resources/db/changelog/db.changelog-master.yamlmonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
🚧 Files skipped from review as they are similar to previous changes (5)
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
- monitor-server/src/main/resources/db/changelog/db.changelog-master.yaml
- monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java
- monitor-server/src/main/resources/application-local.yaml
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
Comment on lines
+33
to
36
| public void sendProcessRunMessage(UUID caseUuid, ProcessConfig processConfig, UUID executionId, String bindingName, String debugFileLocation) { | ||
| ProcessRunMessage<?> message = new ProcessRunMessage<>(executionId, caseUuid, processConfig, debugFileLocation); | ||
| publisher.send(bindingName, message); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of the 5-arg sendProcessRunMessage overload
rg -n --type java 'sendProcessRunMessage\s*\(\s*[^,]+\s*,\s*[^,]+\s*,\s*[^,]+\s*,\s*[^,]+\s*,\s*[^,]+\s*\)' -A 2 -B 2Repository: gridsuite/monitor-core
Length of output: 2037
🏁 Script executed:
#!/bin/bash
# Get directory structure to understand package layout
git ls-files "monitor-server/src/main/java/org/gridsuite/monitor/server/**" | grep -E "(NotificationService|MonitorService)" | head -20Repository: gridsuite/monitor-core
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Read the full NotificationService.java to understand the context
wc -l monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.javaRepository: gridsuite/monitor-core
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Read NotificationService.java around the method definition
cat -n monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java | head -37Repository: gridsuite/monitor-core
Length of output: 1817
🏁 Script executed:
#!/bin/bash
# Read MonitorService.java around the call site to see full context
sed -n '255,270p' monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java | cat -nRepository: gridsuite/monitor-core
Length of output: 680
🏁 Script executed:
#!/bin/bash
# Check if there are other call sites to this method
rg 'sendProcessRunMessage' monitor-server/src/main/java/org/gridsuite/monitor/server/services/ -B 1 -A 1Repository: gridsuite/monitor-core
Length of output: 2266
Fix parameter order mismatch in MonitorService call to the 5-parameter overload.
The call at MonitorService.java:264 passes arguments in the wrong positions for the two adjacent String parameters:
notificationService.sendProcessRunMessage(caseUuid, processConfig, execution.getId(), null, bindingName);But the method signature expects:
public void sendProcessRunMessage(..., String bindingName, String debugFileLocation)This swaps the parameters: null is passed to bindingName (causing publisher.send(null, ...)) and bindingName is passed to debugFileLocation. Correct the argument order:
Fix parameter order
-notificationService.sendProcessRunMessage(caseUuid, processConfig, execution.getId(), null, bindingName);
+notificationService.sendProcessRunMessage(caseUuid, processConfig, execution.getId(), bindingName, null);Alternatively, to avoid this fragility in the future, consider whether this call should use the 4-parameter overload instead and let the routing logic remain centralized in NotificationService.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java`
around lines 33 - 36, The MonitorService call to
NotificationService.sendProcessRunMessage passes the two String arguments in the
wrong order; update the call site in MonitorService where
notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), null, bindingName) is invoked to either swap the last two
args to notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), bindingName, null) so bindingName is used for publisher.send,
or replace the call with the 4-parameter overload
(notificationService.sendProcessRunMessage(caseUuid, processConfig,
execution.getId(), bindingName)) to centralize routing logic; ensure you modify
the call that references execution.getId() and bindingName accordingly.
|
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
1 participant
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.
PR Summary