import study - #202
Open
ghazwarhili wants to merge 32 commits into
Open
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multipart study archive import through a new controller endpoint, archive orchestration service, case and study REST clients, export metadata records, import error handling, and integration tests for successful and failing archives. ChangesStudy import
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExploreController
participant StudyImportService
participant CaseService
participant StudyService
Client->>ExploreController: POST multipart study archive
ExploreController->>ExploreController: assertCanCreateCase(userId)
ExploreController->>StudyImportService: importStudyArchive(archiveFile, metadata)
StudyImportService->>CaseService: importCaseFromFile(case file)
CaseService-->>StudyImportService: new case UUID
StudyImportService->>StudyService: importStudyWithCaseImportAction(updated study data)
StudyService-->>Client: import request accepted
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/explore/server/error/ExploreExceptionHandler.java (1)
37-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvide a non-null status for
IMPORT_STUDY_FAILED.
mapStatusreturnsnullforIMPORT_STUDY_FAILED, butAbstractBusinessExceptionHandlerdoes not define a null-safe fallback in the available code. Map this code to an explicitHttpStatusand add/extend a test so importing a study archive cannot crash the exception handler instead of returning a meaningful 4xx response.🤖 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/explore/server/error/ExploreExceptionHandler.java` around lines 37 - 42, The mapStatus method must return a non-null HTTP status for IMPORT_STUDY_FAILED. Replace its null mapping with the appropriate meaningful 4xx HttpStatus, and add or extend coverage to verify that importing a study archive produces that response without crashing the exception handler.
🤖 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/main/java/org/gridsuite/explore/server/services/StudyImportService.java`:
- Around line 78-90: Validate the resolved case file path in the rootNetwork
import loop before existence checks or reading: after resolving caseName from
caseDir, ensure it remains within caseDir using normalized paths and reject or
skip traversal attempts. Preserve the existing handling for valid regular case
files and log invalid paths consistently with the surrounding validation.
- Around line 223-235: Update deleteDirectory to manage the Stream<Path>
returned by Files.walk with try-with-resources, ensuring it is closed after
deletion processing while preserving the existing reverse-order traversal and
warning behavior for individual deletion failures.
- Around line 91-117: Update the case import flow in StudyImportService to track
each UUID returned by caseService.importCaseFromFile and delete all previously
imported cases when a later import fails or the final case-verification loop
detects a missing case. Ensure cleanup runs before rethrowing the existing
ExploreException and preserves the original failure outcome.
- Around line 167-188: Update extractArchive to enforce limits on the number of
ZIP entries and cumulative decompressed bytes while iterating through the
ZipInputStream, rejecting the archive with IOException once either bound is
exceeded. Count bytes as they are read and written, including directory entries
in the entry limit, and define or reuse clear configured constants for both
limits.
- Around line 58-65: Update importStudyArchive so failures from archive
extraction, validation, case imports, or importStudyWithCaseImportAction are
observable by ExploreController.importStudy instead of being hidden by `@Async`
void; either remove asynchronous execution and propagate the exception
synchronously, or implement an explicit status/failure notification mechanism
consumed by the caller.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/explore/server/error/ExploreExceptionHandler.java`:
- Around line 37-42: The mapStatus method must return a non-null HTTP status for
IMPORT_STUDY_FAILED. Replace its null mapping with the appropriate meaningful
4xx HttpStatus, and add or extend coverage to verify that importing a study
archive produces that response without crashing the exception handler.
🪄 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 Plus
Run ID: 984a1d5a-295f-43da-8c2f-60c53bb99951
📒 Files selected for processing (11)
src/main/java/org/gridsuite/explore/server/ExploreController.javasrc/main/java/org/gridsuite/explore/server/dto/CaseExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/NodeTreeExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/RootNetworkExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/StudyExportInfos.javasrc/main/java/org/gridsuite/explore/server/error/ExploreBusinessErrorCode.javasrc/main/java/org/gridsuite/explore/server/error/ExploreExceptionHandler.javasrc/main/java/org/gridsuite/explore/server/services/CaseService.javasrc/main/java/org/gridsuite/explore/server/services/ExploreService.javasrc/main/java/org/gridsuite/explore/server/services/StudyImportService.javasrc/main/java/org/gridsuite/explore/server/services/StudyService.java
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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/main/java/org/gridsuite/explore/server/ExploreController.java (1)
88-99: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAsync import exceptions cannot be returned with the current endpoint contract.
StudyImportService.importStudyArchiveis@Async void, so validation/import failures are logged by Spring’s async uncaught handler, notExploreExceptionHandler; this endpoint returns200 OKbefore the archive is processed. Make the validation/import synchronous when the tests depend on immediate 5xx failures, or return202 Acceptedwith a job-status signal.🤖 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/explore/server/ExploreController.java` around lines 88 - 99, The importStudy endpoint currently returns 200 OK before the `@Async` void StudyImportService.importStudyArchive operation can report validation or import failures. Make the import validation/processing synchronous so exceptions propagate through ExploreExceptionHandler and produce immediate 5xx responses, updating StudyImportService and its callers as needed; adjust the affected assertions in src/test/java/org/gridsuite/explore/server/StudyImportExportTest.java lines 109-224 to verify the synchronous error responses, with no direct change required if covered by the service fix.
🤖 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/explore/server/StudyImportExportTest.java`:
- Around line 74-95: Isolate the Spring context used by StudyImportExportTest so
singleton StudyService and CaseService do not retain per-test WireMock URLs
after tearDown. Restore each service’s original base URI after the test
lifecycle, or mark the test context dirty after the class; preserve the existing
WireMock setup and cleanup behavior.
---
Outside diff comments:
In `@src/main/java/org/gridsuite/explore/server/ExploreController.java`:
- Around line 88-99: The importStudy endpoint currently returns 200 OK before
the `@Async` void StudyImportService.importStudyArchive operation can report
validation or import failures. Make the import validation/processing synchronous
so exceptions propagate through ExploreExceptionHandler and produce immediate
5xx responses, updating StudyImportService and its callers as needed; adjust the
affected assertions in
src/test/java/org/gridsuite/explore/server/StudyImportExportTest.java lines
109-224 to verify the synchronous error responses, with no direct change
required if covered by the service fix.
🪄 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 Plus
Run ID: d6c90234-dc12-4ea5-a5d3-44b507f5e040
📒 Files selected for processing (9)
src/main/java/org/gridsuite/explore/server/ExploreController.javasrc/main/java/org/gridsuite/explore/server/dto/CaseExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/NodeTreeExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/RootNetworkExportInfos.javasrc/main/java/org/gridsuite/explore/server/dto/StudyExportInfos.javasrc/main/java/org/gridsuite/explore/server/error/ExploreExceptionHandler.javasrc/main/java/org/gridsuite/explore/server/services/ExploreService.javasrc/main/java/org/gridsuite/explore/server/services/StudyImportService.javasrc/test/java/org/gridsuite/explore/server/StudyImportExportTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/explore/server/services/StudyImportService.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/gridsuite/explore/server/dto/NodeTreeExportInfos.java
- src/main/java/org/gridsuite/explore/server/dto/CaseExportInfos.java
- src/main/java/org/gridsuite/explore/server/dto/RootNetworkExportInfos.java
- src/main/java/org/gridsuite/explore/server/dto/StudyExportInfos.java
There was a problem hiding this comment.
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/explore/server/services/StudyService.java (1)
115-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEncode imported study metadata in the URI builder.
RestTemplateConfigleaves the defaultRestTemplateBuilderencoding in place, and the existing import tests only use plain names/descriptions. Addencode()here sostudyNameanddescriptionremain intact when they contain spaces or query-signal characters, and cover those edge values in the import tests.- String path = uriComponentsBuilder.buildAndExpand(caseUuid).toUriString(); + String path = uriComponentsBuilder.encode().buildAndExpand(caseUuid).toUriString();🤖 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/explore/server/services/StudyService.java` around lines 115 - 123, Update the UriComponentsBuilder chain in StudyService to call encode() before buildAndExpand, ensuring studyName and description preserve spaces and query-significant characters when constructing the import URI. Extend the existing import tests with names and descriptions containing those edge characters.
🤖 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.
Outside diff comments:
In `@src/main/java/org/gridsuite/explore/server/services/StudyService.java`:
- Around line 115-123: Update the UriComponentsBuilder chain in StudyService to
call encode() before buildAndExpand, ensuring studyName and description preserve
spaces and query-significant characters when constructing the import URI. Extend
the existing import tests with names and descriptions containing those edge
characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 469c1f99-dffc-4a88-850f-8bec487be0a7
📒 Files selected for processing (4)
src/main/java/org/gridsuite/explore/server/error/ExploreException.javasrc/main/java/org/gridsuite/explore/server/services/StudyImportService.javasrc/main/java/org/gridsuite/explore/server/services/StudyService.javasrc/test/java/org/gridsuite/explore/server/StudyImportExportTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/org/gridsuite/explore/server/StudyImportExportTest.java
- src/main/java/org/gridsuite/explore/server/services/StudyImportService.java
# Conflicts: # src/main/java/org/gridsuite/explore/server/ExploreController.java # src/main/java/org/gridsuite/explore/server/services/StudyService.java
EtienneLt
reviewed
Aug 17, 2026
|
|
||
| Map<UUID, UUID> caseUuidMapping = new HashMap<>(); | ||
| try { | ||
| Path casesDir = tempDir.resolve("cases"); |
| private void importStudyFromArchive(MultipartFile archiveFile, String studyName, String description, String userId, | ||
| UUID parentDirectoryUuid, Path tempDir) throws IOException { | ||
| extractArchive(archiveFile.getInputStream(), tempDir); | ||
| Path studyJsonPath = tempDir.resolve("tree.json"); |
| extractArchive(archiveFile.getInputStream(), tempDir); | ||
| Path studyJsonPath = tempDir.resolve("tree.json"); | ||
| if (!Files.exists(studyJsonPath)) { | ||
| throw new ExploreException(IMPORT_STUDY_FAILED, "tree.json not found in archive"); |
Contributor
There was a problem hiding this comment.
replace tree.json with constant
| try { | ||
| Path casesDir = tempDir.resolve("cases"); | ||
| if (!Files.exists(casesDir) || !Files.isDirectory(casesDir)) { | ||
| throw new ExploreException(IMPORT_STUDY_FAILED, "No cases found in archive"); |
This reverts commit 1a02fb5.
etiennehomer
requested changes
Sep 1, 2026
| return caseUuid; | ||
| } | ||
|
|
||
| public UUID importCaseFromFile(File file) { |
Collaborator
There was a problem hiding this comment.
UUID importMultipartCase(MultipartFile multipartFile) {
if (multipartFile != null) {
Objects.requireNonNull(multipartFile.getOriginalFilename());
return importCaseResource(multipartFile.getResource());
}
return importCaseResource(null);
}
public UUID importFileCase(File file) {
return importCaseResource(new org.springframework.core.io.FileSystemResource(file));
}
private UUID importCaseResource(org.springframework.core.io.Resource resource) {
MultiValueMap<String, Object> body = new LinkedMultiValueMap<>();
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.MULTIPART_FORM_DATA);
if (resource != null) {
body.add("file", resource);
}
HttpEntity<MultiValueMap<String, Object>> request = new HttpEntity<>(body, headers);
return restTemplate.postForObject(
caseServerBaseUri + "/" + CASE_SERVER_API_VERSION + "/cases",
request,
UUID.class
);
}
do something like this
| Map<UUID, UUID> caseUuidMapping = new HashMap<>(); | ||
| try { | ||
| Path casesDir = tempDir.resolve("cases"); | ||
| if (!Files.exists(casesDir) || !Files.isDirectory(casesDir)) { |
| throw new ExploreException(IMPORT_STUDY_FAILED, "No root networks found in archive"); | ||
| } | ||
|
|
||
| Map<UUID, UUID> caseUuidMapping = new HashMap<>(); |
Collaborator
There was a problem hiding this comment.
Suggested change
| Map<UUID, UUID> caseUuidMapping = new HashMap<>(); | |
| Map<UUID, UUID> oldCaseUuidToNewCaseUuid = new HashMap<>(); |
| } | ||
|
|
||
| TreeExportInfos treeExportInfos = objectMapper.readValue(studyJsonPath.toFile(), TreeExportInfos.class); | ||
| if (treeExportInfos == null || treeExportInfos.rootNetworks() == null || treeExportInfos.rootNetworks().isEmpty()) { |
| String caseName = rootNetwork.caseInfos().caseName(); | ||
| Path caseDir = casesDir.resolve(oldCaseUuid.toString()); | ||
|
|
||
| if (!Files.exists(caseDir) || !Files.isDirectory(caseDir)) { |
| } | ||
|
|
||
| Path caseFile = caseDir.resolve(caseName); | ||
| if (!Files.exists(caseFile) || !Files.isRegularFile(caseFile)) { |
| } | ||
| try { | ||
| UUID newCaseUuid = caseService.importCaseFromFile(caseFile.toFile()); | ||
| if (newCaseUuid == null) { |
| * Extract zip archive to directory | ||
| */ | ||
| private void extractArchive(InputStream inputStream, Path destDir) throws IOException { | ||
| try (ZipInputStream zipIn = new ZipInputStream(inputStream)) { |
Collaborator
There was a problem hiding this comment.
|
etiennehomer
requested changes
Sep 9, 2026
| UUID importDirectoryUuid = newElementAttributes.getElementUuid(); | ||
| try { | ||
| Path casesDir = tempDir.resolve("cases"); | ||
| for (var rootNetwork : treeExportInfos.rootNetworks()) { |
Collaborator
There was a problem hiding this comment.
the case and rootnetwork treatment looks to complicated and too verbose.
Fill the case uuid after each import in the TreeExportInfos. No need to keep a map, no need to checkAllCasesWereImported()
| /** | ||
| * Update case UUIDs in StudyExportInfos with new imported case UUIDs | ||
| */ | ||
| private TreeExportInfos updateCaseUuidsAndStudyUuidInExportInfos(TreeExportInfos original, Map<UUID, UUID> oldCaseUuidToNewCaseUuid, UUID createdStudyUuid) { |
Collaborator
There was a problem hiding this comment.
Too much code to only change one UUID.
I think you can remove this method with better conception
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
4 participants
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
Export import study