Moving Dynamic Simulation params to dynamic-simulation-server#969
Open
Moving Dynamic Simulation params to dynamic-simulation-server#969
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
📝 WalkthroughWalkthroughRefactors dynamic-simulation parameter handling from DTO/entity-based models to UUID-referenced JSON strings. Removes multiple DTOs and a dedicated parameters entity, adds UUID column on Study, introduces parameter management APIs (create/get/update/duplicate/delete/default) and updates controller/service/client signatures to use parameter UUIDs and serialized JSON. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as StudyController
participant StudyService
participant DynamicSimulationService
participant DynamicSimulationClient
participant ExtSim as ExternalSimulationAPI
Client->>Controller: POST /studies/{id}/runDynamicSimulation?parametersUuid=UUID\nbody: events JSON
Controller->>StudyService: runDynamicSimulation(studyUuid, rootNetworkUuid, nodeUuid, debug, userId)
StudyService->>DynamicSimulationService: runDynamicSimulation(nodeUuid, rootNetworkUuid, networkUuid, variantId, reportUuid, parametersUuid, events, userId, debug)
DynamicSimulationService->>DynamicSimulationClient: run(receiver, networkUuid, variantId, reportInfos, parametersUuid, events, userId, debug)
DynamicSimulationClient->>ExtSim: POST /api/vX/run?parametersUuid=UUID\nbody: events
ExtSim-->>DynamicSimulationClient: 202 Accepted / resultUuid
DynamicSimulationClient-->>DynamicSimulationService: resultUuid
DynamicSimulationService-->>StudyService: resultUuid
StudyService-->>Controller: resultUuid
Controller-->>Client: 202 Accepted with resultUuid
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javaThanks 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.java (1)
154-162:⚠️ Potential issue | 🟡 MinorAdd null validation for
dynamicSimulationParametersUuid.The parameter
dynamicSimulationParametersUuidis annotated with@NonNullbut lacks the correspondingObjects.requireNonNull()validation that other@NonNullparameters in this method have. This inconsistency could lead to less clear error messages ifnullis accidentally passed.Proposed fix
public UUID run(`@NonNull` String receiver, `@NonNull` UUID networkUuid, String variantId, `@NonNull` ReportInfos reportInfos, `@NonNull` UUID dynamicSimulationParametersUuid, `@NonNull` UUID dynamicSecurityAnalysisParametersUuid, `@NonNull` UUID parametersUuid, String userId, boolean debug) { Objects.requireNonNull(receiver); Objects.requireNonNull(networkUuid); Objects.requireNonNull(reportInfos); + Objects.requireNonNull(dynamicSimulationParametersUuid); Objects.requireNonNull(dynamicSecurityAnalysisParametersUuid); Objects.requireNonNull(parametersUuid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.java` around lines 154 - 162, The method run in DynamicMarginCalculationClient is missing a null-check for the `@NonNull` parameter dynamicSimulationParametersUuid; add an Objects.requireNonNull(dynamicSimulationParametersUuid) call in the same validation block where receiver, networkUuid, reportInfos, dynamicSecurityAnalysisParametersUuid, and parametersUuid are checked so the method consistently validates all `@NonNull` parameters and throws a clear NPE if dynamicSimulationParametersUuid is null.src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java (2)
306-321:⚠️ Potential issue | 🟡 MinorWireMock stub missing
dynamicSimulationParametersUuidquery parameter validation.The stub validates
dynamicSecurityAnalysisParametersUuidandparametersUuidbut omits validation fordynamicSimulationParametersUuid. According to the production code, this parameter is sent as a query parameter (.queryParam("dynamicSimulationParametersUuid", dynamicSimulationParametersUuid)). The stub will accept any value for this parameter, reducing test coverage.🐛 Proposed fix to add missing query param validation
wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo("variantId")) + .withQueryParam("dynamicSimulationParametersUuid", equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) .withQueryParam("dynamicSecurityAnalysisParametersUuid", equalTo(DYNAMIC_SECURITY_ANALYSIS_PARAMETERS_UUID.toString())) .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java` around lines 306 - 321, The WireMock stub in DynamicMarginCalculationClientTest is missing validation for the dynamicSimulationParametersUuid query parameter; update the stub chain (the WireMock.post(...).withQueryParam(...) calls) to include .withQueryParam("dynamicSimulationParametersUuid", equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) so the stub matches the production call made by dynamicMarginCalculationClient.run and uses the existing DYNAMIC_SIMULATION_PARAMETERS_UUID test constant.
327-345:⚠️ Potential issue | 🟡 MinorError case WireMock stub also missing
dynamicSimulationParametersUuidvalidation.Same issue as the success case - the error stub should validate the
dynamicSimulationParametersUuidquery parameter for consistency.🐛 Proposed fix
wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) .withQueryParam(QUERY_PARAM_VARIANT_ID, absent()) .withQueryParam("provider", absent()) + .withQueryParam("dynamicSimulationParametersUuid", equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) .withQueryParam("dynamicSecurityAnalysisParametersUuid", equalTo(DYNAMIC_SECURITY_ANALYSIS_PARAMETERS_UUID.toString()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java` around lines 327 - 345, The WireMock error stub in DynamicMarginCalculationClientTest is missing validation for the dynamicSimulationParametersUuid query param; update the stub configured via wireMockServer.stubFor(...) (same block that uses url, QUERY_PARAM_VARIANT_ID, QUERY_PARAM_RECEIVER, etc.) to include .withQueryParam("dynamicSimulationParametersUuid", equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) so the error case validates the dynamicSimulationParametersUuid the same way the success case does.
🧹 Nitpick comments (4)
src/main/resources/db/changelog/changesets/changelog_20260319T184344Z.xml (1)
6-7: Minor: Inconsistent table name casing.The changeset uses lowercase
studyon line 6 but uppercaseSTUDYon lines 11, 14, and 17. While Liquibase and most databases handle this gracefully, consistent casing improves readability.Suggested fix for consistency
<changeSet author="phamquy (generated)" id="1773945844517-17"> - <dropForeignKeyConstraint baseTableName="STUDY" constraintName="DYNAMICSIMULATIONPARAMETERS_ID_FK"/> + <dropForeignKeyConstraint baseTableName="study" constraintName="DYNAMICSIMULATIONPARAMETERS_ID_FK"/> </changeSet> <changeSet author="phamquy (generated)" id="1773945844517-18"> - <dropTable tableName="DYNAMIC_SIMULATION_PARAMETERS"/> + <dropTable tableName="dynamic_simulation_parameters"/> </changeSet> <changeSet author="phamquy (generated)" id="1773945844517-19"> - <dropColumn columnName="DYNAMIC_SIMULATION_PARAMETERS_ENTITY_ID" tableName="STUDY"/> + <dropColumn columnName="dynamic_simulation_parameters_entity_id" tableName="study"/> </changeSet> <changeSet author="phamquy (generated)" id="1773945844517-20"> - <dropColumn columnName="DYNAMIC_SIMULATION_PROVIDER" tableName="STUDY"/> + <dropColumn columnName="dynamic_simulation_provider" tableName="study"/> </changeSet>Also applies to: 11-11, 14-14, 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/changelog/changesets/changelog_20260319T184344Z.xml` around lines 6 - 7, The XML changeset has inconsistent tableName casing: some tags use tableName="study" and others tableName="STUDY"; update all tableName attributes in this changeset to use a single consistent casing (pick one convention—prefer lowercase "study") for every change element (e.g., the <addColumn tableName="study"> and the other change elements referencing STUDY) so all occurrences match exactly for readability and consistency.src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.java (1)
31-45: Consider consistent@NonNullannotations across similar parameter methods.
getProvideruses@NonNull UUID parametersUuidbutgetParameters,updateParameters,duplicateParameters, anddeleteParametersdon't annotate their UUID parameters. Consider applying@NonNullconsistently for uniform null-safety contracts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.java` around lines 31 - 45, The UUID parameter null-safety is inconsistent: add `@NonNull` to the UUID parameters of getParameters(UUID), updateParameters(UUID,...), duplicateParameters(UUID) and deleteParameters(UUID) so their signatures match getProvider(`@NonNull` UUID) and enforce a uniform contract; keep existing annotations on createParameters(String) and leave userId as-is unless you want it non-null too.src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java (1)
104-105: Use a real JSON payload and verify it is forwarded unchanged.This endpoint now persists raw JSON strings. With
PARAMETERS_JSON = "parametersJson"andcreateParameters(any()), the test does not exercise the real payload shape and won’t catch body mangling on the controller path.🧪 Strengthen the round-trip assertion
- private static final String PARAMETERS_JSON = "parametersJson"; + private static final String PARAMETERS_JSON = "{\"startTime\":0,\"stopTime\":500}"; @@ - doAnswer(invocation -> PARAMETERS_UUID) - .when(spyDynamicSimulationService).createParameters(any()); + doAnswer(invocation -> PARAMETERS_UUID) + .when(spyDynamicSimulationService).createParameters(eq(jsonParameters)); @@ studyClient.perform(post(STUDY_BASE_URL + DELIMITER + STUDY_DYNAMIC_SIMULATION_END_POINT_PARAMETERS, studyUuid) .header(HEADER_USER_ID_NAME, HEADER_USER_ID_VALUE) .contentType(MediaType.APPLICATION_JSON) .content(jsonParameters)) .andExpect(status().isOk()).andReturn(); + verify(spyDynamicSimulationService).createParameters(eq(jsonParameters));Also applies to: 693-706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java` around lines 104 - 105, Replace the placeholder PARAMETERS_JSON usage and the generic createParameters(any()) stub in StudyControllerDynamicSimulationTest with a real JSON string (e.g. a small nested object/array) and use that exact string in the request; then capture the argument passed to the mocked createParameters (via ArgumentCaptor or Mockito.verify) and assert the captured value equals the original JSON string so the controller forwards the raw body unchanged (reference PARAMETERS_JSON, createParameters(...), and the test method in StudyControllerDynamicSimulationTest).src/main/java/org/gridsuite/study/server/controller/StudyController.java (1)
1834-1837: Add explicit JSON media type to DS parameters GET endpoint.The endpoint returns
ResponseEntity<String>without explicitproducesdeclaration, allowing content-type negotiation to default to plain text. PinningMediaType.APPLICATION_JSON_VALUEensures client-side content-type safety and aligns with similar endpoints in the controller that already declare explicit media types.Note: Similar parameter GET endpoints across the controller (voltage-init, loadflow, dynamic-security-analysis, security-analysis, etc.) also lack explicit media-type declarations—consider a broader pass to standardize this pattern.
♻️ Proposed change
- `@GetMapping`(value = "/studies/{studyUuid}/dynamic-simulation/parameters") + `@GetMapping`(value = "/studies/{studyUuid}/dynamic-simulation/parameters", produces = MediaType.APPLICATION_JSON_VALUE) `@Operation`(summary = "Get dynamic simulation parameters on study") `@ApiResponses`(value = {`@ApiResponse`(responseCode = "200", description = "The dynamic simulation parameters")}) public ResponseEntity<String> getDynamicSimulationParameters( `@PathVariable`("studyUuid") UUID studyUuid, `@RequestHeader`(HEADER_USER_ID) String userId) { - return ResponseEntity.ok().body(studyService.getDynamicSimulationParameters(studyUuid, userId)); + return ResponseEntity.ok().contentType(MediaType.APPLICATION_JSON) + .body(studyService.getDynamicSimulationParameters(studyUuid, userId)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java` around lines 1834 - 1837, The GET endpoint method getDynamicSimulationParameters currently returns ResponseEntity<String> without an explicit produces media type; update its mapping annotation (the `@GetMapping/`@RequestMapping for getDynamicSimulationParameters) to include produces = MediaType.APPLICATION_JSON_VALUE so the response Content-Type is pinned to application/json and matches other parameter endpoints; apply the same pattern for other parameter GET endpoints (e.g., voltage-init, loadflow, dynamic-security-analysis, security-analysis) as a follow-up pass to standardize content negotiation.
🤖 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/study/server/service/StudyService.java`:
- Around line 3006-3018: Replace the direct call to
studyEntity.getDynamicSimulationParametersUuid() used as an argument to
dynamicSimulationService.runDynamicSimulation with the defensive helper to
guarantee a non-null UUID: call
getDynamicSimulationParametersUuidOrElseCreateDefault(studyEntity) and pass its
result into runDynamicSimulation; update the reference near where
dynamicSimulationResultUuid is created so runDynamicSimulation receives the
non-null UUID instead of the raw
studyEntity.getDynamicSimulationParametersUuid() value.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java`:
- Around line 127-157: Update the WireMock stubs in DynamicSimulationClientTest
so they assert the forwarded HEADER_USER_ID header is present and equals the
expected value; specifically, for each
wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) ...) (the stubs
used for the success, not-found, and server-error cases) add a header matcher
that checks HEADER_USER_ID equals "userId" (or the test's userId variable) so
the stubs only match when DynamicSimulationClientImpl.getParameters(...)
actually forwards the header.
- Around line 316-352: The test's error path is a false positive because the
WireMock error stub incorrectly uses withHeader(QUERY_PARAM_DEBUG, ...) instead
of withQueryParam(QUERY_PARAM_DEBUG, ...) and doesn't verify the request body
(`events`), so WireMock falls back to 404; update both error stubs to use
withQueryParam("debug", equalTo("true")) (matching the real client behavior) and
add a request body matcher that verifies `events` (e.g.
withRequestBody(equalToJson(objectMapper.writeValueAsString(events)))); finally
change the asserted exception from HttpClientErrorException.NotFound.class to
HttpServerErrorException.class when calling dynamicSimulationClient.run(...) so
the test expects the 500 server error.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.java`:
- Around line 154-162: The method run in DynamicMarginCalculationClient is
missing a null-check for the `@NonNull` parameter dynamicSimulationParametersUuid;
add an Objects.requireNonNull(dynamicSimulationParametersUuid) call in the same
validation block where receiver, networkUuid, reportInfos,
dynamicSecurityAnalysisParametersUuid, and parametersUuid are checked so the
method consistently validates all `@NonNull` parameters and throws a clear NPE if
dynamicSimulationParametersUuid is null.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java`:
- Around line 306-321: The WireMock stub in DynamicMarginCalculationClientTest
is missing validation for the dynamicSimulationParametersUuid query parameter;
update the stub chain (the WireMock.post(...).withQueryParam(...) calls) to
include .withQueryParam("dynamicSimulationParametersUuid",
equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) so the stub matches the
production call made by dynamicMarginCalculationClient.run and uses the existing
DYNAMIC_SIMULATION_PARAMETERS_UUID test constant.
- Around line 327-345: The WireMock error stub in
DynamicMarginCalculationClientTest is missing validation for the
dynamicSimulationParametersUuid query param; update the stub configured via
wireMockServer.stubFor(...) (same block that uses url, QUERY_PARAM_VARIANT_ID,
QUERY_PARAM_RECEIVER, etc.) to include
.withQueryParam("dynamicSimulationParametersUuid",
equalTo(DYNAMIC_SIMULATION_PARAMETERS_UUID.toString())) so the error case
validates the dynamicSimulationParametersUuid the same way the success case
does.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java`:
- Around line 1834-1837: The GET endpoint method getDynamicSimulationParameters
currently returns ResponseEntity<String> without an explicit produces media
type; update its mapping annotation (the `@GetMapping/`@RequestMapping for
getDynamicSimulationParameters) to include produces =
MediaType.APPLICATION_JSON_VALUE so the response Content-Type is pinned to
application/json and matches other parameter endpoints; apply the same pattern
for other parameter GET endpoints (e.g., voltage-init, loadflow,
dynamic-security-analysis, security-analysis) as a follow-up pass to standardize
content negotiation.
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.java`:
- Around line 31-45: The UUID parameter null-safety is inconsistent: add
`@NonNull` to the UUID parameters of getParameters(UUID),
updateParameters(UUID,...), duplicateParameters(UUID) and deleteParameters(UUID)
so their signatures match getProvider(`@NonNull` UUID) and enforce a uniform
contract; keep existing annotations on createParameters(String) and leave userId
as-is unless you want it non-null too.
In `@src/main/resources/db/changelog/changesets/changelog_20260319T184344Z.xml`:
- Around line 6-7: The XML changeset has inconsistent tableName casing: some
tags use tableName="study" and others tableName="STUDY"; update all tableName
attributes in this changeset to use a single consistent casing (pick one
convention—prefer lowercase "study") for every change element (e.g., the
<addColumn tableName="study"> and the other change elements referencing STUDY)
so all occurrences match exactly for readability and consistency.
In
`@src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java`:
- Around line 104-105: Replace the placeholder PARAMETERS_JSON usage and the
generic createParameters(any()) stub in StudyControllerDynamicSimulationTest
with a real JSON string (e.g. a small nested object/array) and use that exact
string in the request; then capture the argument passed to the mocked
createParameters (via ArgumentCaptor or Mockito.verify) and assert the captured
value equals the original JSON string so the controller forwards the raw body
unchanged (reference PARAMETERS_JSON, createParameters(...), and the test method
in StudyControllerDynamicSimulationTest).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c70644d-b3cf-4168-8cd8-2d1ec27f4338
📒 Files selected for processing (44)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/UserProfileInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/DynamicSimulationParametersInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/curve/CurveInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/network/NetworkInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/AbstractSolverInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/IdaSolverInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SimSolverInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverInfos.javasrc/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverTypeInfos.javasrc/main/java/org/gridsuite/study/server/repository/DynamicSimulationParametersEntity.javasrc/main/java/org/gridsuite/study/server/repository/StudyEntity.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/FilterService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/client/RestClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicmapping/impl/DynamicMappingClientImpl.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/client/timeseries/impl/TimeSeriesClientImpl.javasrc/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.javasrc/main/resources/db/changelog/changesets/changelog_20260319T184344Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/study/server/StudyControllerDynamicMarginCalculationTest.javasrc/test/java/org/gridsuite/study/server/StudyControllerDynamicSecurityAnalysisTest.javasrc/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.javasrc/test/java/org/gridsuite/study/server/dto/dynamicsimulation/curve/CurveInfosTest.javasrc/test/java/org/gridsuite/study/server/dto/dynamicsimulation/network/NetworkInfosTest.javasrc/test/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverInfosTest.javasrc/test/java/org/gridsuite/study/server/service/StudyServiceDynamicSimulationTest.javasrc/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.javasrc/test/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClientTest.javasrc/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.javasrc/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/ParameterTestUtils.javasrc/test/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationServiceTest.javasrc/test/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisServiceTest.javasrc/test/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationServiceTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTestBase.java
💤 Files with no reviewable changes (12)
- src/test/java/org/gridsuite/study/server/dto/dynamicsimulation/curve/CurveInfosTest.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverTypeInfos.java
- src/test/java/org/gridsuite/study/server/dto/dynamicsimulation/network/NetworkInfosTest.java
- src/test/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverInfosTest.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/curve/CurveInfos.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/AbstractSolverInfos.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/IdaSolverInfos.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SolverInfos.java
- src/main/java/org/gridsuite/study/server/repository/DynamicSimulationParametersEntity.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/solver/SimSolverInfos.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/DynamicSimulationParametersInfos.java
- src/main/java/org/gridsuite/study/server/dto/dynamicsimulation/network/NetworkInfos.java
Comment on lines
+3006
to
+3018
| // get dynamic simulation result uuid | ||
| UUID dynamicSimulationParametersUuid = studyEntity.getDynamicSimulationParametersUuid(); | ||
|
|
||
| // load configured events persisted in the study server DB | ||
| List<EventInfos> events = dynamicSimulationEventService.getEventsByNodeId(nodeUuid); | ||
|
|
||
| // override configured parameters by provided parameters (only provided fields) | ||
| DynamicSimulationParametersInfos mergeParameters = new DynamicSimulationParametersInfos(); | ||
| // attach events to the merged parameters | ||
| mergeParameters.setEvents(events); | ||
|
|
||
| if (configuredParameters != null) { | ||
| PropertyUtils.copyNonNullProperties(configuredParameters, mergeParameters); | ||
| } | ||
| if (parameters != null) { | ||
| PropertyUtils.copyNonNullProperties(parameters, mergeParameters); | ||
| } | ||
| UUID reportUuid = networkModificationTreeService.getComputationReports(nodeUuid, rootNetworkUuid).getOrDefault(DYNAMIC_SIMULATION.name(), UUID.randomUUID()); | ||
| networkModificationTreeService.updateComputationReportUuid(nodeUuid, rootNetworkUuid, DYNAMIC_SIMULATION, reportUuid); | ||
|
|
||
| // launch dynamic simulation | ||
| UUID networkUuid = rootNetworkService.getNetworkUuid(rootNetworkUuid); | ||
| String variantId = networkModificationTreeService.getVariantId(nodeUuid, rootNetworkUuid); | ||
| UUID dynamicSimulationResultUuid = dynamicSimulationService.runDynamicSimulation(studyEntity.getDynamicSimulationProvider(), | ||
| nodeUuid, rootNetworkUuid, networkUuid, variantId, reportUuid, mergeParameters, userId, debug); | ||
| UUID dynamicSimulationResultUuid = dynamicSimulationService.runDynamicSimulation(nodeUuid, rootNetworkUuid, networkUuid, variantId, reportUuid, dynamicSimulationParametersUuid, events, userId, debug); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how null parametersUuid is handled in DynamicSimulationService.runDynamicSimulation
ast-grep --pattern $'runDynamicSimulation($$$) {
$$$
}'
# Check if there are null checks or default creation in the service
rg -n -A 10 "runDynamicSimulation" --type java -g '*DynamicSimulationService*.java'Repository: gridsuite/study-server
Length of output: 5191
🏁 Script executed:
#!/bin/bash
# Get the full implementation of runDynamicSimulation
sed -n '114,200p' src/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.java
# Also search for any null checks on parametersUuid
rg -n "parametersUuid" --type java -A 5 -B 5 src/main/java/org/gridsuite/study/server/service/dynamicsimulation/Repository: gridsuite/study-server
Length of output: 19174
Use defensive pattern for dynamic simulation parameters UUID.
studyEntity.getDynamicSimulationParametersUuid() is passed directly to runDynamicSimulation without null handling. Replace with getDynamicSimulationParametersUuidOrElseCreateDefault(studyEntity) (lines 105-110 of DynamicSimulationServiceImpl) to ensure a non-null UUID is passed, matching the defensive pattern used elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around
lines 3006 - 3018, Replace the direct call to
studyEntity.getDynamicSimulationParametersUuid() used as an argument to
dynamicSimulationService.runDynamicSimulation with the defensive helper to
guarantee a non-null UUID: call
getDynamicSimulationParametersUuidOrElseCreateDefault(studyEntity) and pass its
result into runDynamicSimulation; update the reference near where
dynamicSimulationResultUuid is created so runDynamicSimulation receives the
non-null UUID instead of the raw
studyEntity.getDynamicSimulationParametersUuid() value.
Comment on lines
+127
to
+157
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | ||
| .willReturn(WireMock.ok() | ||
| .withBody(parametersJson) | ||
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | ||
| )); | ||
| // call service to test | ||
| String resultParametersJson = dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId"); | ||
|
|
||
| // check result | ||
| assertThat(resultParametersJson).isEqualTo(parametersJson); | ||
|
|
||
| // --- Not Found --- // | ||
| // configure mock server response | ||
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | ||
| .willReturn(WireMock.notFound())); | ||
|
|
||
| // check result | ||
| assertThrows( | ||
| HttpClientErrorException.NotFound.class, | ||
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | ||
| ); | ||
|
|
||
| // --- Error --- // | ||
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | ||
| .willReturn(WireMock.serverError())); | ||
|
|
||
| // check result | ||
| assertThrows( | ||
| HttpServerErrorException.class, | ||
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | ||
| ); |
There was a problem hiding this comment.
Assert the forwarded userId header in this test.
DynamicSimulationClientImpl.getParameters(...) now conditionally sends HEADER_USER_ID, but these stubs match even when that header is missing. If header forwarding regresses, all three branches still stay green.
🔧 Tighten the WireMock expectations
- wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ .withHeader(HEADER_USER_ID, equalTo("userId"))
.willReturn(WireMock.ok()
.withBody(parametersJson)
.withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE)
));
@@
- wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ .withHeader(HEADER_USER_ID, equalTo("userId"))
.willReturn(WireMock.notFound()));
@@
- wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url))
+ .withHeader(HEADER_USER_ID, equalTo("userId"))
.willReturn(WireMock.serverError()));📝 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
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .willReturn(WireMock.ok() | |
| .withBody(parametersJson) | |
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | |
| )); | |
| // call service to test | |
| String resultParametersJson = dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId"); | |
| // check result | |
| assertThat(resultParametersJson).isEqualTo(parametersJson); | |
| // --- Not Found --- // | |
| // configure mock server response | |
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .willReturn(WireMock.notFound())); | |
| // check result | |
| assertThrows( | |
| HttpClientErrorException.NotFound.class, | |
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | |
| ); | |
| // --- Error --- // | |
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .willReturn(WireMock.serverError())); | |
| // check result | |
| assertThrows( | |
| HttpServerErrorException.class, | |
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | |
| ); | |
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.ok() | |
| .withBody(parametersJson) | |
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | |
| )); | |
| // call service to test | |
| String resultParametersJson = dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId"); | |
| // check result | |
| assertThat(resultParametersJson).isEqualTo(parametersJson); | |
| // --- Not Found --- // | |
| // configure mock server response | |
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.notFound())); | |
| // check result | |
| assertThrows( | |
| HttpClientErrorException.NotFound.class, | |
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | |
| ); | |
| // --- Error --- // | |
| wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.serverError())); | |
| // check result | |
| assertThrows( | |
| HttpServerErrorException.class, | |
| () -> dynamicSimulationClient.getParameters(PARAMETERS_UUID, "userId") | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java`
around lines 127 - 157, Update the WireMock stubs in DynamicSimulationClientTest
so they assert the forwarded HEADER_USER_ID header is present and equals the
expected value; specifically, for each
wireMockServer.stubFor(WireMock.get(WireMock.urlEqualTo(url)) ...) (the stubs
used for the success, not-found, and server-error cases) add a header matcher
that checks HEADER_USER_ID equals "userId" (or the test's userId variable) so
the stubs only match when DynamicSimulationClientImpl.getParameters(...)
actually forwards the header.
Comment on lines
316
to
352
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | ||
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo(VARIANT_ID)) | ||
| .withQueryParam("provider", equalTo(DYNAWO_PROVIDER)) | ||
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | ||
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | ||
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | ||
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | ||
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | ||
| .withHeader(HEADER_USER_ID, equalTo("userId")) | ||
| .willReturn(WireMock.ok() | ||
| .withBody(objectMapper.writeValueAsString(RESULT_UUID)) | ||
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | ||
| )); | ||
|
|
||
| // call service to test | ||
| UUID resultUuid = dynamicSimulationClient.run(DYNAWO_PROVIDER, "receiver", NETWORK_UUID, VARIANT_ID, | ||
| new ReportInfos(REPORT_UUID, NODE_UUID), parameters, "userId", false); | ||
| UUID resultUuid = dynamicSimulationClient.run("receiver", NETWORK_UUID, VARIANT_ID, | ||
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", false); | ||
|
|
||
| // check result | ||
| assertThat(resultUuid).isEqualTo(RESULT_UUID); | ||
|
|
||
| // --- Error --- // | ||
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | ||
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo("variantIdFailed")) | ||
| .withQueryParam("provider", equalTo("Dynawo")) | ||
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | ||
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | ||
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | ||
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | ||
| .withHeader(QUERY_PARAM_DEBUG, equalTo("true")) | ||
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | ||
| .withHeader(HEADER_USER_ID, equalTo("userId")) | ||
| .willReturn(WireMock.serverError())); | ||
|
|
||
| // check result | ||
| assertThrows( | ||
| HttpClientErrorException.NotFound.class, | ||
| () -> dynamicSimulationClient.run("Dynawo", "receiver", NETWORK_UUID, "variantIdFailed", | ||
| new ReportInfos(REPORT_UUID, NODE_UUID), parameters, "userId", true) | ||
| () -> dynamicSimulationClient.run("receiver", NETWORK_UUID, "variantIdFailed", | ||
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", true) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Implementation contract:"
rg -n -C2 'queryParam\(QUERY_PARAM_DEBUG|HttpEntity<List<EventInfos>>|postForObject' \
src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.java
echo
echo "Current test matchers:"
rg -n -C2 'testRun\(|withHeader\(QUERY_PARAM_DEBUG|withQueryParam\(QUERY_PARAM_DEBUG|withRequestBody' \
src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.javaRepository: gridsuite/study-server
Length of output: 2874
Fix the run error path—test is a false positive.
The implementation sends debug as a query parameter and events as the request body, but the error stub checks debug as a header and ignores the body. WireMock's fallback 404 satisfies the test instead of the intended 500 path. Additionally, the exception type is incorrect: HttpServerErrorException.class not HttpClientErrorException.NotFound.class.
Apply these fixes to both stubs
wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url))
+ .withRequestBody(WireMock.equalToJson(objectMapper.writeValueAsString(events)))
.withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo(VARIANT_ID))
.withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver"))
.withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString()))
.withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString()))
.withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey))
.withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString()))
.withHeader(HEADER_USER_ID, equalTo("userId"))
.willReturn(WireMock.ok()
.withBody(objectMapper.writeValueAsString(RESULT_UUID))
.withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE)
));
wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url))
.withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo("variantIdFailed"))
.withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver"))
.withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString()))
.withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString()))
.withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey))
- .withHeader(QUERY_PARAM_DEBUG, equalTo("true"))
+ .withQueryParam(QUERY_PARAM_DEBUG, equalTo("true"))
+ .withRequestBody(WireMock.equalToJson(objectMapper.writeValueAsString(events)))
.withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString()))
.withHeader(HEADER_USER_ID, equalTo("userId"))
.willReturn(WireMock.serverError()));
- assertThrows(
- HttpClientErrorException.NotFound.class,
+ assertThrows(
+ HttpServerErrorException.class,
() -> dynamicSimulationClient.run("receiver", NETWORK_UUID, "variantIdFailed",
new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", true)
);📝 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
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | |
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo(VARIANT_ID)) | |
| .withQueryParam("provider", equalTo(DYNAWO_PROVIDER)) | |
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | |
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | |
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.ok() | |
| .withBody(objectMapper.writeValueAsString(RESULT_UUID)) | |
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | |
| )); | |
| // call service to test | |
| UUID resultUuid = dynamicSimulationClient.run(DYNAWO_PROVIDER, "receiver", NETWORK_UUID, VARIANT_ID, | |
| new ReportInfos(REPORT_UUID, NODE_UUID), parameters, "userId", false); | |
| UUID resultUuid = dynamicSimulationClient.run("receiver", NETWORK_UUID, VARIANT_ID, | |
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", false); | |
| // check result | |
| assertThat(resultUuid).isEqualTo(RESULT_UUID); | |
| // --- Error --- // | |
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | |
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo("variantIdFailed")) | |
| .withQueryParam("provider", equalTo("Dynawo")) | |
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | |
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | |
| .withHeader(QUERY_PARAM_DEBUG, equalTo("true")) | |
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.serverError())); | |
| // check result | |
| assertThrows( | |
| HttpClientErrorException.NotFound.class, | |
| () -> dynamicSimulationClient.run("Dynawo", "receiver", NETWORK_UUID, "variantIdFailed", | |
| new ReportInfos(REPORT_UUID, NODE_UUID), parameters, "userId", true) | |
| () -> dynamicSimulationClient.run("receiver", NETWORK_UUID, "variantIdFailed", | |
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", true) | |
| ); | |
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | |
| .withRequestBody(WireMock.equalToJson(objectMapper.writeValueAsString(events))) | |
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo(VARIANT_ID)) | |
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | |
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | |
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.ok() | |
| .withBody(objectMapper.writeValueAsString(RESULT_UUID)) | |
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | |
| )); | |
| // call service to test | |
| UUID resultUuid = dynamicSimulationClient.run("receiver", NETWORK_UUID, VARIANT_ID, | |
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", false); | |
| // check result | |
| assertThat(resultUuid).isEqualTo(RESULT_UUID); | |
| // --- Error --- // | |
| wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(url)) | |
| .withQueryParam(QUERY_PARAM_VARIANT_ID, equalTo("variantIdFailed")) | |
| .withQueryParam(QUERY_PARAM_RECEIVER, equalTo("receiver")) | |
| .withQueryParam(QUERY_PARAM_REPORT_UUID, equalTo(REPORT_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORTER_ID, equalTo(NODE_UUID.toString())) | |
| .withQueryParam(QUERY_PARAM_REPORT_TYPE, equalTo(StudyService.ReportType.DYNAMIC_SIMULATION.reportKey)) | |
| .withQueryParam(QUERY_PARAM_DEBUG, equalTo("true")) | |
| .withRequestBody(WireMock.equalToJson(objectMapper.writeValueAsString(events))) | |
| .withQueryParam("parametersUuid", equalTo(PARAMETERS_UUID.toString())) | |
| .withHeader(HEADER_USER_ID, equalTo("userId")) | |
| .willReturn(WireMock.serverError())); | |
| // check result | |
| assertThrows( | |
| HttpServerErrorException.class, | |
| () -> dynamicSimulationClient.run("receiver", NETWORK_UUID, "variantIdFailed", | |
| new ReportInfos(REPORT_UUID, NODE_UUID), PARAMETERS_UUID, events, "userId", true) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java`
around lines 316 - 352, The test's error path is a false positive because the
WireMock error stub incorrectly uses withHeader(QUERY_PARAM_DEBUG, ...) instead
of withQueryParam(QUERY_PARAM_DEBUG, ...) and doesn't verify the request body
(`events`), so WireMock falls back to 404; update both error stubs to use
withQueryParam("debug", equalTo("true")) (matching the real client behavior) and
add a request body matcher that verifies `events` (e.g.
withRequestBody(equalToJson(objectMapper.writeValueAsString(events)))); finally
change the asserted exception from HttpClientErrorException.NotFound.class to
HttpServerErrorException.class when calling dynamicSimulationClient.run(...) so
the test expects the 500 server error.
Signed-off-by: Thang PHAM <phamthang37@gmail.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)
src/main/java/org/gridsuite/study/server/controller/StudyController.java (1)
1905-1916:⚠️ Potential issue | 🟠 MajorFail fast on stale bodies for
/dynamic-simulation/run.This keeps the same public route, but it no longer consumes per-run parameters. The updated tests still POST a body and pass, which means older callers will silently lose their overrides instead of getting a clear contract error. Either keep backward compatibility for one release or reject non-empty bodies with
400.💡 One way to make the contract break explicit
public ResponseEntity<Void> runDynamicSimulation(`@Parameter`(description = "studyUuid") `@PathVariable`("studyUuid") UUID studyUuid, `@Parameter`(description = "rootNetworkUuid") `@PathVariable`("rootNetworkUuid") UUID rootNetworkUuid, `@Parameter`(description = "nodeUuid") `@PathVariable`("nodeUuid") UUID nodeUuid, `@Parameter`(description = "debug") `@RequestParam`(name = "debug", required = false, defaultValue = "false") boolean debug, + `@RequestBody`(required = false) String deprecatedParameters, `@RequestHeader`(HEADER_USER_ID) String userId) { + if (StringUtils.isNotBlank(deprecatedParameters)) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, + "Dynamic-simulation parameters must be set through the parameters endpoint before running"); + } studyService.assertIsNodeNotReadOnly(nodeUuid); studyService.assertCanRunOnConstructionNode(studyUuid, nodeUuid, List.of(DYNAWO_PROVIDER), studyService::getDynamicSimulationProvider); studyService.runDynamicSimulation(studyUuid, nodeUuid, rootNetworkUuid, userId, debug); return ResponseEntity.ok().contentType(MediaType.APPLICATION_JSON).build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java` around lines 1905 - 1916, The runDynamicSimulation endpoint currently ignores request bodies which lets old callers silently lose overrides; modify the StudyController.runDynamicSimulation method to fail-fast on non-empty bodies by adding a `@RequestBody`(required = false) String body parameter and, at the start of the method, if body != null && !body.isBlank() throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Request body not supported for this endpoint"); keep the existing calls to studyService.assertIsNodeNotReadOnly, assertCanRunOnConstructionNode and studyService.runDynamicSimulation unchanged so callers get a clear 400 when they send a body rather than silently dropping parameters.
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java (1)
104-105: Use a real JSON fixture in the parameter round-trip test.
"parametersJson"is plain text, not serialized JSON. Because the production path now treats these parameters as JSON payloads, this test won't catch regressions in JSON validation/serialization. Replacing it with an actual JSON object string would make the new contract much better covered.Also applies to: 689-723
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java` around lines 104 - 105, The test uses the constant PARAMETERS_JSON ("parametersJson") which is plain text rather than a serialized JSON payload, so update the fixture in StudyControllerDynamicSimulationTest to use a real JSON string (e.g., a JSON object with expected fields) for the parameter round-trip assertions; replace occurrences of PARAMETERS_JSON in the test (and the other instances noted around lines 689-723) with a properly escaped JSON object string that matches the production contract, ensuring the test sends/validates the same JSON structure the controller expects.
🤖 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/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.java`:
- Around line 105-110: The method
getDynamicSimulationParametersUuidOrElseCreateDefault in
DynamicSimulationServiceImpl must not perform a remote create during a read;
remove the call to dynamicSimulationClient.createDefaultParameters from that
getter and make it purely a read (return the UUID or null). Implement a separate
initialization/migration routine (e.g.,
initializeDynamicSimulationParametersIfMissing) that is invoked from
write/upgrade flows (or from StudyService during a controlled migration path)
and runs under a DB transaction or explicit lock to create the remote default
once, persist the returned UUID into the StudyEntity, and handle retry/race
conditions; ensure StudyService.getDynamicSimulationParameters calls the new
transactional initializer when migration is required rather than letting the
getter POST remotely.
---
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java`:
- Around line 1905-1916: The runDynamicSimulation endpoint currently ignores
request bodies which lets old callers silently lose overrides; modify the
StudyController.runDynamicSimulation method to fail-fast on non-empty bodies by
adding a `@RequestBody`(required = false) String body parameter and, at the start
of the method, if body != null && !body.isBlank() throw new
ResponseStatusException(HttpStatus.BAD_REQUEST, "Request body not supported for
this endpoint"); keep the existing calls to
studyService.assertIsNodeNotReadOnly, assertCanRunOnConstructionNode and
studyService.runDynamicSimulation unchanged so callers get a clear 400 when they
send a body rather than silently dropping parameters.
---
Nitpick comments:
In
`@src/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.java`:
- Around line 104-105: The test uses the constant PARAMETERS_JSON
("parametersJson") which is plain text rather than a serialized JSON payload, so
update the fixture in StudyControllerDynamicSimulationTest to use a real JSON
string (e.g., a JSON object with expected fields) for the parameter round-trip
assertions; replace occurrences of PARAMETERS_JSON in the test (and the other
instances noted around lines 689-723) with a properly escaped JSON object string
that matches the production contract, ensuring the test sends/validates the same
JSON structure the controller expects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffbbecc6-76be-4e84-bf10-058867204a93
📒 Files selected for processing (10)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.javasrc/test/java/org/gridsuite/study/server/StudyControllerDynamicSimulationTest.javasrc/test/java/org/gridsuite/study/server/service/StudyServiceDynamicSimulationTest.javasrc/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.javasrc/test/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/org/gridsuite/study/server/service/StudyServiceDynamicSimulationTest.java
- src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java
- src/main/java/org/gridsuite/study/server/service/StudyService.java
Comment on lines
+105
to
+110
| public UUID getDynamicSimulationParametersUuidOrElseCreateDefault(StudyEntity studyEntity) { | ||
| if (studyEntity.getDynamicSimulationParametersUuid() == null) { | ||
| // not supposed to happen because we create it as the study creation | ||
| studyEntity.setDynamicSimulationParametersUuid(dynamicSimulationClient.createDefaultParameters()); | ||
| } | ||
| return studyEntity.getDynamicSimulationParametersUuid(); |
There was a problem hiding this comment.
Don't lazily create remote default parameters from read paths.
getDynamicSimulationParametersUuidOrElseCreateDefault(...) performs a remote create when the UUID is null. StudyService.getDynamicSimulationParameters(...) already calls this helper from a GET flow (src/main/java/org/gridsuite/study/server/service/StudyService.java:2871-2876), so the first read on an older study now mutates remote state. This is also a check-then-act race: two concurrent requests can each create a default parameter set and only one UUID is retained locally. Please initialize/migrate this UUID once under a transaction/lock instead of opportunistically POSTing here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.java`
around lines 105 - 110, The method
getDynamicSimulationParametersUuidOrElseCreateDefault in
DynamicSimulationServiceImpl must not perform a remote create during a read;
remove the call to dynamicSimulationClient.createDefaultParameters from that
getter and make it purely a read (return the UUID or null). Implement a separate
initialization/migration routine (e.g.,
initializeDynamicSimulationParametersIfMissing) that is invoked from
write/upgrade flows (or from StudyService during a controlled migration path)
and runs under a DB transaction or explicit lock to create the remote default
once, persist the returned UUID into the StudyEntity, and handle retry/race
conditions; ensure StudyService.getDynamicSimulationParameters calls the new
transactional initializer when migration is required rather than letting the
getter POST remotely.
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