New notification on study creation#981
Merged
antoinebhs merged 5 commits intomainfrom Apr 29, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe notification message contract for study-creation events is refactored from a single generic update type to two distinct lifecycle types: Changes
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)src/main/java/org/gridsuite/study/server/service/StudyService.javaThanks 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
new-notif-study-creation
branch
from
748b0ff to
7a8706f
Compare
antoinebhs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java (1)
320-327: Optional: add a null guard before reading message payload/headers.In async broker tests, a timeout currently fails with a less-informative NPE. An explicit null assertion improves diagnosis.
Proposed tweak
private void assertStudyCreationMessageReceived(UUID studyUuid, String userId, String expectedUpdateType) { Message<byte[]> message = output.receive(TIMEOUT, studyUpdateDestination); + assertThat(message).as("Expected study.update message with updateType=%s", expectedUpdateType).isNotNull(); assertEquals("", new String(message.getPayload())); MessageHeaders headers = message.getHeaders(); assertEquals(userId, headers.get(HEADER_USER_ID)); assertEquals(studyUuid, headers.get(NotificationService.HEADER_STUDY_UUID)); assertEquals(expectedUpdateType, headers.get(HEADER_UPDATE_TYPE)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java` around lines 320 - 327, The test helper assertStudyCreationMessageReceived should guard against a timed-out receive returning null: after calling output.receive(TIMEOUT, studyUpdateDestination) assert the returned Message is not null (and fail with a clear message), then assert the payload is not null before converting to String and assert that headers (MessageHeaders) are not null before reading HEADER_USER_ID, NotificationService.HEADER_STUDY_UUID and HEADER_UPDATE_TYPE; update the method (referencing output, TIMEOUT, studyUpdateDestination, Message, MessageHeaders, HEADER_USER_ID, NotificationService.HEADER_STUDY_UUID, HEADER_UPDATE_TYPE) to perform these null checks and use assertNotNull with a helpful message so an NPE is avoided and test failures are descriptive.
🤖 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/study/server/service/StudyService.java`:
- Around line 799-803: Wrap the call to
notificationService.emitStudyCreationStarted(...) inside a try-catch in
insertStudyCreationRequest so a publishing failure cannot leave a persisted
orphan; after creating newStudy via insertStudyCreationRequestEntity(...),
attempt the emit in try and on any RuntimeException perform a compensated
cleanup (either call an existing deleteStudyCreationRequest(newStudy.getId()) or
studyCreationRequestRepository.deleteById(newStudy.getId()); if deletion API
doesn't exist, update the persisted entity to a FAILED/ROLLED_BACK status via
updateStudyCreationRequestStatus(newStudy.getId(), FAILED)) and log the error;
rethrow or return a clear failure as appropriate for caller behavior.
In `@src/test/java/org/gridsuite/study/server/studycontroller/StudyTest.java`:
- Around line 484-485: The test currently asserts only the studyCreationStarted
update; add a second assertion to consume the next message and verify the
terminal update equals the finished event used by the other failure-path tests:
fetch the next message (same way the test retrieved the first), extract headers,
and assertEquals(NotificationService.UPDATE_TYPE_STUDY_CREATION_FINISHED,
headers.get(HEADER_UPDATE_TYPE)) to mirror the behavior produced by
emitStudyCreationError(); place this immediately after the existing
started-event assertion in StudyTest so the lifecycle is fully validated like
testCreateStudyCreationFailedWithoutErrorMessage/testCreateStudyCreationFailedWithStudyCreationError.
---
Nitpick comments:
In
`@src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.java`:
- Around line 320-327: The test helper assertStudyCreationMessageReceived should
guard against a timed-out receive returning null: after calling
output.receive(TIMEOUT, studyUpdateDestination) assert the returned Message is
not null (and fail with a clear message), then assert the payload is not null
before converting to String and assert that headers (MessageHeaders) are not
null before reading HEADER_USER_ID, NotificationService.HEADER_STUDY_UUID and
HEADER_UPDATE_TYPE; update the method (referencing output, TIMEOUT,
studyUpdateDestination, Message, MessageHeaders, HEADER_USER_ID,
NotificationService.HEADER_STUDY_UUID, HEADER_UPDATE_TYPE) to perform these null
checks and use assertNotNull with a helpful message so an NPE is avoided and
test failures are descriptive.
🪄 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: 89504180-b613-4dea-b8b4-97595fe622d8
📒 Files selected for processing (6)
src/main/java/org/gridsuite/study/server/notification/NotificationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkControllerTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerCreationTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTestBase.java
Comment on lines
+484
to
485
| assertEquals(NotificationService.UPDATE_TYPE_STUDY_CREATION_STARTED, headers.get(HEADER_UPDATE_TYPE)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect study-creation notification emissions in service code and this test class.
rg -n --type=java -C3 "STUDY_CREATION_STARTED|STUDY_CREATION_FINISHED|emit.*StudyCreation|studyCreation" src/main src/test
rg -n --type=java -C6 "testCreateStudyWithErrorDuringCaseImport|CASE_UUID_CAUSING_IMPORT_ERROR" src/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javaRepository: gridsuite/study-server
Length of output: 41955
🏁 Script executed:
sed -n '466,498p' src/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javaRepository: gridsuite/study-server
Length of output: 1936
Add assertion for terminal studyCreationFinished event to match other failure-path tests.
This test receives only the studyCreationStarted message and stops, while the other two failure-path tests (testCreateStudyCreationFailedWithoutErrorMessage and testCreateStudyCreationFailedWithStudyCreationError) both verify a subsequent studyCreationFinished event. The service emits this finished message via emitStudyCreationError() when case import fails, so add a second message assertion to complete the lifecycle validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/gridsuite/study/server/studycontroller/StudyTest.java`
around lines 484 - 485, The test currently asserts only the studyCreationStarted
update; add a second assertion to consume the next message and verify the
terminal update equals the finished event used by the other failure-path tests:
fetch the next message (same way the test retrieved the first), extract headers,
and assertEquals(NotificationService.UPDATE_TYPE_STUDY_CREATION_FINISHED,
headers.get(HEADER_UPDATE_TYPE)) to mirror the behavior produced by
emitStudyCreationError(); place this immediately after the existing
started-event assertion in StudyTest so the lifecycle is fully validated like
testCreateStudyCreationFailedWithoutErrorMessage/testCreateStudyCreationFailedWithStudyCreationError.
|
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
Introduce a distinct notification on study creation to avoid race condition in directory server.
In the current code:
Sometimes we have 3 notifications (if the creation notif from study server arrives after the element is created in directory DB), sometimes 2 if not already created.
Instead of this system that leads to notification race conditions, I only rely on the notification of directory server and study creation finished. The study creation started notif is not consumed by directory server.
Relates to gridsuite/directory-server#244