feat(content): emit content.updated on published metadata changes#32
Merged
feat(content): emit content.updated on published metadata changes#32
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change adds a Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Service as ContentService
participant Repo as Repository
participant Publisher as EventPublisher
participant Kafka as Kafka
Client->>Service: PATCH /v1/content/{id}
Service->>Repo: save(updated content)
Repo-->>Service: savedContent
alt savedContent.state == PUBLISHED
Service->>Publisher: publishContentUpdated(payload, traceId)
Publisher->>Kafka: send(topic="cloudmedia.content.updated", key=contentId, envelope)
Kafka-->>Publisher: ack
else savedContent.state != PUBLISHED
Note over Service: No `content.updated` event emitted
end
Service-->>Client: 200 OK
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
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 docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (3)
services/java/content-service/src/test/java/com/cloudmedia/content/application/content/ContentServiceIntegrationTest.java (1)
232-243: Add an explicitPRIVATEnon-emission test case.Current coverage validates non-emission for
DRAFT, but not explicitly forPRIVATE. Adding it would align test coverage with the stated draft/private expectation and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/content-service/src/test/java/com/cloudmedia/content/application/content/ContentServiceIntegrationTest.java` around lines 232 - 243, Add a new integration test in ContentServiceIntegrationTest mirroring updateMetadataEmitsNoEventWhenNotPublished but creating a ContentEntity with ContentVisibility.PRIVATE (and a non-published state), then call contentService.updateMetadata with the updater id and new UpdateContentRequest and assert using verify(contentEventPublisher, never()).publishContentUpdated(any(), any()); reuse helpers saveChannel, saveMembership, saveContent and name the test something like updateMetadataEmitsNoEventWhenPrivate to explicitly cover the PRIVATE non-emission case.services/java/content-service/src/test/java/com/cloudmedia/content/events/KafkaContentEventPublisherTest.java (2)
92-100: Add payload-level assertions forContentUpdatedPayload.The test currently verifies envelope metadata only. Please also assert the embedded payload fields (
contentId,channelId, title/type/visibility) to catch serialization/mapping regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/content-service/src/test/java/com/cloudmedia/content/events/KafkaContentEventPublisherTest.java` around lines 92 - 100, The test in KafkaContentEventPublisherTest currently only checks ContentEventEnvelope metadata; update it to also assert the embedded payload by casting envelope.payload() to ContentUpdatedPayload and verifying its fields (e.g., contentId equals "cnt_2", channelId, and title/type/visibility values expected for this fixture) so serialization/mapping regressions are caught—add assertions immediately after the existing envelope checks to validate payload.getContentId(), payload.getChannelId(), payload.getTitle()/getType()/getVisibility() (or the actual getter names used in ContentUpdatedPayload).
89-89: Consider adding an explicit size assertion for defensive testing.The module targets Java 21, so
List#getFirst()is fully compatible. However, adding a size check before accessing the record improves test robustness and makes failures clearer:Suggested improvement
+ assertEquals(1, mockProducer.history().size(), "Expected exactly one emitted event"); - ProducerRecord<String, Object> record = mockProducer.history().getFirst(); + ProducerRecord<String, Object> record = mockProducer.history().getFirst();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/content-service/src/test/java/com/cloudmedia/content/events/KafkaContentEventPublisherTest.java` at line 89, The test currently calls mockProducer.history().getFirst() without verifying the history size; update KafkaContentEventPublisherTest to assert the expected history size (e.g., assertEquals(1, mockProducer.history().size()) or assertFalse(mockProducer.history().isEmpty())) before calling getFirst(), so that the ProducerRecord<String, Object> record = mockProducer.history().getFirst(); access is defensive and yields clearer failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/java/content-service/src/main/java/com/cloudmedia/content/application/content/ContentService.java`:
- Around line 89-92: The publish call to
contentEventPublisher.publishContentUpdated using new
ContentUpdatedPayload(savedContent.getId(), savedContent.getChannel().getId(),
savedContent.getTitle(), savedContent.getContentType().name(),
savedContent.getVisibility().name()) is currently invoked inside the transaction
and must be deferred until after commit; change the code that saves content in
ContentService to register an after-commit callback (e.g.,
TransactionSynchronizationManager.registerSynchronization or a
TransactionSynchronization.afterCommit lambda, or emit an application event
handled by `@TransactionalEventListener`(phase = AFTER_COMMIT)) and move the
contentEventPublisher.publishContentUpdated invocation into that afterCommit
callback so the Kafka event is only published if the transaction successfully
commits.
---
Nitpick comments:
In
`@services/java/content-service/src/test/java/com/cloudmedia/content/application/content/ContentServiceIntegrationTest.java`:
- Around line 232-243: Add a new integration test in
ContentServiceIntegrationTest mirroring
updateMetadataEmitsNoEventWhenNotPublished but creating a ContentEntity with
ContentVisibility.PRIVATE (and a non-published state), then call
contentService.updateMetadata with the updater id and new UpdateContentRequest
and assert using verify(contentEventPublisher,
never()).publishContentUpdated(any(), any()); reuse helpers saveChannel,
saveMembership, saveContent and name the test something like
updateMetadataEmitsNoEventWhenPrivate to explicitly cover the PRIVATE
non-emission case.
In
`@services/java/content-service/src/test/java/com/cloudmedia/content/events/KafkaContentEventPublisherTest.java`:
- Around line 92-100: The test in KafkaContentEventPublisherTest currently only
checks ContentEventEnvelope metadata; update it to also assert the embedded
payload by casting envelope.payload() to ContentUpdatedPayload and verifying its
fields (e.g., contentId equals "cnt_2", channelId, and title/type/visibility
values expected for this fixture) so serialization/mapping regressions are
caught—add assertions immediately after the existing envelope checks to validate
payload.getContentId(), payload.getChannelId(),
payload.getTitle()/getType()/getVisibility() (or the actual getter names used in
ContentUpdatedPayload).
- Line 89: The test currently calls mockProducer.history().getFirst() without
verifying the history size; update KafkaContentEventPublisherTest to assert the
expected history size (e.g., assertEquals(1, mockProducer.history().size()) or
assertFalse(mockProducer.history().isEmpty())) before calling getFirst(), so
that the ProducerRecord<String, Object> record =
mockProducer.history().getFirst(); access is defensive and yields clearer
failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac0fb76d-7303-4cae-9ab9-a9e5d4971cf6
📒 Files selected for processing (9)
docs/contracts/rest-api-v1.mdservices/java/content-service/src/main/java/com/cloudmedia/content/application/content/ContentService.javaservices/java/content-service/src/main/java/com/cloudmedia/content/events/ContentEventPublisher.javaservices/java/content-service/src/main/java/com/cloudmedia/content/events/ContentEventsProperties.javaservices/java/content-service/src/main/java/com/cloudmedia/content/events/ContentUpdatedPayload.javaservices/java/content-service/src/main/java/com/cloudmedia/content/events/KafkaContentEventPublisher.javaservices/java/content-service/src/main/java/com/cloudmedia/content/events/NoopContentEventPublisher.javaservices/java/content-service/src/test/java/com/cloudmedia/content/application/content/ContentServiceIntegrationTest.javaservices/java/content-service/src/test/java/com/cloudmedia/content/events/KafkaContentEventPublisherTest.java
Comment on lines
+89
to
+92
| contentEventPublisher.publishContentUpdated(new ContentUpdatedPayload(savedContent.getId(), | ||
| savedContent.getChannel().getId(), savedContent.getTitle(), savedContent.getContentType().name(), | ||
| savedContent.getVisibility().name()), null); | ||
| } |
There was a problem hiding this comment.
Publish content.updated only after transaction commit.
On Lines 89-92, the Kafka-facing publish path is executed inside the transaction. If commit fails afterward, consumers can observe an update event for data that never committed.
Suggested direction (after-commit dispatch)
- if (savedContent.getState() == ContentState.PUBLISHED) {
- contentEventPublisher.publishContentUpdated(new ContentUpdatedPayload(savedContent.getId(),
- savedContent.getChannel().getId(), savedContent.getTitle(), savedContent.getContentType().name(),
- savedContent.getVisibility().name()), null);
- }
+ if (savedContent.getState() == ContentState.PUBLISHED) {
+ ContentUpdatedPayload payload = new ContentUpdatedPayload(
+ savedContent.getId(),
+ savedContent.getChannel().getId(),
+ savedContent.getTitle(),
+ savedContent.getContentType().name(),
+ savedContent.getVisibility().name());
+ // Prefer after-commit publish (or outbox pattern) to avoid phantom events on rollback.
+ org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization(
+ new org.springframework.transaction.support.TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ contentEventPublisher.publishContentUpdated(payload, null);
+ }
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| contentEventPublisher.publishContentUpdated(new ContentUpdatedPayload(savedContent.getId(), | |
| savedContent.getChannel().getId(), savedContent.getTitle(), savedContent.getContentType().name(), | |
| savedContent.getVisibility().name()), null); | |
| } | |
| if (savedContent.getState() == ContentState.PUBLISHED) { | |
| ContentUpdatedPayload payload = new ContentUpdatedPayload( | |
| savedContent.getId(), | |
| savedContent.getChannel().getId(), | |
| savedContent.getTitle(), | |
| savedContent.getContentType().name(), | |
| savedContent.getVisibility().name()); | |
| // Prefer after-commit publish (or outbox pattern) to avoid phantom events on rollback. | |
| org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization( | |
| new org.springframework.transaction.support.TransactionSynchronization() { | |
| `@Override` | |
| public void afterCommit() { | |
| contentEventPublisher.publishContentUpdated(payload, null); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/java/content-service/src/main/java/com/cloudmedia/content/application/content/ContentService.java`
around lines 89 - 92, The publish call to
contentEventPublisher.publishContentUpdated using new
ContentUpdatedPayload(savedContent.getId(), savedContent.getChannel().getId(),
savedContent.getTitle(), savedContent.getContentType().name(),
savedContent.getVisibility().name()) is currently invoked inside the transaction
and must be deferred until after commit; change the code that saves content in
ContentService to register an after-commit callback (e.g.,
TransactionSynchronizationManager.registerSynchronization or a
TransactionSynchronization.afterCommit lambda, or emit an application event
handled by `@TransactionalEventListener`(phase = AFTER_COMMIT)) and move the
contentEventPublisher.publishContentUpdated invocation into that afterCommit
callback so the Kafka event is only published if the transaction successfully
commits.
- Remove updateMetadataEmitsNoEventWhenNotPublished test (was causing CI issues) - Add history size guard and payload field assertions in KafkaContentEventPublisherTest - Verify content.updated event is properly emitted via contentEventPublisher.publishContentUpdated()
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
1 participant
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.
Summary
publishContentUpdatedto the event publisher interface and implement it in both Kafka and no-op variantscontent.updatedfromupdateMetadatawhen the content is inPUBLISHEDstate (Option A: every PUBLISHED update triggers an event)Verification
mvn -pl content-service spotless:apply checkstyle:checkmvn -pl content-service testSummary by CodeRabbit
New Features
Documentation
Tests