Fix import parameters#990
Open
etiennehomer wants to merge 14 commits into
Open
Conversation
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
|
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:
📝 WalkthroughWalkthroughMigrates importParameters from Map<String,String> to Map<String,Object>. DTOs serialize objects to JSON strings for persistence; entities deserialize stored strings back to objects. Services, consumer, and tests updated; Liquibase change normalizes empty stored values. ChangesImport Parameters Type Migration
Sequence Diagram(s) sequenceDiagram
participant Consumer as ConsumerService
participant Service as RootNetworkService
participant Entity as RootNetworkEntity(DB)
Consumer->>Service: send importParameters (Map<String,Object>)
Service->>Service: serializeImportParameters -> Map<String,String>
Service->>Entity: persist importParameters (Map<String,String>)
Entity->>Entity: deserializeImportParameters -> Map<String,Object>
Entity->>Service: return DTO with importParameters (Map<String,Object>)
Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/study/server/service/RootNetworkService.java`:
- Line 187: The duplicated importParameters are being copied as
Map<String,Object> from a persisted Map<String,String> (newImportParameters =
Map.copyOf(rootNetworkEntityToDuplicate.getImportParameters())), which leads to
double-serialization when RootNetworkInfos.toEntity() writes them out; instead
transform the entries so String values that contain JSON are deserialized into
their native objects before storing in newImportParameters (e.g., iterate
rootNetworkEntityToDuplicate.getImportParameters(), for each entry: if value is
a String attempt to parse it with the project JSON mapper into Object, otherwise
keep as-is), then use that transformed map in place of the direct Map.copyOf to
prevent quoted strings becoming double-quoted JSON in
RootNetworkInfos.toEntity().
🪄 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: 0b5debd4-6097-4146-9ce5-258e59cd963f
📒 Files selected for processing (6)
src/main/java/org/gridsuite/study/server/dto/RootNetworkInfos.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.java
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/repository/rootnetwork/RootNetworkEntity.java (1)
76-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winChange
importParametersto lazy loadingThe
@ElementCollection(fetch = FetchType.EAGER)forces unnecessary loading of the separateimportParameterstable on allRootNetworkEntityqueries. Most operations that fetch root networks (e.g.,getNetworkUuid(),getCaseName(),getRootReportUuid()) only access basic properties and never need the import parameters. The single method that requires them (getImportParameters()) already explicitly loads them usingfindWithImportParametersById()with@EntityGraph. Change toFetchType.LAZYto avoid the overhead.Suggested change
- `@ElementCollection`(fetch = FetchType.EAGER) + `@ElementCollection`(fetch = FetchType.LAZY)🤖 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/repository/rootnetwork/RootNetworkEntity.java` around lines 76 - 80, Change the importParameters element collection in RootNetworkEntity from eager to lazy loading: replace `@ElementCollection`(fetch = FetchType.EAGER) on the importParameters field with `@ElementCollection`(fetch = FetchType.LAZY) so the separate importParameters table isn't always loaded; keep the existing findWithImportParametersById() / getImportParameters() usage (EntityGraph) to explicitly load parameters where needed. Ensure the field declaration name importParameters and class RootNetworkEntity remain unchanged so existing EntityGraph/query methods continue to work.
🤖 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/study/server/repository/rootnetwork/RootNetworkEntity.java`:
- Around line 117-121: In RootNetworkEntity where objectMapper.readValue(value,
Object.class) is called (the block that populates result.put(key, ...)), guard
against null values before attempting Jackson deserialization: if value == null,
put null (or the raw null value) into result for that key instead of calling
readValue, otherwise proceed to readValue and fall back to result.put(key,
value) in the existing JsonProcessingException catch; reference
objectMapper.readValue, result.put, and the surrounding try/catch to locate the
fix.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.java`:
- Around line 76-80: Change the importParameters element collection in
RootNetworkEntity from eager to lazy loading: replace `@ElementCollection`(fetch =
FetchType.EAGER) on the importParameters field with `@ElementCollection`(fetch =
FetchType.LAZY) so the separate importParameters table isn't always loaded; keep
the existing findWithImportParametersById() / getImportParameters() usage
(EntityGraph) to explicitly load parameters where needed. Ensure the field
declaration name importParameters and class RootNetworkEntity remain unchanged
so existing EntityGraph/query methods continue to work.
🪄 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: 0fdedd79-971f-4983-8f3f-9888cd45d7c9
📒 Files selected for processing (2)
src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/study/server/utils/JsonUtils.java (1)
80-80: ⚡ Quick winReuse ObjectMapper instance for better performance.
Creating a new
ObjectMapperinstance on every call todeserializeImportParametersandserializeImportParametersis inefficient.ObjectMapperis thread-safe and relatively expensive to construct. Consider using a shared static instance or injecting a singleton.♻️ Refactor to use static ObjectMapper
public final class JsonUtils { + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + private JsonUtils() { throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); } // ... existing methods ... public static Map<String, Object> deserializeImportParameters(Map<String, String> rawParams) { Map<String, Object> result = new HashMap<>(); if (rawParams == null) { return result; } - ObjectMapper objectMapper = new ObjectMapper(); rawParams.forEach((key, value) -> { if (value == null) { result.put(key, null); return; } try { - result.put(key, objectMapper.readValue(value, Object.class)); + result.put(key, OBJECT_MAPPER.readValue(value, Object.class)); } catch (JsonProcessingException e) { throw new StudyException(UNPROCESSABLE_IMPORT_PARAMETER, "Import parameter '" + key + " => " + value + "' is not valid JSON: " + e.getMessage()); } }); return result; } public static Map<String, String> serializeImportParameters(Map<String, Object> params) { Map<String, String> result = new HashMap<>(); if (params == null) { return result; } - ObjectMapper objectMapper = new ObjectMapper(); params.forEach((key, value) -> { try { - result.put(key, objectMapper.writeValueAsString(value)); + result.put(key, OBJECT_MAPPER.writeValueAsString(value)); } catch (JsonProcessingException e) { result.put(key, String.valueOf(value)); } }); return result; }Also applies to: 101-101
🤖 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/utils/JsonUtils.java` at line 80, Replace the per-call new ObjectMapper() allocations in JsonUtils by a shared, thread-safe singleton: add a private static final ObjectMapper (e.g., OBJECT_MAPPER) field on the JsonUtils class and initialize it once, then update deserializeImportParameters and serializeImportParameters to use JsonUtils.OBJECT_MAPPER instead of creating a new ObjectMapper; alternatively accept/inject a singleton ObjectMapper if preferred. Ensure any configuration (modules, visibility, date formats) currently applied to the local instances is applied once to the shared instance.src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.java (1)
76-76: 💤 Low valueConsider the performance impact of EAGER fetching.
Changing the fetch strategy to
FetchType.EAGERmeans all import parameters will be loaded whenever aRootNetworkEntityis retrieved, even if they're not needed. This could impact performance if import parameters are large or if entities are frequently loaded without needing the parameters.If import parameters are always required when loading the entity, EAGER fetch is appropriate. Otherwise, consider keeping the default LAZY fetch and explicitly join-fetch when needed.
🤖 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/repository/rootnetwork/RootNetworkEntity.java` at line 76, The annotation on the import-parameters collection in RootNetworkEntity currently uses FetchType.EAGER which may hurt performance; change the `@ElementCollection`(fetch = FetchType.EAGER) on the import parameters field in class RootNetworkEntity to FetchType.LAZY (or remove explicit fetch to use the default LAZY) and then update callers/repositories that need parameters to explicitly fetch them (e.g., add join fetch to the RootNetworkEntity repository queries or define an EntityGraph for methods that must load import parameters).
🤖 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/study/server/utils/JsonUtils.java`:
- Around line 95-110: The current serializeImportParameters method silently
falls back to String.valueOf(value) on JsonProcessingException which can store
non-JSON strings and break deserializeImportParameters; change
serializeImportParameters (in class JsonUtils) to fail-fast by removing the
String.valueOf fallback and instead catch JsonProcessingException and rethrow a
StudyException with a clear message including the key and original exception
(propagate the cause), so invalid objects are not written to the DB and
deserializeImportParameters can assume stored values are valid JSON.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.java`:
- Line 76: The annotation on the import-parameters collection in
RootNetworkEntity currently uses FetchType.EAGER which may hurt performance;
change the `@ElementCollection`(fetch = FetchType.EAGER) on the import parameters
field in class RootNetworkEntity to FetchType.LAZY (or remove explicit fetch to
use the default LAZY) and then update callers/repositories that need parameters
to explicitly fetch them (e.g., add join fetch to the RootNetworkEntity
repository queries or define an EntityGraph for methods that must load import
parameters).
In `@src/main/java/org/gridsuite/study/server/utils/JsonUtils.java`:
- Line 80: Replace the per-call new ObjectMapper() allocations in JsonUtils by a
shared, thread-safe singleton: add a private static final ObjectMapper (e.g.,
OBJECT_MAPPER) field on the JsonUtils class and initialize it once, then update
deserializeImportParameters and serializeImportParameters to use
JsonUtils.OBJECT_MAPPER instead of creating a new ObjectMapper; alternatively
accept/inject a singleton ObjectMapper if preferred. Ensure any configuration
(modules, visibility, date formats) currently applied to the local instances is
applied once to the shared instance.
🪄 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: a3024a42-1a4a-441b-9720-23c71eb091e2
📒 Files selected for processing (8)
src/main/java/org/gridsuite/study/server/dto/RootNetworkInfos.javasrc/main/java/org/gridsuite/study/server/error/StudyBusinessErrorCode.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkEntity.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkService.javasrc/main/java/org/gridsuite/study/server/utils/JsonUtils.javasrc/main/resources/db/changelog/changesets/changelog_20260519T112945Z.xmlsrc/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/db/changelog/changesets/changelog_20260519T112945Z.xml
- src/main/java/org/gridsuite/study/server/error/StudyBusinessErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java
- src/main/java/org/gridsuite/study/server/service/RootNetworkService.java
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
Signed-off-by: Etienne Homer <etiennehomer@gmail.com>
|
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