Skip to content

Conversation

@thangqp
Copy link

@thangqp thangqp commented Feb 4, 2026

PR Summary

  • To make it easier to view the results during monitoring development, we return a List<JsonNode>.
  • Note that the return type can be changed later when developing the front end.

Signed-off-by: Thang PHAM <phamthang37@gmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

The MonitorController was modified to deserialize string-based execution results into JSON nodes. Jackson dependencies were introduced, the constructor now accepts an ObjectMapper, and the getExecutionResults method's return type changed from List to List, with per-item parsing and error handling added.

Changes

Cohort / File(s) Summary
Result Deserialization
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java
Modified constructor to inject ObjectMapper; updated getExecutionResults to parse string results into JsonNode objects with JsonProcessingException error handling; changed return type from List to List.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A controller hops through strings so plain,
And turns them into JSON's domain,
With ObjectMapper's parsing grace,
Each result finds its proper place,
Data flowers bloom in structured rain! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title '[Monitor Server] Format results in List' clearly and specifically describes the main change—converting the results return type to List for improved viewing during monitoring development.
Description check ✅ Passed The description directly relates to the changeset, explaining the rationale for returning List and noting the return type may evolve with front-end development.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch monitor_server_format_results_json_node

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 74-80: The error message in MonitorController is misleading and
nulls can cause NPEs: change the exception text in the objectMapper.readValue
handler to say "Unable to deserialize result from JSON: <resultString>" (or
similar) and guard against null entries from results before calling readValue;
e.g., filter the results stream with Objects::nonNull or add an explicit null
check in the lambda so nulls are handled (throw a clear IllegalStateException
like "Result is null" or skip them), referencing the results variable, the
objectMapper.readValue(...) call, and the lambda in the stream.

Comment on lines +74 to +80
List<JsonNode> resultsJsonNode = results.stream().map(resultString -> {
try {
return objectMapper.readValue(resultString, JsonNode.class);
} catch (JsonProcessingException e) {
throw new IllegalStateException(String.format("Unable to serialize result to JSON: %s", resultString), e);
}
}).toList();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Error message incorrectly says "serialize" instead of "deserialize".

The operation readValue is deserialization (parsing JSON string to object), not serialization. Additionally, if resultString is null, this will throw a NullPointerException rather than the expected JsonProcessingException.

Proposed fix
         List<JsonNode> resultsJsonNode = results.stream().map(resultString -> {
             try {
                 return objectMapper.readValue(resultString, JsonNode.class);
             } catch (JsonProcessingException e) {
-                throw new IllegalStateException(String.format("Unable to serialize result to JSON: %s", resultString), e);
+                throw new IllegalStateException(String.format("Unable to deserialize result from JSON: %s", resultString), e);
             }
         }).toList();

If null results are possible from monitorService.getResults(), consider filtering them out or handling explicitly:

List<JsonNode> resultsJsonNode = results.stream()
    .filter(Objects::nonNull)
    .map(resultString -> { ... })
    .toList();
🤖 Prompt for AI Agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`
around lines 74 - 80, The error message in MonitorController is misleading and
nulls can cause NPEs: change the exception text in the objectMapper.readValue
handler to say "Unable to deserialize result from JSON: <resultString>" (or
similar) and guard against null entries from results before calling readValue;
e.g., filter the results stream with Objects::nonNull or add an explicit null
check in the lambda so nulls are handled (throw a clear IllegalStateException
like "Result is null" or skip them), referencing the results variable, the
objectMapper.readValue(...) call, and the lambda in the stream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant