Move Dynamic Simulation Params to Dynamic Simulation server#167
Open
Move Dynamic Simulation Params to Dynamic Simulation server#167
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
📝 WalkthroughWalkthroughParameters are persisted as new JPA entities and exposed via a full CRUD REST API; simulations now accept a parameters UUID plus a separate events list. Controller/service layers load parameters by UUID, serialize events into run context, and the DB schema and repositories were added/updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Database
Client->>Controller: POST /v1/networks/{networkUuid}/run<br/>(parametersUuid, events)
activate Controller
Controller->>Service: createRunContext(networkUuid, variantId, receiver, reportContext, userId, parametersUuid, events, debug)
activate Service
Service->>Repository: findById(parametersUuid)
activate Repository
Repository->>Database: SELECT * FROM dynamic_simulation_parameters WHERE id=?
Database-->>Repository: DynamicSimulationParametersEntity
deactivate Repository
Repository-->>Service: entity
Service->>Service: entity.toDto() and set events on runContext
Service-->>Controller: DynamicSimulationRunContext
deactivate Service
Controller->>Service: start simulation worker (uses runContext)
Service->>Controller: result UUID
Controller-->>Client: 200 OK (result UUID)
deactivate Controller
sequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Database
Client->>Controller: POST /v1/parameters (DynamicSimulationParametersInfos)
activate Controller
Controller->>Service: createParameters(parametersInfos)
activate Service
Service->>Repository: save(new DynamicSimulationParametersEntity)
activate Repository
Repository->>Database: INSERT dynamic_simulation_parameters (+ solver/network/curve via cascade)
Database-->>Repository: persisted entity with id
deactivate Repository
Repository-->>Service: persisted entity
Service-->>Controller: UUID
Controller-->>Client: 200 OK (UUID)
deactivate Controller
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![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
🧹 Nitpick comments (3)
src/main/java/org/gridsuite/ds/server/dto/curve/CurveInfos.java (1)
11-11: Consider using explicit imports instead of wildcard import.Using
lombok.*;works but explicit imports (@NoArgsConstructor,@AllArgsConstructor,@Getter,@Setter,@Builder) improve readability and make dependencies clearer at a glance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/ds/server/dto/curve/CurveInfos.java` at line 11, Replace the wildcard lombok import in CurveInfos with explicit annotation imports to improve readability and clarity: remove "import lombok.*;" and add specific imports for the annotations used on the class (e.g., NoArgsConstructor, AllArgsConstructor, Getter, Setter, Builder) so the class declaration (CurveInfos) references those named imports instead of a wildcard.src/main/java/org/gridsuite/ds/server/dto/DynamicSimulationParametersInfos.java (1)
24-31: Consider adding@Builder.Defaultfor collection fields.With
@Builderon a class that hasListfields (solvers,curves), these fields will benullby default when using the builder unless explicitly set. If an empty list is the preferred default, consider using@Builder.Default:`@Builder.Default` private List<SolverInfos> solvers = new ArrayList<>(); `@Builder.Default` private List<CurveInfos> curves = new ArrayList<>();This prevents potential
NullPointerExceptionwhen iterating over these collections if not set during construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/ds/server/dto/DynamicSimulationParametersInfos.java` around lines 24 - 31, DynamicSimulationParametersInfos has List fields (solvers, curves) that will be null when using Lombok `@Builder`; update those fields to use `@Builder.Default` and initialize them to empty lists (e.g., add `@Builder.Default` private List<SolverInfos> solvers = new ArrayList<>(); and similarly for curves) so instances created via the builder have non-null empty collections and avoid NullPointerException when iterating.src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java (1)
77-84: Consider making the events header optional to align with defensive coding patterns.The code using events (
getEventModel()at line 113) explicitly handles empty/null cases by returningCollections.emptyList(). However,getNonNullHeader()will throw an exception ifHEADER_EVENTSis missing. To match the defensive pattern used elsewhere in the codebase (e.g.,getCurveModel()at line 126) and improve robustness for edge cases:Suggested change
- String eventsJson = getNonNullHeader(headers, HEADER_EVENTS); - List<EventInfos> events; - try { - events = objectMapper.readValue(eventsJson, new TypeReference<>() { }); - } catch (JsonProcessingException e) { - throw new UncheckedIOException(e); - } + String eventsJson = (String) headers.get(HEADER_EVENTS); + List<EventInfos> events = Collections.emptyList(); + if (eventsJson != null && !eventsJson.isEmpty()) { + try { + events = objectMapper.readValue(eventsJson, new TypeReference<>() { }); + } catch (JsonProcessingException e) { + throw new UncheckedIOException(e); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java` around lines 77 - 84, The events header should be treated as optional: replace the call to getNonNullHeader(headers, HEADER_EVENTS) with a nullable header lookup (e.g., getHeader or a null-safe utility) and only call objectMapper.readValue when the header value is non-null/non-empty; if missing or blank, set runContext.setEvents(Collections.emptyList()) so getEventModel() can continue to return an empty list. Keep references to HEADER_EVENTS, objectMapper.readValue(..., new TypeReference<>() { }), and runContext.setEvents(...) when implementing this null-safe flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/DynamicSimulationParametersEntity.java`:
- Around line 103-110: The code currently assumes non-null child IDs belong to
this aggregate before calling existingEntity.update(...); modify the loop that
iterates solverInfosList to first check
solversByIdMap.containsKey(solverInfos.getId()) when solverInfos.getId() != null
and if not present throw or return a controlled 4xx/domain-specific exception
(e.g., InvalidChildReferenceException) instead of dereferencing null or silently
overwriting; apply the same membership validation pattern for the analogous
loops handling networks and curves (use their maps and corresponding Entity
classes and update(...) calls) so any unknown child UUIDs are rejected before
calling update.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.java`:
- Around line 143-145: The update(SolverInfos) in SolverParametersEntity casts
to AbstractSolverInfos and calls assignAttributes(AbstractSolverInfos), causing
wrong overload resolution and losing solver-specific fields; fix by removing the
cast and implementing proper per-subclass dispatch: either make
update(SolverInfos) abstract in SolverParametersEntity and implement
update(SolverInfos) in IdaSolverParametersEntity and SimSolverParametersEntity
to call assignAttributes(IdaSolverInfos) and assignAttributes(SimSolverInfos)
respectively, or override update(SolverInfos) in both subclasses so they cast to
their concrete SolverInfos type and call their specific assignAttributes
overloads (e.g., assignAttributes(IdaSolverInfos),
assignAttributes(SimSolverInfos)) to ensure subtype fields like order, initStep,
minStep, maxStep, relAccuracy, hMin, hMax, kReduceStep are updated.
In
`@src/main/java/org/gridsuite/ds/server/service/parameters/impl/ParametersServiceImpl.java`:
- Around line 330-334: getParametersValues (and the other overloaded variant
near the 378-386 region) returns parameter values for a persisted parameters
record but does not guard against a null mapping created by
createDefaultParameters; this causes value generation to fail. Fix by either (A)
in getParametersValues(DynamicSimulationParametersInfos, Network) and the other
values-entrypoint check parametersInfos.getMapping() and if null throw the
existing MAPPING_NOT_PROVIDED error used in createRunContext, or (B) change
createDefaultParameters to assign a valid default mapping when persisting the
record so downstream calls to getParametersValues/doGetParameters always see a
non-null mapping. Use the existing MAPPING_NOT_PROVIDED symbol and the
doGetParameters/createDefaultParameters/getParametersValues function names to
locate the spots to change.
- Around line 139-146: Validate the selected solver and the solvers list before
attempting to serialize: in ParametersServiceImpl where you compute inputSolver
from inputParameters.getSolvers().stream() and use inputParameters.getSolver(),
first check that inputParameters.getSolvers() is non-null and non-empty and that
inputParameters.getSolver() (the selected solver id/type) is present; if any
check fails, fail fast by throwing a clear exception (e.g.,
IllegalArgumentException or a domain-specific exception) rather than continuing
to serialize or dereference nulls, so avoid proceeding to
ByteArrayOutputStream/XmlSerializableParameter.writeParameter/ParametersXml.load/dynawoSimulationParameters.setSolverType/setSolverParameters
when inputSolver is null or inputs are invalid.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/ds/server/dto/curve/CurveInfos.java`:
- Line 11: Replace the wildcard lombok import in CurveInfos with explicit
annotation imports to improve readability and clarity: remove "import lombok.*;"
and add specific imports for the annotations used on the class (e.g.,
NoArgsConstructor, AllArgsConstructor, Getter, Setter, Builder) so the class
declaration (CurveInfos) references those named imports instead of a wildcard.
In
`@src/main/java/org/gridsuite/ds/server/dto/DynamicSimulationParametersInfos.java`:
- Around line 24-31: DynamicSimulationParametersInfos has List fields (solvers,
curves) that will be null when using Lombok `@Builder`; update those fields to use
`@Builder.Default` and initialize them to empty lists (e.g., add `@Builder.Default`
private List<SolverInfos> solvers = new ArrayList<>(); and similarly for curves)
so instances created via the builder have non-null empty collections and avoid
NullPointerException when iterating.
In
`@src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java`:
- Around line 77-84: The events header should be treated as optional: replace
the call to getNonNullHeader(headers, HEADER_EVENTS) with a nullable header
lookup (e.g., getHeader or a null-safe utility) and only call
objectMapper.readValue when the header value is non-null/non-empty; if missing
or blank, set runContext.setEvents(Collections.emptyList()) so getEventModel()
can continue to return an empty list. Keep references to HEADER_EVENTS,
objectMapper.readValue(..., new TypeReference<>() { }), and
runContext.setEvents(...) when implementing this null-safe flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7be6a3d3-17de-4013-819d-af6f65614370
📒 Files selected for processing (37)
src/main/java/org/gridsuite/ds/server/controller/DynamicSimulationController.javasrc/main/java/org/gridsuite/ds/server/controller/DynamicSimulationParametersController.javasrc/main/java/org/gridsuite/ds/server/dto/DynamicSimulationParametersInfos.javasrc/main/java/org/gridsuite/ds/server/dto/curve/CurveInfos.javasrc/main/java/org/gridsuite/ds/server/dto/network/NetworkInfos.javasrc/main/java/org/gridsuite/ds/server/dto/solver/AbstractSolverInfos.javasrc/main/java/org/gridsuite/ds/server/dto/solver/IdaSolverInfos.javasrc/main/java/org/gridsuite/ds/server/dto/solver/SimSolverInfos.javasrc/main/java/org/gridsuite/ds/server/dto/solver/SolverInfos.javasrc/main/java/org/gridsuite/ds/server/dto/solver/SolverTypeInfos.javasrc/main/java/org/gridsuite/ds/server/entities/AbstractManuallyAssignedIdentifierEntity.javasrc/main/java/org/gridsuite/ds/server/entities/ResultEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/DynamicSimulationParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/curve/CurveEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/network/NetworkParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/IdaSolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/SimSolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/repository/DynamicSimulationParametersRepository.javasrc/main/java/org/gridsuite/ds/server/repository/ResultRepository.javasrc/main/java/org/gridsuite/ds/server/service/DynamicSimulationResultService.javasrc/main/java/org/gridsuite/ds/server/service/DynamicSimulationWorkerService.javasrc/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.javasrc/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationRunContext.javasrc/main/java/org/gridsuite/ds/server/service/parameters/ParameterUtils.javasrc/main/java/org/gridsuite/ds/server/service/parameters/ParametersService.javasrc/main/java/org/gridsuite/ds/server/service/parameters/impl/ParametersServiceImpl.javasrc/main/resources/db/changelog/changesets/changelog_20260318T185133Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/ds/server/controller/AbstractDynamicSimulationControllerTest.javasrc/test/java/org/gridsuite/ds/server/controller/DynamicSimulationControllerIEEE14Test.javasrc/test/java/org/gridsuite/ds/server/controller/DynamicSimulationControllerTest.javasrc/test/java/org/gridsuite/ds/server/controller/DynamicSimulationParametersControllerIEEE14Test.javasrc/test/java/org/gridsuite/ds/server/controller/DynamicSimulationParametersControllerTest.javasrc/test/java/org/gridsuite/ds/server/controller/utils/ParameterTestUtils.javasrc/test/java/org/gridsuite/ds/server/dto/XmlSerializableParameterTest.javasrc/test/java/org/gridsuite/ds/server/service/DynamicSimulationResultServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/ds/server/dto/solver/SolverTypeInfos.java
Comment on lines
+103
to
+110
| Map<UUID, SolverParametersEntity> solversByIdMap = solvers.stream().collect(Collectors.toMap(SolverParametersEntity::getId, solverEntity -> solverEntity)); | ||
|
|
||
| // merge existing and add new solvers | ||
| List<SolverParametersEntity> mergedSolvers = new ArrayList<>(); | ||
| for (SolverInfos solverInfos : solverInfosList) { | ||
| if (solverInfos.getId() != null) { | ||
| SolverParametersEntity existingEntity = solversByIdMap.get(solverInfos.getId()); | ||
| existingEntity.update(solverInfos); |
There was a problem hiding this comment.
Reject unknown child UUIDs before updating nested entities.
Line 109, Line 129, and Line 148 assume every non-null child id already belongs to this aggregate. A stale/foreign solver, network, or curve id will either dereference null or overwrite the current network instead of returning a controlled 4xx/domain error. Please validate child membership before calling update(...), and reject unknown nested ids explicitly.
Also applies to: 128-131, 142-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/DynamicSimulationParametersEntity.java`
around lines 103 - 110, The code currently assumes non-null child IDs belong to
this aggregate before calling existingEntity.update(...); modify the loop that
iterates solverInfosList to first check
solversByIdMap.containsKey(solverInfos.getId()) when solverInfos.getId() != null
and if not present throw or return a controlled 4xx/domain-specific exception
(e.g., InvalidChildReferenceException) instead of dereferencing null or silently
overwriting; apply the same membership validation pattern for the analogous
loops handling networks and curves (use their maps and corresponding Entity
classes and update(...) calls) so any unknown child UUIDs are rejected before
calling update.
src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.java
Outdated
Show resolved
Hide resolved
Comment on lines
139
to
146
| // solver from input parameter | ||
| SolverInfos inputSolver = inputParameters.getSolvers().stream().filter(elem -> elem.getId().equals(inputParameters.getSolverId())).findFirst().orElse(null); | ||
| SolverInfos inputSolver = inputParameters.getSolvers().stream().filter(elem -> elem.getType() == inputParameters.getSolver()).findFirst().orElse(null); | ||
| if (inputSolver != null) { | ||
| ByteArrayOutputStream os = new ByteArrayOutputStream(); | ||
| XmlSerializableParameter.writeParameter(os, XmlSerializableParameter.PARAMETER_SET, inputSolver); | ||
| ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getId()); | ||
| dynawoSimulationParameters.setSolverType(inputSolver.getType().toSolverType()); | ||
| ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getType().name()); | ||
| dynawoSimulationParameters.setSolverType(inputSolver.getType()); | ||
| dynawoSimulationParameters.setSolverParameters(solverParameters); |
There was a problem hiding this comment.
Fail fast when the selected solver is missing.
Line 140 assumes inputParameters.getSolvers() is non-null and contains the solver referenced by inputParameters.getSolver(). With the new CRUD API, persisted rows can violate that assumption, which makes this either NPE or serializes Dynawo parameters without any solver block. Please validate solver, solvers, and the selected entry before continuing.
💡 Proposed guard
- SolverInfos inputSolver = inputParameters.getSolvers().stream().filter(elem -> elem.getType() == inputParameters.getSolver()).findFirst().orElse(null);
- if (inputSolver != null) {
+ List<SolverInfos> configuredSolvers = Optional.ofNullable(inputParameters.getSolvers())
+ .orElseThrow(() -> new IllegalArgumentException("No solver definitions configured"));
+ SolverInfos inputSolver = configuredSolvers.stream()
+ .filter(elem -> elem.getType() == inputParameters.getSolver())
+ .findFirst()
+ .orElseThrow(() -> new IllegalArgumentException("Selected solver is missing from solvers list"));
+ {
ByteArrayOutputStream os = new ByteArrayOutputStream();
XmlSerializableParameter.writeParameter(os, XmlSerializableParameter.PARAMETER_SET, inputSolver);
ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getType().name());
dynawoSimulationParameters.setSolverType(inputSolver.getType());
dynawoSimulationParameters.setSolverParameters(solverParameters);
}📝 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
| // solver from input parameter | |
| SolverInfos inputSolver = inputParameters.getSolvers().stream().filter(elem -> elem.getId().equals(inputParameters.getSolverId())).findFirst().orElse(null); | |
| SolverInfos inputSolver = inputParameters.getSolvers().stream().filter(elem -> elem.getType() == inputParameters.getSolver()).findFirst().orElse(null); | |
| if (inputSolver != null) { | |
| ByteArrayOutputStream os = new ByteArrayOutputStream(); | |
| XmlSerializableParameter.writeParameter(os, XmlSerializableParameter.PARAMETER_SET, inputSolver); | |
| ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getId()); | |
| dynawoSimulationParameters.setSolverType(inputSolver.getType().toSolverType()); | |
| ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getType().name()); | |
| dynawoSimulationParameters.setSolverType(inputSolver.getType()); | |
| dynawoSimulationParameters.setSolverParameters(solverParameters); | |
| // solver from input parameter | |
| List<SolverInfos> configuredSolvers = Optional.ofNullable(inputParameters.getSolvers()) | |
| .orElseThrow(() -> new IllegalArgumentException("No solver definitions configured")); | |
| SolverInfos inputSolver = configuredSolvers.stream() | |
| .filter(elem -> elem.getType() == inputParameters.getSolver()) | |
| .findFirst() | |
| .orElseThrow(() -> new IllegalArgumentException("Selected solver is missing from solvers list")); | |
| { | |
| ByteArrayOutputStream os = new ByteArrayOutputStream(); | |
| XmlSerializableParameter.writeParameter(os, XmlSerializableParameter.PARAMETER_SET, inputSolver); | |
| ParametersSet solverParameters = ParametersXml.load(new ByteArrayInputStream(os.toByteArray()), inputSolver.getType().name()); | |
| dynawoSimulationParameters.setSolverType(inputSolver.getType()); | |
| dynawoSimulationParameters.setSolverParameters(solverParameters); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/service/parameters/impl/ParametersServiceImpl.java`
around lines 139 - 146, Validate the selected solver and the solvers list before
attempting to serialize: in ParametersServiceImpl where you compute inputSolver
from inputParameters.getSolvers().stream() and use inputParameters.getSolver(),
first check that inputParameters.getSolvers() is non-null and non-empty and that
inputParameters.getSolver() (the selected solver id/type) is present; if any
check fails, fail fast by throwing a clear exception (e.g.,
IllegalArgumentException or a domain-specific exception) rather than continuing
to serialize or dereference nulls, so avoid proceeding to
ByteArrayOutputStream/XmlSerializableParameter.writeParameter/ParametersXml.load/dynawoSimulationParameters.setSolverType/setSolverParameters
when inputSolver is null or inputs are invalid.
Comment on lines
+330
to
334
| public DynamicSimulationParametersValues getParametersValues(UUID parametersUuid, UUID networkUuid, String variantId) { | ||
| Network network = getNetwork(networkUuid, variantId); | ||
| DynamicSimulationParametersInfos parametersInfos = doGetParameters(parametersUuid); | ||
|
|
||
| return getParametersValues(parametersInfos, network); |
There was a problem hiding this comment.
/default now produces parameter sets that /values cannot resolve.
createDefaultParameters() persists defaults without a mapping, but the new UUID-based /values path routes those records into value generation anyway. A freshly created default parameters set can therefore fail as soon as the client calls /v1/parameters/{uuid}/values, because the downstream mapping export/load path still expects a non-null mapping. Please either reject missing mappings here with the same MAPPING_NOT_PROVIDED error used in createRunContext, or assign a default mapping when creating the record.
Also applies to: 378-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/service/parameters/impl/ParametersServiceImpl.java`
around lines 330 - 334, getParametersValues (and the other overloaded variant
near the 378-386 region) returns parameter values for a persisted parameters
record but does not guard against a null mapping created by
createDefaultParameters; this causes value generation to fail. Fix by either (A)
in getParametersValues(DynamicSimulationParametersInfos, Network) and the other
values-entrypoint check parametersInfos.getMapping() and if null throw the
existing MAPPING_NOT_PROVIDED error used in createRunContext, or (B) change
createDefaultParameters to assign a valid default mapping when persisting the
record so downstream calls to getParametersValues/doGetParameters always see a
non-null mapping. Use the existing MAPPING_NOT_PROVIDED symbol and the
doGetParameters/createDefaultParameters/getParametersValues function names to
locate the spots to change.
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/gridsuite/ds/server/dto/event/EventInfos.java`:
- Around line 30-32: The EventInfos class currently marks the UUID field
`private UUID id` with both `@JsonProperty`("uuid") and `@JsonIgnore` which cancels
serialization/deserialization; update the annotations on `id` in EventInfos to
allow round-tripping (either remove `@JsonIgnore` or replace the annotation with
`@JsonProperty`(value = "uuid", access = JsonProperty.Access.READ_WRITE)) so
DynamicSimulationResultContext can serialize and deserialize the `uuid` field
correctly.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/IdaSolverParametersEntity.java`:
- Around line 61-63: The update(SolverInfos) method currently performs an
unchecked cast to IdaSolverInfos via assignAttributes((IdaSolverInfos)
solverInfos); guard this by checking the runtime type of solverInfos (e.g., if
(!(solverInfos instanceof IdaSolverInfos)) ) and throw a clear, controlled
validation exception (IllegalArgumentException or your domain
ValidationException) with a descriptive message; otherwise cast safely
(IdaSolverInfos ida = (IdaSolverInfos) solverInfos) and call
assignAttributes(ida). Ensure the exception type and message align with existing
validation/error handling in the service so callers receive a proper 4xx
response rather than a 500.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.java`:
- Around line 175-181: Add a null guard at the start of
SolverParametersEntity.fromDto(SolverInfos) to fail fast when solverInfos is
null: check if solverInfos == null and throw an IllegalArgumentException with a
clear message (e.g. "solverInfos must not be null") before the existing
instanceof branches (so the method either handles IdaSolverInfos/SimSolverInfos
or immediately rejects null).
- Around line 113-118: The assignAttributes method currently sets the entity's
type from the incoming DTO (type = solverInfos.getType()), but for a
discriminator-backed hierarchy the type must come from the concrete entity
class; change assignAttributes in SolverParametersEntity to stop copying
solverInfos.getType() and instead assign type from a protected/abstract
getSolverType() called on this instance (e.g., type = getSolverType()), then
implement getSolverType() in each concrete subclass (IdaSolverParametersEntity,
SimSolverParametersEntity) to return their correct SolverType; ensure no other
code paths overwrite type from the DTO.
In
`@src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java`:
- Around line 77-85: The code calls headers.get(HEADER_EVENTS) into eventsJson
and immediately does eventsJson.toString(), which will NPE if the header is
missing; update the block in DynamicSimulationResultContext to first check if
headers.get(HEADER_EVENTS) (HEADER_EVENTS / eventsJson) is null and in that case
set events to Collections.emptyList() (or call a getNonNullHeader helper) before
calling objectMapper.readValue; otherwise parse using
objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { }) and
keep the existing JsonProcessingException -> UncheckedIOException handling, then
call runContext.setEvents(events).
- Around line 91-99: The getSpecificMsgHeaders method currently serializes
getRunContext().getEvents() directly which can be null; change it to normalize
null to an empty list before serialization (e.g., assign a local events variable
= getRunContext().getEvents() != null ? getRunContext().getEvents() :
Collections.emptyList()) and then call objectMapper.writeValueAsString on that
normalized list so HEADER_EVENTS always contains "[]" instead of null after
deserialization; update references in getSpecificMsgHeaders accordingly (method:
getSpecificMsgHeaders, call site: getRunContext().getEvents(), header key:
HEADER_EVENTS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64cb943c-6d7e-49a9-9999-b9874ec3a23c
📒 Files selected for processing (5)
src/main/java/org/gridsuite/ds/server/dto/event/EventInfos.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/IdaSolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/SimSolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.javasrc/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SimSolverParametersEntity.java
Comment on lines
30
to
32
| @JsonProperty("uuid") | ||
| @JsonIgnore | ||
| private UUID id; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the files mentioned in the review
find . -type f -name "EventInfos.java" | head -5
find . -type f -name "DynamicSimulationResultContext.java" | head -5Repository: gridsuite/dynamic-simulation-server
Length of output: 237
🏁 Script executed:
# Check repository structure
git ls-files | grep -E "(EventInfos|DynamicSimulationResultContext)" | head -20Repository: gridsuite/dynamic-simulation-server
Length of output: 233
🏁 Script executed:
# Read EventInfos.java to verify the annotation state
cat -n ./src/main/java/org/gridsuite/ds/server/dto/event/EventInfos.java | head -50Repository: gridsuite/dynamic-simulation-server
Length of output: 1467
🏁 Script executed:
# Read DynamicSimulationResultContext.java around lines 81 and 94
cat -n ./src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java | sed -n '70,105p'Repository: gridsuite/dynamic-simulation-server
Length of output: 1530
🏁 Script executed:
# Check for ObjectMapper configuration or custom setup
rg -A 5 -B 5 "objectMapper" ./src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java | head -40Repository: gridsuite/dynamic-simulation-server
Length of output: 1712
🏁 Script executed:
# Search for any custom Jackson configuration in the codebase
find ./src -type f -name "*.java" | xargs grep -l "JsonIgnore\|JsonProperty" | head -10Repository: gridsuite/dynamic-simulation-server
Length of output: 596
🏁 Script executed:
# Search for ObjectMapper bean configuration or Jackson setup
find ./src -type f -name "*.java" | xargs grep -l "ObjectMapper" | xargs grep -l "Bean\|Configuration\|@Configuration" | head -10Repository: gridsuite/dynamic-simulation-server
Length of output: 963
🏁 Script executed:
# Look for Jackson configuration or feature disabling
rg "ObjectMapper|Jackson|@Bean" ./src --type java -B 2 -A 5 | grep -A 7 -B 2 "ObjectMapper" | head -50Repository: gridsuite/dynamic-simulation-server
Length of output: 5462
🏁 Script executed:
# Check if there's Jackson configuration in the main config
cat -n ./src/main/java/org/gridsuite/ds/server/config/RestTemplateConfig.java | head -100Repository: gridsuite/dynamic-simulation-server
Length of output: 3510
🏁 Script executed:
# Verify Jackson annotation behavior is standard - check if there are any custom configurations
rg "setSerializationInclusion|disable|enable|JsonProperty|JsonIgnore" ./src/main/java --type java -B 2 -A 2 | head -60Repository: gridsuite/dynamic-simulation-server
Length of output: 6052
@JsonIgnore suppresses both serialization and deserialization, completely neutralizing @JsonProperty("uuid").
At lines 30-31, the conflicting annotations cause the uuid field to be dropped during the serialization/deserialization cycle in DynamicSimulationResultContext (lines 81 and 94), preventing round-tripping of event data.
Remove @JsonIgnore if the uuid field should be serialized, or use @JsonProperty(value = "uuid", access = JsonProperty.Access.READ_WRITE) to be explicit about allowing both directions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/gridsuite/ds/server/dto/event/EventInfos.java` around lines
30 - 32, The EventInfos class currently marks the UUID field `private UUID id`
with both `@JsonProperty`("uuid") and `@JsonIgnore` which cancels
serialization/deserialization; update the annotations on `id` in EventInfos to
allow round-tripping (either remove `@JsonIgnore` or replace the annotation with
`@JsonProperty`(value = "uuid", access = JsonProperty.Access.READ_WRITE)) so
DynamicSimulationResultContext can serialize and deserialize the `uuid` field
correctly.
Comment on lines
+61
to
+63
| public void update(SolverInfos solverInfos) { | ||
| assignAttributes((IdaSolverInfos) solverInfos); | ||
| } |
There was a problem hiding this comment.
Guard update() against wrong subtype to avoid runtime ClassCastException.
Line 62 performs an unchecked cast. If a mismatched SolverInfos reaches this entity, this will fail with a 500 instead of a controlled validation error.
Proposed fix
`@Override`
public void update(SolverInfos solverInfos) {
- assignAttributes((IdaSolverInfos) solverInfos);
+ if (!(solverInfos instanceof IdaSolverInfos idaSolverInfos)) {
+ throw new IllegalArgumentException(
+ "Expected IdaSolverInfos but got " +
+ (solverInfos == null ? "null" : solverInfos.getClass().getSimpleName())
+ );
+ }
+ assignAttributes(idaSolverInfos);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/IdaSolverParametersEntity.java`
around lines 61 - 63, The update(SolverInfos) method currently performs an
unchecked cast to IdaSolverInfos via assignAttributes((IdaSolverInfos)
solverInfos); guard this by checking the runtime type of solverInfos (e.g., if
(!(solverInfos instanceof IdaSolverInfos)) ) and throw a clear, controlled
validation exception (IllegalArgumentException or your domain
ValidationException) with a descriptive message; otherwise cast safely
(IdaSolverInfos ida = (IdaSolverInfos) solverInfos) and call
assignAttributes(ida). Ensure the exception type and message align with existing
validation/error handling in the service so callers receive a proper 4xx
response rather than a 500.
Comment on lines
+113
to
+118
| protected void assignAttributes(AbstractSolverInfos solverInfos) { | ||
| if (id == null) { | ||
| id = UUID.randomUUID(); | ||
| } | ||
| type = solverInfos.getType(); | ||
| fNormTolAlg = solverInfos.getFNormTolAlg(); |
There was a problem hiding this comment.
Do not source entity type from client DTO in base assignment.
Line 117 copies type from input. For a discriminator-backed hierarchy, this should be derived from the concrete entity subtype, not request payload, otherwise in-memory DTO round-trips can report inconsistent solver types.
Suggested direction
protected void assignAttributes(AbstractSolverInfos solverInfos) {
if (id == null) {
id = UUID.randomUUID();
}
- type = solverInfos.getType();
+ type = getSolverType();
fNormTolAlg = solverInfos.getFNormTolAlg();
...
}
+
+protected abstract SolverType getSolverType();Then implement getSolverType() in each subtype (IdaSolverParametersEntity, SimSolverParametersEntity).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.java`
around lines 113 - 118, The assignAttributes method currently sets the entity's
type from the incoming DTO (type = solverInfos.getType()), but for a
discriminator-backed hierarchy the type must come from the concrete entity
class; change assignAttributes in SolverParametersEntity to stop copying
solverInfos.getType() and instead assign type from a protected/abstract
getSolverType() called on this instance (e.g., type = getSolverType()), then
implement getSolverType() in each concrete subclass (IdaSolverParametersEntity,
SimSolverParametersEntity) to return their correct SolverType; ensure no other
code paths overwrite type from the DTO.
Comment on lines
+175
to
+181
| public static SolverParametersEntity fromDto(SolverInfos solverInfos) { | ||
| if (solverInfos instanceof IdaSolverInfos idaInfos) { | ||
| return new IdaSolverParametersEntity(idaInfos); | ||
| } else if (solverInfos instanceof SimSolverInfos simInfos) { | ||
| return new SimSolverParametersEntity(simInfos); | ||
| } | ||
| throw new IllegalArgumentException("Unknown solver type: " + solverInfos.getType()); |
There was a problem hiding this comment.
Add an explicit null guard in fromDto(...).
A null input currently triggers a less clear NPE path. Fail fast with a clear IllegalArgumentException.
Proposed fix
public static SolverParametersEntity fromDto(SolverInfos solverInfos) {
+ if (solverInfos == null) {
+ throw new IllegalArgumentException("solverInfos must not be null");
+ }
if (solverInfos instanceof IdaSolverInfos idaInfos) {
return new IdaSolverParametersEntity(idaInfos);
} else if (solverInfos instanceof SimSolverInfos simInfos) {
return new SimSolverParametersEntity(simInfos);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/entities/parameters/solver/SolverParametersEntity.java`
around lines 175 - 181, Add a null guard at the start of
SolverParametersEntity.fromDto(SolverInfos) to fail fast when solverInfos is
null: check if solverInfos == null and throw an IllegalArgumentException with a
clear message (e.g. "solverInfos must not be null") before the existing
instanceof branches (so the method either handles IdaSolverInfos/SimSolverInfos
or immediately rejects null).
Comment on lines
+77
to
+85
| // using Object then toString() to avoid casting exception since rabbitmq uses LongString instead of String | ||
| Object eventsJson = headers.get(HEADER_EVENTS); | ||
| List<EventInfos> events; | ||
| try { | ||
| events = objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { }); | ||
| } catch (JsonProcessingException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| runContext.setEvents(events); |
There was a problem hiding this comment.
Guard against missing events header to avoid runtime NPE.
At Line 81, eventsJson.toString() will throw if Line 78 returns null (missing header / backward-compat message). Handle absent header explicitly and default to an empty list (or fail fast with getNonNullHeader).
Proposed null-safe parsing
// using Object then toString() to avoid casting exception since rabbitmq uses LongString instead of String
Object eventsJson = headers.get(HEADER_EVENTS);
- List<EventInfos> events;
- try {
- events = objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { });
- } catch (JsonProcessingException e) {
- throw new UncheckedIOException(e);
- }
+ List<EventInfos> events = List.of();
+ if (eventsJson != null) {
+ try {
+ events = objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { });
+ if (events == null) {
+ events = List.of();
+ }
+ } catch (JsonProcessingException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
runContext.setEvents(events);📝 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
| // using Object then toString() to avoid casting exception since rabbitmq uses LongString instead of String | |
| Object eventsJson = headers.get(HEADER_EVENTS); | |
| List<EventInfos> events; | |
| try { | |
| events = objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { }); | |
| } catch (JsonProcessingException e) { | |
| throw new UncheckedIOException(e); | |
| } | |
| runContext.setEvents(events); | |
| // using Object then toString() to avoid casting exception since rabbitmq uses LongString instead of String | |
| Object eventsJson = headers.get(HEADER_EVENTS); | |
| List<EventInfos> events = List.of(); | |
| if (eventsJson != null) { | |
| try { | |
| events = objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { }); | |
| if (events == null) { | |
| events = List.of(); | |
| } | |
| } catch (JsonProcessingException e) { | |
| throw new UncheckedIOException(e); | |
| } | |
| } | |
| runContext.setEvents(events); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java`
around lines 77 - 85, The code calls headers.get(HEADER_EVENTS) into eventsJson
and immediately does eventsJson.toString(), which will NPE if the header is
missing; update the block in DynamicSimulationResultContext to first check if
headers.get(HEADER_EVENTS) (HEADER_EVENTS / eventsJson) is null and in that case
set events to Collections.emptyList() (or call a getNonNullHeader helper) before
calling objectMapper.readValue; otherwise parse using
objectMapper.readValue(eventsJson.toString(), new TypeReference<>() { }) and
keep the existing JsonProcessingException -> UncheckedIOException handling, then
call runContext.setEvents(events).
Comment on lines
+91
to
+99
| public Map<String, String> getSpecificMsgHeaders(ObjectMapper objectMapper) { | ||
| String eventsJson; | ||
| try { | ||
| eventsJson = objectMapper.writeValueAsString(getRunContext().getEvents()); | ||
| } catch (JsonProcessingException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| return Map.of(HEADER_MAPPING, getRunContext().getMapping(), | ||
| HEADER_EVENTS, eventsJson); |
There was a problem hiding this comment.
Normalize null events before serializing headers.
At Line 94, serializing a null events list propagates a non-list payload and weakens invariants downstream. Serialize [] instead to keep runContext.getEvents() consistently non-null after deserialization.
Proposed normalization
public Map<String, String> getSpecificMsgHeaders(ObjectMapper objectMapper) {
String eventsJson;
try {
- eventsJson = objectMapper.writeValueAsString(getRunContext().getEvents());
+ eventsJson = objectMapper.writeValueAsString(
+ Objects.requireNonNullElseGet(getRunContext().getEvents(), List::of)
+ );
} catch (JsonProcessingException e) {
throw new UncheckedIOException(e);
}
return Map.of(HEADER_MAPPING, getRunContext().getMapping(),
HEADER_EVENTS, eventsJson);📝 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
| public Map<String, String> getSpecificMsgHeaders(ObjectMapper objectMapper) { | |
| String eventsJson; | |
| try { | |
| eventsJson = objectMapper.writeValueAsString(getRunContext().getEvents()); | |
| } catch (JsonProcessingException e) { | |
| throw new UncheckedIOException(e); | |
| } | |
| return Map.of(HEADER_MAPPING, getRunContext().getMapping(), | |
| HEADER_EVENTS, eventsJson); | |
| public Map<String, String> getSpecificMsgHeaders(ObjectMapper objectMapper) { | |
| String eventsJson; | |
| try { | |
| eventsJson = objectMapper.writeValueAsString( | |
| Objects.requireNonNullElseGet(getRunContext().getEvents(), List::of) | |
| ); | |
| } catch (JsonProcessingException e) { | |
| throw new UncheckedIOException(e); | |
| } | |
| return Map.of(HEADER_MAPPING, getRunContext().getMapping(), | |
| HEADER_EVENTS, eventsJson); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/ds/server/service/contexts/DynamicSimulationResultContext.java`
around lines 91 - 99, The getSpecificMsgHeaders method currently serializes
getRunContext().getEvents() directly which can be null; change it to normalize
null to an empty list before serialization (e.g., assign a local events variable
= getRunContext().getEvents() != null ? getRunContext().getEvents() :
Collections.emptyList()) and then call objectMapper.writeValueAsString on that
normalized list so HEADER_EVENTS always contains "[]" instead of null after
deserialization; update references in getSpecificMsgHeaders accordingly (method:
getSpecificMsgHeaders, call site: getRunContext().getEvents(), header key:
HEADER_EVENTS).
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
providerandmappingfrom run request's params