Get multiple computation statuses at once#40
Conversation
Signed-off-by: sBouzols <[email protected]>
sBouzols
changed the title
|
Warning Review limit reached
More reviews will be available in 7 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces a bulk status retrieval API in the computation service hierarchy. ChangesBulk Status Retrieval Refactoring
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/gridsuite/computation/service/AbstractComputationService.java`:
- Around line 108-111: In getStatuses in AbstractComputationService, guard
against a null resultUuids before calling isEmpty(): check if resultUuids ==
null (or use Objects.requireNonNull with a custom message) and throw the same
ComputationException(EMPTY_PARAMS, "Result uuids cannot be empty") so that null
input produces the controlled ComputationException instead of an NPE; update the
null/empty conditional that currently calls resultUuids.isEmpty() to handle both
cases.
🪄 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: 824339f1-07ca-48c3-8854-193d14900d07
📒 Files selected for processing (4)
src/main/java/org/gridsuite/computation/error/ComputationBusinessErrorCode.javasrc/main/java/org/gridsuite/computation/service/AbstractComputationResultService.javasrc/main/java/org/gridsuite/computation/service/AbstractComputationService.javasrc/test/java/org/gridsuite/computation/ComputationTest.java
|
|
||
| public Map<UUID, S> getStatuses(List<UUID> resultUuids) { | ||
| if (resultUuids.isEmpty()) { | ||
| throw new ComputationException(EMPTY_PARAMS, "Result uuids cannot be empty"); |
There was a problem hiding this comment.
Ok I removed the error management here. returning an empty Map then
Co-authored-by: Copilot <[email protected]> Signed-off-by: sBouzols <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/computation/service/AbstractComputationResultService.java (1)
25-29: LGTM!The contract inversion is clean:
findStatusnow provides a default implementation by delegating to the bulkfindStatusesmethod. This allows subclasses to optimize by implementing only the batch operation.Optional: Consider documenting the contract for missing UUIDs.
The implicit contract for
findStatuseswhen a UUID doesn't exist in the database could be clarified:
- Should the map omit the key (current behavior based on the mock)?
- Or include the key with a
nullvalue?Adding a Javadoc comment on
findStatusesspecifying that missing UUIDs should be omitted from the returned map (not included with null values) would help implementers and prevent subtle bugs.🤖 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/computation/service/AbstractComputationResultService.java` around lines 25 - 29, Add a Javadoc to the abstract method findStatuses in AbstractComputationResultService that documents the contract for missing UUIDs: state that implementations should omit keys for UUIDs not found (do not include keys with null values), and mention that findStatus delegates to findStatuses so callers rely on that omission behavior; update the method's JavaDoc to clearly describe input (List<UUID> resultUuids), expected Map<UUID, S> return (only present entries), and any null-safety expectations for S implementations.
🤖 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/computation/ComputationTest.java`:
- Around line 136-139: The findStatuses method can NPE when mockDBStatus.get
returns null because Collectors.toMap rejects null values; change the logic to
guard against missing keys by filtering resultUuids to only those present in
mockDBStatus (use mockDBStatus.containsKey(uuid)) before collecting, or
alternatively map each UUID to a non-null default (e.g., Optional/placeholder)
instead of allowing nulls; update the method that references resultUuids,
mockDBStatus, and findStatuses accordingly so the returned Map contains no null
values.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/computation/service/AbstractComputationResultService.java`:
- Around line 25-29: Add a Javadoc to the abstract method findStatuses in
AbstractComputationResultService that documents the contract for missing UUIDs:
state that implementations should omit keys for UUIDs not found (do not include
keys with null values), and mention that findStatus delegates to findStatuses so
callers rely on that omission behavior; update the method's JavaDoc to clearly
describe input (List<UUID> resultUuids), expected Map<UUID, S> return (only
present entries), and any null-safety expectations for S implementations.
🪄 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: faf64c40-7b4a-41da-8b0d-4fa148a07e4a
📒 Files selected for processing (3)
src/main/java/org/gridsuite/computation/service/AbstractComputationResultService.javasrc/main/java/org/gridsuite/computation/service/AbstractComputationService.javasrc/test/java/org/gridsuite/computation/ComputationTest.java
sBouzols
changed the title
Avoid `NullPointerException` if any UUID in `resultUuids` is not present in `mockDBStatus` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: sBouzols <[email protected]>
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/computation/ComputationTest.java (1)
389-399: ⚡ Quick winConsider adding test coverage for partial results.
The test verifies the happy path where all requested UUIDs exist in the database. However, it doesn't test the behavior when
getStatusesis called with a mix of existing and non-existing UUIDs. Given thatfindStatuses(lines 138-140) filters out missing UUIDs, callinggetStatuseswith non-existent UUIDs will silently omit them from the result. This behavior should be tested and documented.🧪 Proposed additional test case
`@Test` void testGetStatusesWithMixedExistingAndNonExistingUuids() { UUID existingUuid = UUID.randomUUID(); UUID nonExistingUuid = UUID.randomUUID(); computationService.setStatus(List.of(existingUuid), MockComputationStatus.RUNNING); Map<UUID, MockComputationStatus> statuses = computationService.getStatuses(List.of(existingUuid, nonExistingUuid)); assertThat(statuses).hasSize(1); assertThat(statuses).containsEntry(existingUuid, MockComputationStatus.RUNNING); assertThat(statuses).doesNotContainKey(nonExistingUuid); }🤖 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/test/java/org/gridsuite/computation/ComputationTest.java` around lines 389 - 399, Add a new unit test named testGetStatusesWithMixedExistingAndNonExistingUuids that calls computationService.getStatuses with one UUID that was set via computationService.setStatus and one random UUID that was not set, then assert the returned Map contains only the existing UUID entry (use assertThat(...).hasSize(1) and containsEntry(existingUuid, MockComputationStatus.RUNNING)) and doesNotContainKey(nonExistingUuid); also verify the call to resultService.findStatuses was made with the full list of UUIDs to ensure the filtering behavior in getStatuses/resultService.findStatuses is exercised.
🤖 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.
Nitpick comments:
In `@src/test/java/org/gridsuite/computation/ComputationTest.java`:
- Around line 389-399: Add a new unit test named
testGetStatusesWithMixedExistingAndNonExistingUuids that calls
computationService.getStatuses with one UUID that was set via
computationService.setStatus and one random UUID that was not set, then assert
the returned Map contains only the existing UUID entry (use
assertThat(...).hasSize(1) and containsEntry(existingUuid,
MockComputationStatus.RUNNING)) and doesNotContainKey(nonExistingUuid); also
verify the call to resultService.findStatuses was made with the full list of
UUIDs to ensure the filtering behavior in getStatuses/resultService.findStatuses
is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36447fd6-48bc-45aa-a97b-242dfd580523
📒 Files selected for processing (1)
src/test/java/org/gridsuite/computation/ComputationTest.java
Signed-off-by: sBouzols <[email protected]>
Signed-off-by: sBouzols <[email protected]>
…cific implem Signed-off-by: sBouzols <[email protected]>
|
2 participants
PR Summary
Add
getStatusesinAbstractComputationServiceAdd
findStatusesinAbstractComputationResultService