Refactor createChildReport endpoint to use server-generated identifiers#154
Merged
Refactor createChildReport endpoint to use server-generated identifiers#154
Conversation
Signed-off-by: achour94 <berrahmaachour@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors child-report creation from a client-supplied PUT to a server-generated POST: controller endpoint and responses changed, service now returns the generated child UUID and its helper persists IDs internally, and tests updated to assert the new behavior (404/409 mappings preserved). ChangesChild Report Creation Redesign
sequenceDiagram
participant Client as Client
participant Controller as ReportController
participant Service as ReportService
participant Repo as ReportRepository
Client->>Controller: POST /reports/{rootId}/children + ReportNode
Controller->>Service: createChildReport(rootId, reportNode)
Service->>Repo: persist new child entity (generate ID)
Repo-->>Service: persisted child entity (id)
Service-->>Controller: return generated child UUID
Controller-->>Client: 201 Created + UUID
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/test/java/org/gridsuite/report/server/ReportControllerTest.java (1)
172-201: ⚡ Quick winMissing HTTP-level test for the 409 response (non-root target).
The controller explicitly documents a
409 Conflictresponse and mapsIllegalStateExceptionto it, but there is no controller test that exercises this path. The service-level test covers the exception, but not the HTTP status code mapping. Adding a test that POSTs tochildrenusing a non-root ID as{rootId}would close this gap with minimal effort.💡 Suggested test skeleton
`@Test` public void testCreateChildReportEndpointReturnsConflictForNonRootTarget() throws Exception { // insert a root report that has at least one child insertReport(REPORT_UUID, toString(REPORT_ONE)); // get the ID of a direct child of the root (a non-root node) Report rootReport = objectMapper.readValue( mvc.perform(get(URL_TEMPLATE + "/reports/" + REPORT_UUID)) .andExpect(status().isOk()).andReturn() .getResponse().getContentAsString(), Report.class); UUID nonRootId = rootReport.getSubReports().get(0).getId(); mvc.perform(post(URL_TEMPLATE + "/reports/" + nonRootId + "/children") .content(toString(REPORT_TWO)) .contentType(APPLICATION_JSON)) .andExpect(status().isConflict()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/report/server/ReportControllerTest.java` around lines 172 - 201, Add a new controller test method (e.g., testCreateChildReportEndpointReturnsConflictForNonRootTarget) that inserts the root via insertReport(REPORT_UUID, toString(REPORT_ONE)), retrieves the root with mvc.perform(get(URL_TEMPLATE + "/reports/" + REPORT_UUID)) and objectMapper to parse a Report, grab a non-root id from rootReport.getSubReports().get(0).getId(), then POST to mvc.perform(post(URL_TEMPLATE + "/reports/" + nonRootId + "/children").content(toString(REPORT_TWO)).contentType(APPLICATION_JSON)) and assert status().isConflict(); use existing helpers/fields insertReport, mvc, objectMapper, URL_TEMPLATE, REPORT_ONE and REPORT_TWO to locate and implement the test.src/test/java/org/gridsuite/report/server/ReportServiceTest.java (1)
181-233: ⚡ Quick winNew tests are correct; consider adding SQL count assertions for consistency.
The two new tests properly cover the happy path and the non-root-target error path. However, unlike every other test in this file, they don't reset
SQLStatementCountValidatoror callassertRequestsCount. ForcreateChildReportGeneratesIdAndAppendsAsChildin particular — which exercises the full persist-and-flush path — asserting the expected query counts would catch unintentional N+1 or extra write regressions in this new code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/report/server/ReportServiceTest.java` around lines 181 - 233, These two new tests (createChildReportGeneratesIdAndAppendsAsChild and createChildReportFailsWhenTargetIsNotARootReport) are missing the SQL statement count checks used elsewhere; reset the SQLStatementCountValidator at test start and call the existing assertRequestsCount with the expected select/insert/update counts after exercising reportService.createChildReport to verify no extra queries or writes were introduced (mirror the pattern used in other tests in this class).src/main/java/org/gridsuite/report/server/ReportController.java (1)
156-172: 💤 Low valueThe 409
@ApiResponsedescription is slightly ambiguous after the refactor.Line 161's description
"Provided id does not refer to a root report"referred to the old, explicitly supplied child ID. In the new API, the relevant "provided id" isrootId(a path variable), not a body-level child ID. Consider clarifying:"The rootId path variable does not refer to a root report".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/report/server/ReportController.java` around lines 156 - 172, Update the `@ApiResponse` description for the 409 response in createChildReport to refer to the path variable rootId rather than a body-level child id; locate the `@ApiResponse` annotation near the createChildReport method and change the message from "Provided id does not refer to a root report" to something explicit like "The rootId path variable does not refer to a root report" so the documentation matches the refactored API semantics.src/main/java/org/gridsuite/report/server/ReportService.java (1)
254-272: Consider matching the UUID generation pattern used induplicateReportfor consistency.
childReportEntityis built without an explicit.id(...)call; instead,ReportNodeEntityuses Lombok's@Builder.Defaultwithprivate UUID id = UUID.randomUUID()to assign a UUID during builder construction. This is functionally correct andchildReportEntity.getId()at line 272 will be non-null. However, for consistency with the explicit UUID assignment pattern used elsewhere in the codebase (duplicateReport,createNewReport), consider generating the UUID explicitly before building:+ UUID childId = UUID.randomUUID(); ReportNodeEntity childReportEntity = ReportNodeEntity.builder() + .id(childId) .message(sizedChildReportNode.getMessage()) ... .build(); entitiesToSave.add(childReportEntity); ... if (!entitiesToSave.isEmpty()) { self.saveBatchedReports(entitiesToSave); } - return childReportEntity.getId(); + return childId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/report/server/ReportService.java` around lines 254 - 272, The child ReportNodeEntity is relying on a `@Builder.Default` UUID rather than following the explicit UUID assignment pattern used in duplicateReport/createNewReport; generate a UUID explicitly before building (e.g., UUID childId = UUID.randomUUID()) and pass it to the ReportNodeEntity builder via .id(childId) when constructing childReportEntity so the new node's id is assigned consistently with other creation paths (references: ReportNodeEntity, sizedChildReportNode, childReportEntity, saveReportNodeRecursively, self.saveBatchedReports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/gridsuite/report/server/ReportController.java`:
- Around line 156-172: Update the `@ApiResponse` description for the 409 response
in createChildReport to refer to the path variable rootId rather than a
body-level child id; locate the `@ApiResponse` annotation near the
createChildReport method and change the message from "Provided id does not refer
to a root report" to something explicit like "The rootId path variable does not
refer to a root report" so the documentation matches the refactored API
semantics.
In `@src/main/java/org/gridsuite/report/server/ReportService.java`:
- Around line 254-272: The child ReportNodeEntity is relying on a
`@Builder.Default` UUID rather than following the explicit UUID assignment pattern
used in duplicateReport/createNewReport; generate a UUID explicitly before
building (e.g., UUID childId = UUID.randomUUID()) and pass it to the
ReportNodeEntity builder via .id(childId) when constructing childReportEntity so
the new node's id is assigned consistently with other creation paths
(references: ReportNodeEntity, sizedChildReportNode, childReportEntity,
saveReportNodeRecursively, self.saveBatchedReports).
In `@src/test/java/org/gridsuite/report/server/ReportControllerTest.java`:
- Around line 172-201: Add a new controller test method (e.g.,
testCreateChildReportEndpointReturnsConflictForNonRootTarget) that inserts the
root via insertReport(REPORT_UUID, toString(REPORT_ONE)), retrieves the root
with mvc.perform(get(URL_TEMPLATE + "/reports/" + REPORT_UUID)) and objectMapper
to parse a Report, grab a non-root id from
rootReport.getSubReports().get(0).getId(), then POST to
mvc.perform(post(URL_TEMPLATE + "/reports/" + nonRootId +
"/children").content(toString(REPORT_TWO)).contentType(APPLICATION_JSON)) and
assert status().isConflict(); use existing helpers/fields insertReport, mvc,
objectMapper, URL_TEMPLATE, REPORT_ONE and REPORT_TWO to locate and implement
the test.
In `@src/test/java/org/gridsuite/report/server/ReportServiceTest.java`:
- Around line 181-233: These two new tests
(createChildReportGeneratesIdAndAppendsAsChild and
createChildReportFailsWhenTargetIsNotARootReport) are missing the SQL statement
count checks used elsewhere; reset the SQLStatementCountValidator at test start
and call the existing assertRequestsCount with the expected select/insert/update
counts after exercising reportService.createChildReport to verify no extra
queries or writes were introduced (mirror the pattern used in other tests in
this class).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbdf16e6-b1ae-451b-8310-04cc9371b3e7
📒 Files selected for processing (4)
src/main/java/org/gridsuite/report/server/ReportController.javasrc/main/java/org/gridsuite/report/server/ReportService.javasrc/test/java/org/gridsuite/report/server/ReportControllerTest.javasrc/test/java/org/gridsuite/report/server/ReportServiceTest.java
KoloMenek
requested changes
May 4, 2026
Signed-off-by: achour94 <berrahmaachour@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
2 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