Refactor duplication endpoints#1014
Open
carojeandat wants to merge 2 commits into
Open
Conversation
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
📝 WalkthroughWalkthroughDuplicate operations across the study server are migrated from query-parameter-based endpoints ( ChangesDuplicate endpoint path migration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
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/test/java/org/gridsuite/study/server/SpreadsheetConfigCollectionTest.java (1)
161-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the duplicate dispatcher ID parsing.
path.substring(path.lastIndexOf("=") + 1)still assumes the old?duplicateFrom=<id>format. With/v1/spreadsheet-config-collections/<id>/duplicate, there is no=, so the"non-existing-collection"branch never matches and the intended 404 path is lost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/SpreadsheetConfigCollectionTest.java` around lines 161 - 166, The duplicate dispatcher in SpreadsheetConfigCollectionTest is still extracting the collection ID using the old query-string parsing, so the non-existing collection case is never matched for the new /duplicate path. Update the ID extraction logic in the mock dispatcher branch that handles the duplicate POST route to derive the identifier from the path segments instead of using lastIndexOf("="), and keep the existing non-existing-collection 404 check working against that parsed ID.
🧹 Nitpick comments (4)
src/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.java (1)
107-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
getParametersWithUuidUrlto avoid duplicated URL-building.
duplicateParametersre-derivesparametersBaseUrland rebuilds the{uuid}segment instead of reusing the existing helper.♻️ Suggested simplification
public UUID duplicateParameters(`@NonNull` UUID sourceParametersUuid) { Objects.requireNonNull(sourceParametersUuid); - String parametersBaseUrl = buildEndPointUrl(getBaseUri(), DYNAMIC_SECURITY_ANALYSIS_API_VERSION, DYNAMIC_SECURITY_ANALYSIS_END_POINT_PARAMETER); - - String url = UriComponentsBuilder.fromUriString(parametersBaseUrl + "/{uuid}/duplicate") - .buildAndExpand(sourceParametersUuid) - .toUriString(); + String url = getParametersWithUuidUrl(sourceParametersUuid) + "/duplicate"; return getRestTemplate().postForObject(url, null, UUID.class); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.java` around lines 107 - 117, The duplicate URL construction in duplicateParameters should be simplified by reusing the existing getParametersWithUuidUrl helper instead of rebuilding parametersBaseUrl and the {uuid} path segment locally. Update duplicateParameters in DynamicSecurityAnalysisClient to derive the duplicate endpoint from getParametersWithUuidUrl(sourceParametersUuid) and append the /duplicate suffix there, keeping the UUID handling and REST call behavior unchanged.src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java (1)
227-227: 🎯 Functional Correctness | 🔵 TrivialConsider
urlPathEqualToinstead ofurlPathTemplatefor a fully-resolved path.
urlPathTemplateis intended for RFC 6570 templates with{variable}placeholders matched via.withPathParam(...); here the path is already fully concrete (PARAMETERS_UUIDis interpolated as a literal value), sourlPathEqualTowould be the more idiomatic matcher. Functionally this should still behave as an exact match since there are no path variables, so this is purely a clarity nit, and the same pattern is already used consistently in sibling dynamic-client tests.Also applies to: 240-240, 250-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java` at line 227, The WireMock stubs in DynamicMarginCalculationClientTest are using urlPathTemplate for a fully concrete endpoint path, which is clearer as an exact match with urlPathEqualTo. Update the stub setups in the test methods around the duplicate/delete-style endpoints to use urlPathEqualTo instead of urlPathTemplate, keeping the same PARAMETERS_BASE_URL, DELIMITER, and PARAMETERS_UUID path construction so the matcher remains an exact literal path match.src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java (1)
234-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisuse of
urlPathTemplatefor a non-templated literal path.
urlPathTemplateis designed for RFC 6570 path templates containing{var}placeholders (e.g./{uuid}/duplicate), enabling path-variable matching. Here the UUID is already interpolated into the string before being passed in, so there are no template variables left — the same idiom used elsewhere in this file for literal paths isWireMock.urlPathEqualTo(url)(e.g.testGetProvider,testGetParameters). While this will likely still match as a literal string, it deviates from the documented usage pattern and is inconsistent with the rest of the test class.♻️ Suggested fix
- wireMockServer.stubFor(WireMock.post(WireMock.urlPathTemplate(PARAMETERS_BASE_URL + "/" + PARAMETERS_UUID + "/duplicate")) + wireMockServer.stubFor(WireMock.post(WireMock.urlPathEqualTo(PARAMETERS_BASE_URL + "/" + PARAMETERS_UUID + "/duplicate"))The same pattern is repeated in the sibling
DynamicMarginCalculationClientTestandDynamicSecurityAnalysisClientTestfiles per the provided graph context.Also applies to: 247-247, 257-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java` at line 234, Replace the misuse of WireMock.urlPathTemplate in the duplicate stubs with the literal-path matcher used elsewhere in DynamicSimulationClientTest, since the PARAMETERS_UUID is already interpolated and no template variables remain. Update the duplicate request setup in the corresponding duplicate-related test methods to use WireMock.urlPathEqualTo with the fully built URL, and apply the same change in the sibling DynamicMarginCalculationClientTest and DynamicSecurityAnalysisClientTest test cases that use the same pattern.src/main/java/org/gridsuite/study/server/service/StudyConfigService.java (1)
127-135: 📐 Maintainability & Code Quality | 🔵 TrivialUse templated path variables instead of raw string concatenation for the UUID.
duplicateSpreadsheetConfigCollectionandduplicateWorkspacesConfigconcatenatesourceUuiddirectly into the path string and callbuildAndExpand()with no arguments, unlikeduplicateNetworkVisualizationParameters(and the rest of the class) which uses a{uuid}placeholder +buildAndExpand(uuid). Functionally safe today sinceUUID#toString()never needs escaping, but it's inconsistent with the class's established pattern.♻️ Proposed fix for consistency
public UUID duplicateSpreadsheetConfigCollection(UUID sourceUuid) { Objects.requireNonNull(sourceUuid); - var path = UriComponentsBuilder.fromPath(DELIMITER + STUDY_CONFIG_API_VERSION + SPREADSHEET_CONFIG_COLLECTION_URI + DELIMITER + sourceUuid + "/duplicate") - .buildAndExpand().toUriString(); + var path = UriComponentsBuilder.fromPath(DELIMITER + STUDY_CONFIG_API_VERSION + SPREADSHEET_CONFIG_COLLECTION_URI + "/{uuid}/duplicate") + .buildAndExpand(sourceUuid).toUriString();public UUID duplicateWorkspacesConfig(UUID sourceUuid) { Objects.requireNonNull(sourceUuid); - var path = UriComponentsBuilder.fromPath(DELIMITER + STUDY_CONFIG_API_VERSION + WORKSPACES_CONFIG_URI + DELIMITER + sourceUuid + "/duplicate") - .toUriString(); + var path = UriComponentsBuilder.fromPath(DELIMITER + STUDY_CONFIG_API_VERSION + WORKSPACES_CONFIG_URI + "/{uuid}/duplicate") + .buildAndExpand(sourceUuid).toUriString();Also applies to: 332-337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/study/server/service/StudyConfigService.java` around lines 127 - 135, `duplicateSpreadsheetConfigCollection` is building the URI by concatenating `sourceUuid` directly into the path instead of using the class’s templated `{uuid}` pattern. Update this method, and the matching `duplicateWorkspacesConfig` path construction, to use a placeholder plus `buildAndExpand(...)` like `duplicateNetworkVisualizationParameters` does, so the URI assembly is consistent across `StudyConfigService`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.java`:
- Around line 786-788: The verifyNetworkAreaDiagramConfig helper is using a path
pattern that is currently treated as a literal string, so it will not match
duplicate-config requests. Update this verifier to enable regex matching when
calling verifyPostRequest, keeping the existing NetworkAreaDiagram duplicate
endpoint pattern and the stubId parameter so the helper matches the intended
request.
---
Outside diff comments:
In
`@src/test/java/org/gridsuite/study/server/SpreadsheetConfigCollectionTest.java`:
- Around line 161-166: The duplicate dispatcher in
SpreadsheetConfigCollectionTest is still extracting the collection ID using the
old query-string parsing, so the non-existing collection case is never matched
for the new /duplicate path. Update the ID extraction logic in the mock
dispatcher branch that handles the duplicate POST route to derive the identifier
from the path segments instead of using lastIndexOf("="), and keep the existing
non-existing-collection 404 check working against that parsed ID.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.java`:
- Around line 107-117: The duplicate URL construction in duplicateParameters
should be simplified by reusing the existing getParametersWithUuidUrl helper
instead of rebuilding parametersBaseUrl and the {uuid} path segment locally.
Update duplicateParameters in DynamicSecurityAnalysisClient to derive the
duplicate endpoint from getParametersWithUuidUrl(sourceParametersUuid) and
append the /duplicate suffix there, keeping the UUID handling and REST call
behavior unchanged.
In `@src/main/java/org/gridsuite/study/server/service/StudyConfigService.java`:
- Around line 127-135: `duplicateSpreadsheetConfigCollection` is building the
URI by concatenating `sourceUuid` directly into the path instead of using the
class’s templated `{uuid}` pattern. Update this method, and the matching
`duplicateWorkspacesConfig` path construction, to use a placeholder plus
`buildAndExpand(...)` like `duplicateNetworkVisualizationParameters` does, so
the URI assembly is consistent across `StudyConfigService`.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClientTest.java`:
- Line 227: The WireMock stubs in DynamicMarginCalculationClientTest are using
urlPathTemplate for a fully concrete endpoint path, which is clearer as an exact
match with urlPathEqualTo. Update the stub setups in the test methods around the
duplicate/delete-style endpoints to use urlPathEqualTo instead of
urlPathTemplate, keeping the same PARAMETERS_BASE_URL, DELIMITER, and
PARAMETERS_UUID path construction so the matcher remains an exact literal path
match.
In
`@src/test/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClientTest.java`:
- Line 234: Replace the misuse of WireMock.urlPathTemplate in the duplicate
stubs with the literal-path matcher used elsewhere in
DynamicSimulationClientTest, since the PARAMETERS_UUID is already interpolated
and no template variables remain. Update the duplicate request setup in the
corresponding duplicate-related test methods to use WireMock.urlPathEqualTo with
the fully built URL, and apply the same change in the sibling
DynamicMarginCalculationClientTest and DynamicSecurityAnalysisClientTest test
cases that use the same pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8495158c-54fd-4b5c-bb2a-b37c6e84f567
📒 Files selected for processing (26)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/CaseService.javasrc/main/java/org/gridsuite/study/server/service/LoadFlowService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/PccMinService.javasrc/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StateEstimationService.javasrc/main/java/org/gridsuite/study/server/service/StudyConfigService.javasrc/main/java/org/gridsuite/study/server/service/VoltageInitService.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/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.javasrc/test/java/org/gridsuite/study/server/PccMinTest.javasrc/test/java/org/gridsuite/study/server/SpreadsheetConfigCollectionTest.javasrc/test/java/org/gridsuite/study/server/VoltageInitTest.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/studycontroller/StudyTest.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/CaseServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/ComputationServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/LoadflowServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/WireMockUtils.java
Comment on lines
786
to
788
| public void verifyNetworkAreaDiagramConfig(UUID stubId) { | ||
| verifyPostRequest(wireMock, stubId, "/v1/network-area-diagram/config", Map.of("duplicateFrom", WireMock.matching(".*"))); | ||
| verifyPostRequest(wireMock, stubId, "/v1/network-area-diagram/config/.*/duplicate", Map.of()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of verifyNetworkAreaDiagramConfig to assess whether this bug is currently exercised.
rg -n "verifyNetworkAreaDiagramConfig" -g '*.java'Repository: gridsuite/study-server
Length of output: 1996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'WireMockStubs.java|WireMockUtils.java' src/test/java src/main/java 2>/dev/null || true
echo
echo "== find relevant symbols =="
rg -n "verifyNetworkAreaDiagramConfig|verifyPostRequest\\(" src/test/java src/main/java -g '*.java' || true
echo
echo "== show relevant sections from WireMockStubs.java =="
file=$(fd -a 'WireMockStubs.java' src/test/java 2>/dev/null | head -n 1 || true)
if [ -n "${file:-}" ]; then
line=$(rg -n "verifyNetworkAreaDiagramConfig" "$file" | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-12))
end=$((line+12))
sed -n "${start},${end}p" "$file"
fi
fi
echo
echo "== show relevant sections from WireMockUtils.java =="
file2=$(fd -a 'WireMockUtils.java' src/test/java 2>/dev/null | head -n 1 || true)
if [ -n "${file2:-}" ]; then
rg -n "verifyPostRequest\\(" "$file2" || true
sed -n '1,260p' "$file2" | sed -n '/verifyPostRequest(/,/^}/p'
fiRepository: gridsuite/study-server
Length of output: 1967
Pass regexMatching=true for this verifier
/v1/network-area-diagram/config/.*/duplicate is treated as a literal path unless regex matching is enabled, so this helper will never match a real duplicate-config request as written.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.java`
around lines 786 - 788, The verifyNetworkAreaDiagramConfig helper is using a
path pattern that is currently treated as a literal string, so it will not match
duplicate-config requests. Update this verifier to enable regex matching when
calling verifyPostRequest, keeping the existing NetworkAreaDiagram duplicate
endpoint pattern and the stubId parameter so the helper matches the intended
request.
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