Conversation
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
…into add-endpoint-for-allcomputationstatus
📝 WalkthroughWalkthroughA new REST endpoint was added to retrieve computation statuses for a study node. The endpoint queries multiple computation status values from a service and returns them in a newly created DTO object as JSON. Changes
🚥 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. 📝 Coding Plan
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.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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
|
![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
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/ShortCircuitTest.java (1)
105-105: Prefer structural JSON assertions over exact JSON string literals.This constant drives a strict
content().string(...)comparison, which is fragile for JSON responses (field order/formatting changes can fail tests without behavior changes). Use JSON-aware assertions instead.💡 Suggested change
- content().string(ALL_COMPUTATIONS_STATUS_JSON)); + content().json(ALL_COMPUTATIONS_STATUS_JSON));🤖 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/ShortCircuitTest.java` at line 105, The test currently uses the fragile exact string constant ALL_COMPUTATIONS_STATUS_JSON in ShortCircuitTest for content().string(...); change the assertion to a structural JSON comparison instead—either replace content().string(ALL_COMPUTATIONS_STATUS_JSON) with MockMvcResultMatchers.content().json(ALL_COMPUTATIONS_STATUS_JSON) or parse both actual and expected into Jackson JsonNodes and compare with equals/JSONAssert; ensure you account for the embedded escaped JSON in the allBusShortCircuitStatus field (treat it as a string or parse it separately) so the structural comparison is robust to field order/formatting changes.
🤖 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/controller/StudyController.java`:
- Line 2510: allComputationStatus.setPccMinStatus(...) is being passed the full
PCC-min JSON document from rootNetworkNodeInfoService.getPccMinStatus(nodeUuid,
rootNetworkUuid) which results in a JSON blob being serialized; instead call
getPccMinStatus, extract the internal "status" token (e.g. via .get("status") or
parse the returned JSON/string and pull the "status" field), handle null/missing
cases (fallback to null or an UNKNOWN token), and pass that extracted status
string into allComputationStatus.setPccMinStatus so the DTO contains the status
token not the whole JSON.
- Around line 2504-2506: In getAllComputationsStatus in StudyController, the
studyUuid path variable is currently unused allowing cross-study access; add a
call to studyService.assertIsRootNetworkAndNodeInStudy(studyUuid,
rootNetworkUuid, nodeUuid) at the start of the method (before any call to
rootNetworkNodeInfoService) to validate the path tuple and abort if the
node/root network do not belong to the provided study.
In `@src/test/java/org/gridsuite/study/server/ShortCircuitTest.java`:
- Around line 281-286: The test currently verifies a single status call but the
/computations/status endpoint triggers two extra GETs; update the verification
of computationServerStubs.verifyGetResultStatus for
SHORT_CIRCUIT_ANALYSIS_RESULT_UUID to expect a total of 3 invocations (1 earlier
+ 2 from getShortCircuitAnalysisStatus) — e.g., change the call to use the
verification overload that asserts invocation count so it expects 3 calls for
SHORT_CIRCUIT_ANALYSIS_RESULT_UUID.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/study/server/ShortCircuitTest.java`:
- Line 105: The test currently uses the fragile exact string constant
ALL_COMPUTATIONS_STATUS_JSON in ShortCircuitTest for content().string(...);
change the assertion to a structural JSON comparison instead—either replace
content().string(ALL_COMPUTATIONS_STATUS_JSON) with
MockMvcResultMatchers.content().json(ALL_COMPUTATIONS_STATUS_JSON) or parse both
actual and expected into Jackson JsonNodes and compare with equals/JSONAssert;
ensure you account for the embedded escaped JSON in the allBusShortCircuitStatus
field (treat it as a string or parse it separately) so the structural comparison
is robust to field order/formatting changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1efafdb1-cb25-4c86-b880-a911f6c381fd
📒 Files selected for processing (5)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/NetworkMapService.javasrc/main/java/org/gridsuite/study/server/utils/AllComputationStatus.javasrc/test/java/org/gridsuite/study/server/PccMinTest.javasrc/test/java/org/gridsuite/study/server/ShortCircuitTest.java
| public ResponseEntity<AllComputationStatus> getAllComputationsStatus(@Parameter(description = "Study UUID") @PathVariable("studyUuid") UUID studyUuid, | ||
| @PathVariable("rootNetworkUuid") UUID rootNetworkUuid, | ||
| @Parameter(description = "nodeUuid") @PathVariable("nodeUuid") UUID nodeUuid) { |
There was a problem hiding this comment.
Validate the path tuple before reading statuses.
studyUuid is unused here, so a caller can pass any studyUuid and still read statuses for a valid rootNetworkUuid/nodeUuid pair from another study. Please add the same studyService.assertIsRootNetworkAndNodeInStudy(...) guard used elsewhere in this controller before querying rootNetworkNodeInfoService.
Suggested fix
public ResponseEntity<AllComputationStatus> getAllComputationsStatus(`@Parameter`(description = "Study UUID") `@PathVariable`("studyUuid") UUID studyUuid,
`@PathVariable`("rootNetworkUuid") UUID rootNetworkUuid,
`@Parameter`(description = "nodeUuid") `@PathVariable`("nodeUuid") UUID nodeUuid) {
+ studyService.assertIsRootNetworkAndNodeInStudy(studyUuid, rootNetworkUuid, nodeUuid);
AllComputationStatus allComputationStatus = new AllComputationStatus();🤖 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 2504 - 2506, In getAllComputationsStatus in StudyController, the
studyUuid path variable is currently unused allowing cross-study access; add a
call to studyService.assertIsRootNetworkAndNodeInStudy(studyUuid,
rootNetworkUuid, nodeUuid) at the start of the method (before any call to
rootNetworkNodeInfoService) to validate the path tuple and abort if the
node/root network do not belong to the provided study.
| AllComputationStatus allComputationStatus = new AllComputationStatus(); | ||
| LoadFlowStatus loadFlowStatus = rootNetworkNodeInfoService.getLoadFlowStatus(nodeUuid, rootNetworkUuid); | ||
| allComputationStatus.setLoadFlowStatus(loadFlowStatus == null ? null : loadFlowStatus.name()); | ||
| allComputationStatus.setPccMinStatus(rootNetworkNodeInfoService.getPccMinStatus(nodeUuid, rootNetworkUuid)); |
There was a problem hiding this comment.
Don't serialize the whole PCC-min JSON blob into pccMinStatus.
getPccMinStatus(...) returns the PCC-min status document, so this field becomes "{\"status\":\"COMPLETED\"}" in the aggregate response instead of a status token like the other properties. Please extract the actual "status" value before setting the DTO.
🤖 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` at
line 2510, allComputationStatus.setPccMinStatus(...) is being passed the full
PCC-min JSON document from rootNetworkNodeInfoService.getPccMinStatus(nodeUuid,
rootNetworkUuid) which results in a JSON blob being serialized; instead call
getPccMinStatus, extract the internal "status" token (e.g. via .get("status") or
parse the returned JSON/string and pull the "status" field), handle null/missing
cases (fallback to null or an UNKNOWN token), and pass that extracted status
string into allComputationStatus.setPccMinStatus so the DTO contains the status
token not the whole JSON.
| // get all computing status and check shortcircuit status | ||
| mockMvc.perform(get("/v1/studies/{studyUuid}/root-networks/{rootNetworkUuid}/nodes/{nodeUuid}/computations/status", studyNameUserIdUuid, firstRootNetworkUuid, modificationNode3Uuid)).andExpectAll( | ||
| status().isOk(), | ||
| content().string(ALL_COMPUTATIONS_STATUS_JSON)); | ||
| computationServerStubs.verifyGetResultStatus(SHORT_CIRCUIT_ANALYSIS_RESULT_UUID); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify default vs overload behavior of ComputationServerStubs.verifyGetResultStatus
# Expected: confirm whether no-count overload enforces exact 1 call or delegates to cumulative/at-least-one behavior.
set -euo pipefail
fd 'ComputationServerStubs\.java$' | while read -r f; do
echo "== $f =="
rg -n -C3 'verifyGetResultStatus\s*\(' "$f"
doneRepository: gridsuite/study-server
Length of output: 632
🏁 Script executed:
sed -n '270,295p' src/test/java/org/gridsuite/study/server/ShortCircuitTest.javaRepository: gridsuite/study-server
Length of output: 2243
🏁 Script executed:
rg -n 'computations/status' src/main/java --type javaRepository: gridsuite/study-server
Length of output: 260
🏁 Script executed:
sed -n '2501,2530p' src/main/java/org/gridsuite/study/server/controller/StudyController.javaRepository: gridsuite/study-server
Length of output: 3224
🏁 Script executed:
rg -n 'getShortCircuitAnalysisStatus' src/main/java --type java -A 5Repository: gridsuite/study-server
Length of output: 5986
🏁 Script executed:
sed -n '196,210p' src/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.javaRepository: gridsuite/study-server
Length of output: 640
🏁 Script executed:
rg -n 'stubGetResultStatus' src/test/java --type java -B 2 -A 2 | head -30Repository: gridsuite/study-server
Length of output: 3769
🏁 Script executed:
rg -n 'verifyGetResultStatus.*,' src/test/java --type java | head -20Repository: gridsuite/study-server
Length of output: 674
🏁 Script executed:
sed -n '2540,2570p' src/test/java/org/gridsuite/study/server/NetworkModificationTest.javaRepository: gridsuite/study-server
Length of output: 2328
🏁 Script executed:
rg -n 'verifyGetRequest' src/test/java/org/gridsuite/study/server/utils/wiremock --type java -A 5Repository: gridsuite/study-server
Length of output: 42635
🏁 Script executed:
rg -n 'static void verifyRequest' src/test/java/org/gridsuite/study/server/utils/wiremock --type java -A 10 | head -40Repository: gridsuite/study-server
Length of output: 3261
🏁 Script executed:
sed -n '110,130p' src/test/java/org/gridsuite/study/server/utils/wiremock/WireMockUtilsCriteria.javaRepository: gridsuite/study-server
Length of output: 1205
Update the verification call to expect cumulative status requests from the /computations/status endpoint.
The /computations/status endpoint calls getShortCircuitAnalysisStatus twice (once for ONE_BUS, once for ALL_BUSES), each triggering a GET request to /v1/results/{resultUuid}/status. Since line 279 already verified one status call, line 286 must account for the two additional calls made by the /computations/status endpoint.
Suggested fix
- computationServerStubs.verifyGetResultStatus(SHORT_CIRCUIT_ANALYSIS_RESULT_UUID);
+ computationServerStubs.verifyGetResultStatus(SHORT_CIRCUIT_ANALYSIS_RESULT_UUID, 2);📝 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.
| // get all computing status and check shortcircuit status | |
| mockMvc.perform(get("/v1/studies/{studyUuid}/root-networks/{rootNetworkUuid}/nodes/{nodeUuid}/computations/status", studyNameUserIdUuid, firstRootNetworkUuid, modificationNode3Uuid)).andExpectAll( | |
| status().isOk(), | |
| content().string(ALL_COMPUTATIONS_STATUS_JSON)); | |
| computationServerStubs.verifyGetResultStatus(SHORT_CIRCUIT_ANALYSIS_RESULT_UUID); | |
| // get all computing status and check shortcircuit status | |
| mockMvc.perform(get("/v1/studies/{studyUuid}/root-networks/{rootNetworkUuid}/nodes/{nodeUuid}/computations/status", studyNameUserIdUuid, firstRootNetworkUuid, modificationNode3Uuid)).andExpectAll( | |
| status().isOk(), | |
| content().string(ALL_COMPUTATIONS_STATUS_JSON)); | |
| computationServerStubs.verifyGetResultStatus(SHORT_CIRCUIT_ANALYSIS_RESULT_UUID, 2); |
🤖 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/ShortCircuitTest.java` around lines
281 - 286, The test currently verifies a single status call but the
/computations/status endpoint triggers two extra GETs; update the verification
of computationServerStubs.verifyGetResultStatus for
SHORT_CIRCUIT_ANALYSIS_RESULT_UUID to expect a total of 3 invocations (1 earlier
+ 2 from getShortCircuitAnalysisStatus) — e.g., change the call to use the
verification overload that asserts invocation count so it expects 3 calls for
SHORT_CIRCUIT_ANALYSIS_RESULT_UUID.
1 participant
PR Summary