Implement Strategy Design Pattern for process configs#95
Merged
Conversation
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared ChangesProcess Config Comparison and Handler Refactor
Sequence Diagram(s)sequenceDiagram
participant ProcessConfigController
participant ProcessConfigService
participant ProcessConfigRepository
participant ProcessConfigHandler
participant ProcessConfig
ProcessConfigController->>ProcessConfigService: compareProcessConfigs(uuid1, uuid2)
ProcessConfigService->>ProcessConfigRepository: findById(uuid1)
ProcessConfigRepository-->>ProcessConfigService: entity1
ProcessConfigService->>ProcessConfigRepository: findById(uuid2)
ProcessConfigRepository-->>ProcessConfigService: entity2
ProcessConfigService->>ProcessConfigHandler: toDto(entity1)
ProcessConfigHandler-->>ProcessConfigService: processConfig1
ProcessConfigService->>ProcessConfigHandler: toDto(entity2)
ProcessConfigHandler-->>ProcessConfigService: processConfig2
ProcessConfigService->>ProcessConfig: compareWith(processConfig2)
ProcessConfig-->>ProcessConfigService: ProcessConfigFieldComparison list
ProcessConfigService-->>ProcessConfigController: ProcessConfigComparison
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java`:
- Around line 37-42: The constructor for ProcessConfigService currently builds
processConfigHandlers with
Collectors.toMap(ProcessConfigHandler::getProcessType, Function.identity())
which will throw a cryptic IllegalStateException on duplicate ProcessType;
update the constructor to detect duplicates from the incoming listHandlers (or
supply a merge function) and throw a clear IllegalArgumentException indicating
the duplicated ProcessType and the conflicting handler classes. Specifically,
iterate or use a collector that checks duplicates keyed by
ProcessConfigHandler::getProcessType (or provide a (a,b) -> { throw new
IllegalArgumentException(...); } merge function) and include the handler class
names in the error message to make the failure explicit during initialization.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.java`:
- Line 42: Add the missing test for PROCESS_CONFIG_NOT_FOUND in
MonitorServerExceptionHandlerTest by following the established pattern used in
the existing test (lines ~32-40): add a new test case (or extend the existing
parameterized/assertion block) that constructs a MonitorServerException with
error code PROCESS_CONFIG_NOT_FOUND and asserts the handler maps it to the
expected HTTP status; import the PROCESS_CONFIG_NOT_FOUND constant and use the
same helper/assertion methods used elsewhere in this test class to keep
consistency.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/mappers/processconfig/AbstractProcessConfigMapperTest.java`:
- Around line 12-19: The AbstractProcessConfigMapperTest class is a stub with
only TODOs and must either be implemented or removed; implement a concrete
abstract test harness that validates the ProcessConfigMapper<C,E> contract by
creating an abstract test class AbstractProcessConfigMapperTest that defines
reusable test cases for the three methods (toEntity, toDto, updateEntityFromDto)
and requires subclasses to supply test fixtures and a concrete mapper instance
(e.g., abstract methods createDto(), createEntity(), and mapper()); write tests
that assert fields are mapped correctly from C to E (toEntity), from E to C
(toDto), and that updateEntityFromDto mutates an existing E appropriately
without losing unchanged fields; alternatively, if you prefer to defer, remove
this stub file entirely to avoid merging empty tests.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandlerTest.java`:
- Around line 12-26: The test stub ProcessConfigHandlerTest must be implemented
(or removed); add parameterized tests that validate the generic handler contract
methods (getProcessType, update, copyEntity, toEntity, toProcessConfig) using a
sample concrete handler, a test registry lookup for
ProcessType→ProcessConfigHandler to assert registration/retrieval, dedicated
unit tests for the process-specific computeDifferences behavior using distinct
entity pairs, and a negative test that requesting a handler for an unknown
ProcessType throws the expected error; locate and exercise the concrete handler
class used in production (the implementation referenced by the registry) and the
registry lookup code to drive these assertions.
🪄 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: a024e638-7aff-4472-848f-d9c01156adfb
📒 Files selected for processing (16)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerBusinessErrorCode.javamonitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/LoadFlowConfigMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/ProcessConfigMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/SecurityAnalysisConfigMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandler.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/mappers/processconfig/AbstractProcessConfigMapperTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.java
antoinebhs
reviewed
Jun 19, 2026
| Optional<ProcessConfigComparison> comparison = processConfigService.compareProcessConfigs(uuid1, uuid2); | ||
| return comparison.map(c -> ResponseEntity.status(HttpStatus.OK).body(c)).orElseGet(() -> ResponseEntity.status(HttpStatus.NOT_FOUND).build()); | ||
| ProcessConfigComparison comparison = processConfigService.compareProcessConfigs(uuid1, uuid2); | ||
| return ResponseEntity.ok().body(comparison); |
Contributor
There was a problem hiding this comment.
as discussed: maybe better to have this behavior change in another PR?
antoinebhs
reviewed
Jun 19, 2026
Comment on lines
+21
to
+74
| public abstract class AbstractProcessConfigHandler<C extends ProcessConfig, E extends ProcessConfigEntity, M extends ProcessConfigMapper<C, E>> | ||
| implements ProcessConfigHandler<C, E> { | ||
|
|
||
| protected final M mapper; | ||
|
|
||
| protected AbstractProcessConfigHandler(M mapper) { | ||
| this.mapper = mapper; | ||
| } | ||
|
|
||
| @Override | ||
| public void update(E entity, C processConfig) { | ||
| mapper.updateEntityFromDto(processConfig, entity); | ||
| } | ||
|
|
||
| @Override | ||
| public E copyEntity(E sourceEntity) { | ||
| return toEntity(toProcessConfig(sourceEntity)); | ||
| } | ||
|
|
||
| @Override | ||
| public E toEntity(C processConfig) { | ||
| return mapper.toEntity(processConfig); | ||
| } | ||
|
|
||
| @Override | ||
| public C toProcessConfig(E entity) { | ||
| return mapper.toDto(entity); | ||
| } | ||
|
|
||
| @Override | ||
| public List<ProcessConfigFieldComparison> computeDifferences(E entity1, E entity2) { | ||
| C config1 = toProcessConfig(entity1); | ||
| C config2 = toProcessConfig(entity2); | ||
| List<ProcessConfigFieldComparison> differences = new ArrayList<>(); | ||
|
|
||
| // Compare modifications | ||
| addFieldComparison(config1.modificationUuids(), config2.modificationUuids(), differences, "modifications"); | ||
|
|
||
| // Compare other fields | ||
| addProcessConfigSpecificFieldsComparison(config1, config2, differences); | ||
|
|
||
| return differences; | ||
| } | ||
|
|
||
| protected abstract void addProcessConfigSpecificFieldsComparison(C config1, C config2, List<ProcessConfigFieldComparison> differences); | ||
|
|
||
| protected void addFieldComparison(Object value1, Object value2, List<ProcessConfigFieldComparison> differences, String fieldName) { | ||
| differences.add(new ProcessConfigFieldComparison( | ||
| fieldName, | ||
| Objects.equals(value1, value2), | ||
| value1, | ||
| value2 | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
In my opinion, handler should be kept simple. I think it belongs to the process config object itself? Like the compare Object java method? I think I implemented something like that maybe. To discuss if you're not convinced!
antoinebhs
reviewed
Jun 19, 2026
|
|
||
| @Override | ||
| public List<ProcessConfigFieldComparison> computeDifferences(E entity1, E entity2) { | ||
| C config1 = toProcessConfig(entity1); |
Contributor
There was a problem hiding this comment.
Maybe throw directly if not the same type?
antoinebhs
reviewed
Jun 19, 2026
| .map(sourceEntity -> { | ||
| ProcessConfigEntity entity = getHandler(sourceEntity.getProcessType()).copyEntity(sourceEntity); | ||
| return processConfigRepository.save(entity).getId(); | ||
| }); // TODO: renvoyer l'erreur not found ici au lieu de se trimbaler un Optional ? |
Contributor
There was a problem hiding this comment.
If possible I would keep this behavior and base another PR on this to change that?
Contributor
|
For the tests, I would just test that it delegates correctly |
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
khouadrired
reviewed
Jun 23, 2026
| @Override | ||
| public C toProcessConfig(E entity) { | ||
| return mapper.toDto(entity); | ||
| } |
Contributor
There was a problem hiding this comment.
you can add here :
```
@Override
public final List<ProcessConfigFieldComparison> computeDifferences(E entity1, E entity2) {
C config1 = toProcessConfig(entity1);
C config2 = toProcessConfig(entity2);
List<ProcessConfigFieldComparison> differences = new ArrayList<>();
addSpecificDifferences(config1, config2, differences);
return differences;
}
protected abstract void addSpecificDifferences(C config1, C config2, List<ProcessConfigFieldComparison> differences);
protected void addModificationsDifference(C config1, C config2, List<ProcessConfigFieldComparison> differences) {
differences.add(new ProcessConfigFieldComparison(
"modifications",
Objects.equals(config1.modificationUuids(), config2.modificationUuids()),
config1.modificationUuids(),
config2.modificationUuids()
));
}
protected void addLoadFlowParametersDifference(UUID loadflowParametersUuid1, UUID loadflowParametersUuid2, List<ProcessConfigFieldComparison> differences) {
differences.add(new ProcessConfigFieldComparison(
"loadflowParameters",
Objects.equals(loadflowParametersUuid1, loadflowParametersUuid2),
loadflowParametersUuid1,
loadflowParametersUuid2
));
}
as this part of comparaison is duplicate
and implement in the handlers `addSpecificDifferences`
Contributor
There was a problem hiding this comment.
@khouadrired @carojeandat what do you think of a design like https://github.com/gridsuite/monitor-core/pull/78/changes#diff-fa68b4a3661783f0ad1197ea425a10db6f20e407287207230ce436af04b1daa5R28-R42 ?
comparison would be carried by the object instead of the handler which might more logical in terms of responsibility?
Contributor
There was a problem hiding this comment.
@khouadrired @carojeandat what do you think of a design like https://github.com/gridsuite/monitor-core/pull/78/changes#diff-fa68b4a3661783f0ad1197ea425a10db6f20e407287207230ce436af04b1daa5R28-R42 ? comparison would be carried by the object instead of the handler which might more logical in terms of responsibility?
yes i agree in term of responsibility we could move the comparaison the the process config model
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorExceptionHandler.java (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
handleMonitorServerExceptionto match this handler.This is
MonitorExceptionHandler(commons), not a server handler. The method name looks copied fromMonitorServerExceptionHandlerand is misleading. ConsiderhandleMonitorException.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorExceptionHandler.java` around lines 42 - 46, Rename the exception handler method in MonitorExceptionHandler from handleMonitorServerException to a name that matches the actual class and handled type, such as handleMonitorException. Update the method identifier only, keep the `@ExceptionHandler`(MonitorException.class) behavior and the call to super.handleDomainException(exception, request) unchanged, and make sure any references in the same class or tests use the new name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfig.java`:
- Around line 32-33: The compareWith contract in ProcessConfig is currently
ambiguous about null, and the implementations can hit an unexpected NPE when
other is null. Make the parameter explicitly non-null in the ProcessConfig
interface and update the compareWith implementations to fail fast with a
controlled exception before any access to other.processType(), keeping the
contract consistent across all implementations.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.java`:
- Around line 30-39: Rename the test method in MonitorServerExceptionHandlerTest
because it no longer validates a bad request mapping; the assertion now expects
NOT_FOUND for PROCESS_CONFIG_NOT_FOUND. Update
mapsBadRequestBusinessErrorToStatus to a name that reflects the actual behavior,
and keep the test logic in handleMonitorServerException unchanged.
---
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorExceptionHandler.java`:
- Around line 42-46: Rename the exception handler method in
MonitorExceptionHandler from handleMonitorServerException to a name that matches
the actual class and handled type, such as handleMonitorException. Update the
method identifier only, keep the `@ExceptionHandler`(MonitorException.class)
behavior and the call to super.handleDomainException(exception, request)
unchanged, and make sure any references in the same class or tests use the new
name.
🪄 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: 51f3b7bb-a8f2-4500-9701-229e7595fb6e
📒 Files selected for processing (25)
monitor-commons/pom.xmlmonitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorBusinessErrorCode.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorException.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorExceptionHandler.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfigFieldComparison.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfig.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/error/MonitorBusinessErrorCodeTest.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/error/MonitorExceptionHandlerTest.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfigTest.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfigTest.javamonitor-server/src/main/java/org/gridsuite/monitor/server/dto/processconfig/ProcessConfigComparison.javamonitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerBusinessErrorCode.javamonitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerException.javamonitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandler.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandlerTest.java
💤 Files with no reviewable changes (5)
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandler.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandler.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandlerTest.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandlerTest.java
✅ Files skipped from review due to trivial changes (3)
- monitor-commons/src/test/java/org/gridsuite/monitor/commons/error/MonitorBusinessErrorCodeTest.java
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/error/MonitorBusinessErrorCode.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/dto/processconfig/ProcessConfigComparison.java
Comment on lines
+32
to
+33
|
|
||
| List<ProcessConfigFieldComparison> compareWith(ProcessConfig other); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the compareWith contract explicitly non-null.
Both current implementations dereference other.processType() on the type-mismatch path, so compareWith(null) fails with an unexpected NPE instead of a controlled exception. Tighten the interface contract or fail fast before implementations reach that branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfig.java`
around lines 32 - 33, The compareWith contract in ProcessConfig is currently
ambiguous about null, and the implementations can hit an unexpected NPE when
other is null. Make the parameter explicitly non-null in the ProcessConfig
interface and update the compareWith implementations to fail fast with a
controlled exception before any access to other.processType(), keeping the
contract consistent across all implementations.
Contributor
|
need to fix sonar issues |
khouadrired
reviewed
Jun 25, 2026
|
|
||
| @Override | ||
| public List<ProcessConfigFieldComparison> compareWith(ProcessConfig other) { | ||
| if (!(other instanceof LoadFlowConfig o)) { |
Contributor
There was a problem hiding this comment.
move this check to the handler service, it well be duplicated if we have more services.
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
antoinebhs
reviewed
Jun 25, 2026
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-test</artifactId> |
Contributor
There was a problem hiding this comment.
do we need spring test here?
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.java (1)
31-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep type-mismatch handling explicit.
compareWithis part of the publicProcessConfigcontract, so using the cast as the mismatch check turns a normal domain validation into rawClassCastExceptionflow. That is what now forces the compare endpoint to special-case this low-level exception, and any other caller ofcompareWithinherits the same untyped failure mode. Please reject mismatchedprocessType()explicitly instead of relying on the cast.#!/bin/bash set -euo pipefail sed -n '28,40p' monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.java sed -n '32,46p' monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfig.java sed -n '114,129p' monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.javaExpected result: both
compareWithimplementations rely on direct casts, andProcessConfigService.compareProcessConfigs(...)reaches them without an explicit process-type guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.java` around lines 31 - 32, `LoadFlowConfig.compareWith` currently uses the downcast itself as the type check, which turns mismatched configs into a raw `ClassCastException`; make the mismatch explicit by checking `processType()` before casting and rejecting non-matching `ProcessConfig` instances with a clear domain failure. Apply the same pattern in the `compareWith` implementations referenced by `ProcessConfigService.compareProcessConfigs(...)` so callers don’t depend on low-level cast exceptions for validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.java`:
- Around line 31-32: `LoadFlowConfig.compareWith` currently uses the downcast
itself as the type check, which turns mismatched configs into a raw
`ClassCastException`; make the mismatch explicit by checking `processType()`
before casting and rejecting non-matching `ProcessConfig` instances with a clear
domain failure. Apply the same pattern in the `compareWith` implementations
referenced by `ProcessConfigService.compareProcessConfigs(...)` so callers don’t
depend on low-level cast exceptions for validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 044bf62d-51b0-4189-8789-568fa9e1e128
📒 Files selected for processing (8)
monitor-commons/pom.xmlmonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfig.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfigTest.javamonitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfigTest.javamonitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.java
💤 Files with no reviewable changes (1)
- monitor-commons/pom.xml
antoinebhs
reviewed
Jun 25, 2026
| <version>2.2.22</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Pull request overview
This PR refactors process-config handling to a Strategy-based design by introducing per-ProcessType handlers, centralizing mapping/update/copy operations behind a ProcessConfigHandler interface. It also moves process-config comparison logic into the monitor-commons ProcessConfig types and adjusts API/test behavior accordingly.
Changes:
- Introduce
ProcessConfigHandler+AbstractProcessConfigHandlerand concrete handlers for LoadFlow and SecurityAnalysis. - Update
ProcessConfigServiceto route CRUD/duplicate/compare operations through handlers instead ofswitchlogic. - Add/adjust tests across
monitor-serverandmonitor-commons, and moveProcessConfigFieldComparisoninto commons.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java | Replaces switch logic with handler lookup and delegates CRUD/compare logic to handlers and commons compareWith. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.java | New strategy interface for mapping/updating/copying configs by process type. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandler.java | Shared base implementation for handlers using MapStruct mappers. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandler.java | Concrete handler for LoadFlow configs. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandler.java | Concrete handler for SecurityAnalysis configs. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/ProcessConfigMapper.java | New generic mapper interface for process-config DTO/entity conversion. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/LoadFlowConfigMapper.java | Extends the new generic mapper contract. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/SecurityAnalysisConfigMapper.java | Extends the new generic mapper contract. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.java | Changes compare endpoint to return 400 on type mismatch via exception catching. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/dto/processconfig/ProcessConfigComparison.java | Updates to reference ProcessConfigFieldComparison from commons. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandler.java | Updates business error -> HTTP status mapping. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerException.java | Header/comment formatting update. |
| monitor-server/src/main/java/org/gridsuite/monitor/server/error/MonitorServerBusinessErrorCode.java | Replaces previous business error code with PROCESS_CONFIG_NOT_FOUND. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.java | Refactors tests to use handler strategy and new compare behavior. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandlerTest.java | Adds reusable tests for handler behavior. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandlerTest.java | Adds handler tests for LoadFlow. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandlerTest.java | Adds handler tests for SecurityAnalysis. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.java | Updates controller tests for compare error behavior. |
| monitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.java | Updates expected status/code; test name now slightly misleading. |
| monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfig.java | Adds compareWith to the domain contract. |
| monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfig.java | Implements compareWith for LoadFlow. |
| monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfig.java | Implements compareWith for SecurityAnalysis. |
| monitor-commons/src/main/java/org/gridsuite/monitor/commons/types/processconfig/ProcessConfigFieldComparison.java | Moves field-comparison DTO into commons. |
| monitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/LoadFlowConfigTest.java | Adds tests for compareWith. |
| monitor-commons/src/test/java/org/gridsuite/monitor/commons/types/processconfig/SecurityAnalysisConfigTest.java | Adds tests for compareWith. |
| monitor-commons/pom.xml | Adds test dependencies needed by new commons tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
122
to
126
| ProcessConfig processConfig1 = getHandler(processConfigEntity1.get().getProcessType()).toProcessConfig(processConfigEntity1.get()); | ||
| ProcessConfig processConfig2 = getHandler(processConfigEntity2.get().getProcessType()).toProcessConfig(processConfigEntity2.get()); | ||
|
|
||
| List<ProcessConfigFieldComparison> differences = processConfig1.compareWith(processConfig2); | ||
| boolean identical = differences.stream().allMatch(ProcessConfigFieldComparison::identical); |
Comment on lines
+137
to
+142
| try { | ||
| Optional<ProcessConfigComparison> comparison = processConfigService.compareProcessConfigs(uuid1, uuid2); | ||
| return comparison.map(c -> ResponseEntity.status(HttpStatus.OK).body(c)).orElseGet(() -> ResponseEntity.status(HttpStatus.NOT_FOUND).build()); | ||
| } catch (ClassCastException e) { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); | ||
| } |
Comment on lines
+33
to
+46
| public List<ProcessConfigFieldComparison> compareWith(ProcessConfig other) { | ||
| SecurityAnalysisConfig o = (SecurityAnalysisConfig) other; | ||
| return List.of( | ||
| new ProcessConfigFieldComparison("modifications", | ||
| Objects.equals(this.modificationUuids, o.modificationUuids), | ||
| this.modificationUuids, o.modificationUuids), | ||
| new ProcessConfigFieldComparison("securityAnalysisParameters", | ||
| Objects.equals(this.securityAnalysisParametersUuid, o.securityAnalysisParametersUuid), | ||
| this.securityAnalysisParametersUuid, o.securityAnalysisParametersUuid), | ||
| new ProcessConfigFieldComparison("loadflowParameters", | ||
| Objects.equals(this.loadflowParametersUuid, o.loadflowParametersUuid), | ||
| this.loadflowParametersUuid, o.loadflowParametersUuid) | ||
| ); | ||
| } |
Comment on lines
+31
to
+41
| public List<ProcessConfigFieldComparison> compareWith(ProcessConfig other) { | ||
| LoadFlowConfig o = (LoadFlowConfig) other; | ||
| return List.of( | ||
| new ProcessConfigFieldComparison("modifications", | ||
| Objects.equals(this.modificationUuids, o.modificationUuids), | ||
| this.modificationUuids, o.modificationUuids), | ||
| new ProcessConfigFieldComparison("loadflowParameters", | ||
| Objects.equals(this.loadflowParametersUuid, o.loadflowParametersUuid), | ||
| this.loadflowParametersUuid, o.loadflowParametersUuid) | ||
| ); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
antoinebhs
reviewed
Jun 26, 2026
| protected HttpStatus mapStatus(MonitorServerBusinessErrorCode errorCode) { | ||
| return switch (errorCode) { | ||
| case DIFFERENT_PROCESS_CONFIG_TYPE -> HttpStatus.BAD_REQUEST; | ||
| case PROCESS_CONFIG_NOT_FOUND -> HttpStatus.NOT_FOUND; |
antoinebhs
reviewed
Jun 26, 2026
| ProcessConfig processConfig1 = toProcessConfig(processConfigEntity1.get()); | ||
| ProcessConfig processConfig2 = toProcessConfig(processConfigEntity2.get()); | ||
|
|
||
| if (processConfig1.processType() != processConfig2.processType()) { |
Contributor
There was a problem hiding this comment.
I would prefer to keep the original behavior with the exception we had before
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
|
|
||
| when(processConfigService.compareProcessConfigs(any(), any())) | ||
| .thenThrow(new MonitorServerException(DIFFERENT_PROCESS_CONFIG_TYPE, "Cannot compare different process config types")); | ||
| .thenThrow(new ClassCastException()); |
antoinebhs
reviewed
Jun 26, 2026
|
|
||
| @Test | ||
| void mapsBadRequestBusinessErrorToStatus() { | ||
| void mapsNotFoundBusinessErrorToStatus() { |
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
antoinebhs
reviewed
Jun 26, 2026
|
|
||
| Optional<ProcessConfigComparison> comparison = processConfigService.compareProcessConfigs(uuid1, uuid2); | ||
| return comparison.map(c -> ResponseEntity.status(HttpStatus.OK).body(c)).orElseGet(() -> ResponseEntity.status(HttpStatus.NOT_FOUND).build()); | ||
| try { |
antoinebhs
reviewed
Jun 26, 2026
| */ | ||
| public enum MonitorServerBusinessErrorCode implements BusinessErrorCode { | ||
| DIFFERENT_PROCESS_CONFIG_TYPE("monitor.server.differentProcessConfigType"); | ||
| PROCESS_CONFIG_NOT_FOUND("monitor.server.processConfigNotFound"); |
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandlerTest.java (1)
57-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact DTO forwarded during copy.
Line 70 uses
any(), so this test still passes ifcopyEntity()sends a different DTO than the one returned bymapper.toDto(...). VerifyingprocessConfigdirectly makes the delegation check meaningful.Proposed change
- verify(mapper).toEntity(any()); + verify(mapper).toEntity(processConfig);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandlerTest.java` around lines 57 - 71, The copyEntityTest in AbstractProcessConfigHandlerTest is too weak because verify(mapper).toEntity(any()) does not নিশ্চিত that the DTO produced by mapper.toDto(...) is the one passed through. Update the verification to assert the exact processConfig instance is forwarded from copyEntity(), keeping the existing stubbing of mapper.toDto(processConfigEntity1) and mapper.toEntity(processConfig) so the delegation is checked precisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandlerTest.java`:
- Around line 57-71: The copyEntityTest in AbstractProcessConfigHandlerTest is
too weak because verify(mapper).toEntity(any()) does not নিশ্চিত that the DTO
produced by mapper.toDto(...) is the one passed through. Update the verification
to assert the exact processConfig instance is forwarded from copyEntity(),
keeping the existing stubbing of mapper.toDto(processConfigEntity1) and
mapper.toEntity(processConfig) so the delegation is checked precisely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e80d0d6-fd6a-49d1-807d-3be49aed81c7
📒 Files selected for processing (9)
monitor-commons/pom.xmlmonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.javamonitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/AbstractProcessConfigHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandlerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandlerTest.java
💤 Files with no reviewable changes (1)
- monitor-commons/pom.xml
✅ Files skipped from review due to trivial changes (1)
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/SecurityAnalysisConfigHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigHandler.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/error/MonitorServerExceptionHandlerTest.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/LoadFlowConfigHandlerTest.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
antoinebhs
approved these changes
Jun 26, 2026
|
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
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