Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces the security-analysis-specific execute endpoint with a generic process execute endpoint, adds a required Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
antoinebhs
force-pushed
the
generic-process-endpoint
branch
from
b8558b7 to
02284f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java (1)
44-53:⚠️ Potential issue | 🟠 MajorBreaking API contract without transition path.
Changing both the route and request shape (required
userIdheader) breaks existing callers; CI already shows this via the test wheremonitorService.executeProcess(...)is never reached. Please either update all callers/tests in this PR or add a temporary backward-compatible mapping.💡 Backward-compatible mapping option
- `@PostMapping`("/execute") + `@PostMapping`({"/execute", "/execute/security-analysis"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java` around lines 44 - 53, The API change removed the previous route/shape and added a required HEADER_USER_ID header, breaking callers; restore backward compatibility by adding a secondary endpoint or making the header optional and falling back to the old behavior: update MonitorController.executeProcess (or add an overloaded method) to accept requests without HEADER_USER_ID (either by marking `@RequestHeader`(HEADER_USER_ID, required = false) String userId and deriving the original user identity when null, or by adding another `@PostMapping` with the old route/signature that delegates to monitorService.executeProcess(caseUuid, userId, processConfigUuid, isDebug)); ensure both handlers call monitorService.executeProcess so existing tests/callers still reach monitorService.executeProcess and maintain the new header-based flow for upgraded clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 44-53: The API change removed the previous route/shape and added a
required HEADER_USER_ID header, breaking callers; restore backward compatibility
by adding a secondary endpoint or making the header optional and falling back to
the old behavior: update MonitorController.executeProcess (or add an overloaded
method) to accept requests without HEADER_USER_ID (either by marking
`@RequestHeader`(HEADER_USER_ID, required = false) String userId and deriving the
original user identity when null, or by adding another `@PostMapping` with the old
route/signature that delegates to monitorService.executeProcess(caseUuid,
userId, processConfigUuid, isDebug)); ensure both handlers call
monitorService.executeProcess so existing tests/callers still reach
monitorService.executeProcess and maintain the new header-based flow for
upgraded clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8afe21a3-2042-4820-9038-561e417aba8a
📒 Files selected for processing (2)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
There was a problem hiding this comment.
🧹 Nitpick comments (2)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
578-578: Consider updating verification for consistency with the new delete approach.This test still verifies that
deleteById(executionId)is never called, but the production code now usesdelete(entity). While this test still passes (since neither method is called when the entity isn't found), updating it to verify the current implementation would be more accurate.♻️ Suggested update for consistency
verify(executionRepository).findById(executionId); verifyNoInteractions(reportRestService); verifyNoInteractions(resultService); - verify(executionRepository, never()).deleteById(executionId); + verify(executionRepository, never()).delete(any(ProcessExecutionEntity.class));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java` at line 578, The test currently verifies that executionRepository.deleteById(executionId) is never called, but production now uses executionRepository.delete(entity); update the assertion in MonitorServiceTest to verify that executionRepository.delete(...) is never called instead (e.g., verify(executionRepository, never()).delete(any()) or never().delete(any(Execution.class))) so the test matches the current delete approach and intent; reference executionRepository and the delete(entity) usage in the production code when making the change.monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java (1)
70-106: Test correctly updated for the generic endpoint.The endpoint path
/v1/executeand requireduserIdheader correctly align with the controller's@PostMapping("/execute")andHEADER_USER_IDconstant.Consider using exact matchers in verification for consistency. The verification on line 105 uses
any(String.class)foruserIdandany(UUID.class)forprocessConfigUuid, but both values are known in the test. The second test (executeProcessWithConfigNotFoundShouldReturnError) already uses exact values for verification, creating an inconsistency.Suggested tighter verification
- verify(monitorService).executeProcess(eq(caseUuid), any(String.class), any(UUID.class), eq(expectedDebugValue)); + verify(monitorService).executeProcess(eq(caseUuid), eq("user1"), eq(processConfigUuid), eq(expectedDebugValue));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java` around lines 70 - 106, The test executeProcessShouldReturnExecutionId currently uses loose matchers for userId and processConfigUuid; tighten verification by replacing any(String.class) and any(UUID.class) with exact matches (eq("user1") and eq(processConfigUuid)) in the verify call for monitorService.executeProcess, and likewise update the when(...) stub for monitorService.executeProcess to use eq(caseUuid), eq("user1"), eq(processConfigUuid), eq(expectedDebugValue) so the mocked interaction and the verification both assert the concrete values generated in the test (refer to executeProcessShouldReturnExecutionId, monitorService.executeProcess, and the request built by MockHttpServletRequestBuilder).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java`:
- Around line 70-106: The test executeProcessShouldReturnExecutionId currently
uses loose matchers for userId and processConfigUuid; tighten verification by
replacing any(String.class) and any(UUID.class) with exact matches (eq("user1")
and eq(processConfigUuid)) in the verify call for monitorService.executeProcess,
and likewise update the when(...) stub for monitorService.executeProcess to use
eq(caseUuid), eq("user1"), eq(processConfigUuid), eq(expectedDebugValue) so the
mocked interaction and the verification both assert the concrete values
generated in the test (refer to executeProcessShouldReturnExecutionId,
monitorService.executeProcess, and the request built by
MockHttpServletRequestBuilder).
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`:
- Line 578: The test currently verifies that
executionRepository.deleteById(executionId) is never called, but production now
uses executionRepository.delete(entity); update the assertion in
MonitorServiceTest to verify that executionRepository.delete(...) is never
called instead (e.g., verify(executionRepository, never()).delete(any()) or
never().delete(any(Execution.class))) so the test matches the current delete
approach and intent; reference executionRepository and the delete(entity) usage
in the production code when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14b7541b-2ecc-419b-943e-27c6bac0cbec
📒 Files selected for processing (2)
monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
antoinebhs
force-pushed
the
generic-process-endpoint
branch
from
02284f9 to
b482cb4
Compare
|
2 participants
PR Summary