feat(content): add published-to-private unpublish endpoint#30
Merged
Conversation
📝 WalkthroughWalkthroughAdded a new REST API endpoint Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Database
Client->>Controller: POST /v1/content/{contentId}/unpublish<br/>{userId}
Controller->>Controller: Validate contentId (non-blank)
Controller->>Service: unpublish(contentId, userId)
Service->>Repository: findById(contentId)
Repository->>Database: Query content
Database-->>Repository: ContentEntity
Repository-->>Service: ContentEntity
Service->>Service: Verify channel membership
alt Member verification fails
Service-->>Controller: ApiException(CHANNEL_ACCESS_DENIED)
else State not PUBLISHED
Service-->>Controller: ApiException(CONTENT_STATE_INVALID)
else Valid transition
Service->>Service: Set state=PRIVATE, update updatedAt
Service->>Repository: save(contentEntity)
Repository->>Database: Persist changes
Database-->>Repository: Updated
Repository-->>Service: ContentEntity
Service-->>Controller: ContentResponse
end
Controller-->>Client: ApiSuccessResponse {data, meta}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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: 2
🧹 Nitpick comments (1)
services/java/content-service/src/main/java/com/cloudmedia/content/api/content/ContentController.java (1)
64-66: Prefer a dedicated request DTO for unpublish.Line 64 currently reuses
PublishContentRequest. It works now, but it couples two different endpoint contracts. A separateUnpublishContentRequestkeeps the API surface independent and safer for future changes.🤖 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/api/content/ContentController.java` around lines 64 - 66, The controller currently reuses PublishContentRequest for the unpublish endpoint in ContentController; create a new DTO UnpublishContentRequest (with the same fields currently needed, e.g., userId) and update the method signature to accept `@Valid` `@RequestBody` UnpublishContentRequest request and pass request.userId() into contentService.unpublish(contentId, request.userId()); ensure the new class is placed with other request DTOs and annotated/validated the same way as PublishContentRequest so the endpoint contract is decoupled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contracts/rest-api-v1.md`:
- Around line 95-98: The API contract for the "Unpublish content" endpoint only
lists the 409 CONTENT_STATE_INVALID error; add a new 403 CHANNEL_ACCESS_DENIED
response entry to the same endpoint documentation (the bullet list that starts
with "Unpublishes content from `PUBLISHED` to `PRIVATE`") describing that the
request is rejected when the calling member lacks channel membership/permission;
ensure the new entry mirrors the style of the existing error bullets and clearly
names the error code `403 CHANNEL_ACCESS_DENIED` and the condition
(membership/permission failure).
In
`@services/java/content-service/src/main/java/com/cloudmedia/content/application/content/ContentService.java`:
- Around line 112-120: The check-then-write in ContentService (the method
performing content.getState() check and contentRepository.save(content)) is
racy; make the unpublish/state transition atomic by either adding optimistic
locking to the Content entity (add a `@Version` field and let save throw
OptimisticLockingFailureException which you translate to an ApiException) or
implement a conditional repository update (e.g. a ContentRepository method like
updateStateIfCurrent(Long id, ContentState newState, ContentState expectedState,
LocalDateTime updatedAt) that executes "UPDATE ... WHERE id=? AND state=?" and
returns the updated row count), then call that from the same service method and
throw the HttpStatus.CONFLICT ApiException when the update count is 0. Ensure
you update updatedAt as part of the atomic DB update and handle/translate any
concurrency exceptions into the same conflict ApiException.
---
Nitpick comments:
In
`@services/java/content-service/src/main/java/com/cloudmedia/content/api/content/ContentController.java`:
- Around line 64-66: The controller currently reuses PublishContentRequest for
the unpublish endpoint in ContentController; create a new DTO
UnpublishContentRequest (with the same fields currently needed, e.g., userId)
and update the method signature to accept `@Valid` `@RequestBody`
UnpublishContentRequest request and pass request.userId() into
contentService.unpublish(contentId, request.userId()); ensure the new class is
placed with other request DTOs and annotated/validated the same way as
PublishContentRequest so the endpoint contract is decoupled.
🪄 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: 5592bb07-5b38-4dbf-b10e-bac253f44475
📒 Files selected for processing (5)
docs/contracts/rest-api-v1.mdservices/java/content-service/src/main/java/com/cloudmedia/content/api/content/ContentController.javaservices/java/content-service/src/main/java/com/cloudmedia/content/application/content/ContentService.javaservices/java/content-service/src/test/java/com/cloudmedia/content/api/content/ContentControllerWebMvcTest.javaservices/java/content-service/src/test/java/com/cloudmedia/content/application/content/ContentServiceIntegrationTest.java
Comment on lines
+95
to
+98
| - Unpublishes content from `PUBLISHED` to `PRIVATE` | ||
| - Request fields (MVP): `userId` | ||
| - Returns `409 CONTENT_STATE_INVALID` when state is not `PUBLISHED` | ||
|
|
There was a problem hiding this comment.
Document the membership failure case (403 CHANNEL_ACCESS_DENIED).
This endpoint is membership-protected in implementation, but the contract currently only documents the 409 case. Add 403 CHANNEL_ACCESS_DENIED so client behavior is fully specified.
📝 Suggested doc patch
### `POST /v1/content/{content_id}/unpublish`
- Unpublishes content from `PUBLISHED` to `PRIVATE`
- Request fields (MVP): `userId`
+- Returns `403 CHANNEL_ACCESS_DENIED` when user is not a channel member
- Returns `409 CONTENT_STATE_INVALID` when state is not `PUBLISHED`📝 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
| - Unpublishes content from `PUBLISHED` to `PRIVATE` | |
| - Request fields (MVP): `userId` | |
| - Returns `409 CONTENT_STATE_INVALID` when state is not `PUBLISHED` | |
| - Unpublishes content from `PUBLISHED` to `PRIVATE` | |
| - Request fields (MVP): `userId` | |
| - Returns `403 CHANNEL_ACCESS_DENIED` when user is not a channel member | |
| - Returns `409 CONTENT_STATE_INVALID` when state is not `PUBLISHED` | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contracts/rest-api-v1.md` around lines 95 - 98, The API contract for the
"Unpublish content" endpoint only lists the 409 CONTENT_STATE_INVALID error; add
a new 403 CHANNEL_ACCESS_DENIED response entry to the same endpoint
documentation (the bullet list that starts with "Unpublishes content from
`PUBLISHED` to `PRIVATE`") describing that the request is rejected when the
calling member lacks channel membership/permission; ensure the new entry mirrors
the style of the existing error bullets and clearly names the error code `403
CHANNEL_ACCESS_DENIED` and the condition (membership/permission failure).
Comment on lines
+112
to
+120
| if (content.getState() != ContentState.PUBLISHED) { | ||
| throw new ApiException(HttpStatus.CONFLICT, "CONTENT_STATE_INVALID", | ||
| "Content can only be unpublished from published state", null); | ||
| } | ||
|
|
||
| content.setState(ContentState.PRIVATE); | ||
| content.setUpdatedAt(LocalDateTime.now()); | ||
|
|
||
| return toResponse(contentRepository.save(content)); |
There was a problem hiding this comment.
Make the state transition atomic to preserve guardrails under concurrency.
Line 112 and Line 117 perform check-then-write in separate steps; concurrent requests can both pass the PUBLISHED check and succeed. Use optimistic locking (@Version) or a conditional update (...WHERE id=? AND state='PUBLISHED') and fail when no row is updated.
🤖 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 112 - 120, The check-then-write in ContentService (the method
performing content.getState() check and contentRepository.save(content)) is
racy; make the unpublish/state transition atomic by either adding optimistic
locking to the Content entity (add a `@Version` field and let save throw
OptimisticLockingFailureException which you translate to an ApiException) or
implement a conditional repository update (e.g. a ContentRepository method like
updateStateIfCurrent(Long id, ContentState newState, ContentState expectedState,
LocalDateTime updatedAt) that executes "UPDATE ... WHERE id=? AND state=?" and
returns the updated row count), then call that from the same service method and
throw the HttpStatus.CONFLICT ApiException when the update count is 0. Ensure
you update updatedAt as part of the atomic DB update and handle/translate any
concurrency exceptions into the same conflict ApiException.
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
POST /v1/content/{contentId}/unpublishand route it through content-servicePUBLISHEDcan be unpublished, and transition target isPRIVATEVerification
mvn -pl content-service spotless:apply checkstyle:checkmvn -pl content-service testSummary by CodeRabbit
Release Notes