Add endpoint to update push device from mobile#289
Open
VihangaMunasinghe wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a new POST /devices/{deviceId}/update endpoint, a DTO for a mobile-sent token, an API implementation that extracts the token and returns 204, and a management-service method that delegates to the device handler to perform a mobile-initiated device update. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Mobile Client
participant API as DevicesApiServiceImpl
participant Service as PushDeviceManagementService
participant Handler as deviceHandlerService
Client->>API: POST /devices/{deviceId}/update\nUpdateRequestDTO(token)
activate API
API->>API: extract token
API->>Service: updateDeviceFromMobile(deviceId, token)
activate Service
Service->>Handler: editDeviceMobile(deviceId, token)
activate Handler
Handler-->>Service: success / PushDeviceHandlerException
deactivate Handler
Service-->>API: return / mapped error
deactivate Service
API-->>Client: 204 No Content / error response
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+158
to
+161
| public void updateDeviceFromMobile(String deviceId, String token) { | ||
|
|
||
| try { | ||
| deviceHandlerService.editDeviceMobile(deviceId, token); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public void updateDeviceFromMobile(String deviceId, String token) { | |
| try { | |
| deviceHandlerService.editDeviceMobile(deviceId, token); | |
| public void updateDeviceFromMobile(String deviceId, String token) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Updating device from mobile for deviceId: " + deviceId); | |
| } | |
| try { | |
| deviceHandlerService.editDeviceMobile(deviceId, token); |
Comment on lines
+161
to
+162
| deviceHandlerService.editDeviceMobile(deviceId, token); | ||
| } catch (PushDeviceHandlerException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| deviceHandlerService.editDeviceMobile(deviceId, token); | |
| } catch (PushDeviceHandlerException e) { | |
| deviceHandlerService.editDeviceMobile(deviceId, token); | |
| log.info("Successfully updated device from mobile for deviceId: " + deviceId); | |
| } catch (PushDeviceHandlerException e) { | |
| log.error("Failed to update device from mobile for deviceId: " + deviceId + ". Error: " + e.getMessage()); |
Comment on lines
+77
to
+79
| @Override | ||
| public Response updateDeviceFromMobile(String deviceId, UpdateRequestDTO updateRequestDTO) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| @Override | |
| public Response updateDeviceFromMobile(String deviceId, UpdateRequestDTO updateRequestDTO) { | |
| public Response updateDeviceFromMobile(String deviceId, UpdateRequestDTO updateRequestDTO) { | |
| log.info("Updating device from mobile. Device ID: " + deviceId); | |
| String token = updateRequestDTO.getToken(); |
Comment on lines
+80
to
+81
| String token = updateRequestDTO.getToken(); | ||
| pushDeviceManagementService.updateDeviceFromMobile(deviceId, token); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| String token = updateRequestDTO.getToken(); | |
| pushDeviceManagementService.updateDeviceFromMobile(deviceId, token); | |
| pushDeviceManagementService.updateDeviceFromMobile(deviceId, token); | |
| log.debug("Successfully updated device. Device ID: " + deviceId); | |
| return Response.status(Response.Status.OK).build(); |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.java (1)
82-82: Minor: Consider using the shorterResponse.ok().build()form.For consistency with other methods in this class (e.g., line 60 uses
Response.ok().entity(...)), this could be simplified:♻️ Suggested simplification
- return Response.status(Response.Status.OK).build(); + return Response.ok().build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.java` at line 82, The return statement in DevicesApiServiceImpl that currently uses Response.status(Response.Status.OK).build() should be simplified to use Response.ok().build() for consistency with other methods (e.g., the Response.ok().entity(...) usage); locate the method in DevicesApiServiceImpl containing that return and replace the long form with Response.ok().build().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.java`:
- Line 82: The return statement in DevicesApiServiceImpl that currently uses
Response.status(Response.Status.OK).build() should be simplified to use
Response.ok().build() for consistency with other methods (e.g., the
Response.ok().entity(...) usage); locate the method in DevicesApiServiceImpl
containing that return and replace the long form with Response.ok().build().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e3a6185-ea2f-4b60-acfd-9ec6066e4986
⛔ Files ignored due to path filters (3)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/push/v1/DevicesApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/push/v1/DevicesApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/push/v1/model/UpdateRequestDTO.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.javacomponents/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/resources/push.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/resources/push.yaml (1)
150-182: Consider using 204 No Content for consistency with similar endpoints.The new
/updateendpoint returns200on success without a response body, while the analogous/removeendpoint (line 206) returns204 No Content. Since this endpoint also doesn't return any content on success, using204would be more semantically accurate and consistent.Suggested change
responses: - '200': - description: The device was updated successfully. + '204': + description: The device was updated successfully. '400':Otherwise, the endpoint structure follows the established pattern of the existing
/removeendpoint correctly—using thedevicetag, requiringdeviceIdpath parameter, accepting a JWT token in the request body, and mapping to standard error responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/resources/push.yaml` around lines 150 - 182, Change the successful response code for the /devices/{deviceId}/update operation (operationId: updateDeviceFromMobile) from '200' to '204 No Content' since the endpoint returns no response body; update the responses block under that path to use '204' and align it with the existing /devices/{deviceId}/remove behavior, leaving requestBody (UpdateRequestDTO) and error response references unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/resources/push.yaml`:
- Around line 150-182: Change the successful response code for the
/devices/{deviceId}/update operation (operationId: updateDeviceFromMobile) from
'200' to '204 No Content' since the endpoint returns no response body; update
the responses block under that path to use '204' and align it with the existing
/devices/{deviceId}/remove behavior, leaving requestBody (UpdateRequestDTO) and
error response references unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1898322-4343-4d3d-bafe-38c1f4f8d8e7
⛔ Files ignored due to path filters (2)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/push/v1/DevicesApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/push/v1/model/UpdateRequestDTO.javais excluded by!**/gen/**
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/resources/push.yaml
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.
Purpose
Add a REST API endpoint to allow mobile devices to update their registered push device properties (device name and device token). This complements the existing device registration and removal endpoints by enabling in-place updates from the device side.
Related Issue
Related PRs
Goals
POST /devices/{deviceId}/updateendpoint in the push device REST API.DeviceHandlerService.editDeviceMobile()method from theidentity-notification-pushcomponent.Approach
/devices/{deviceId}/updatepath in the OpenAPI spec (push.yaml) withUpdateRequestDTOschema containing atokenfield (signed JWT).UpdateRequestDTOmodel class in the generated code layer.updateDeviceFromMobile()method toDevicesApiServiceinterface and its implementation inDevicesApiServiceImpl, which extracts the token from the request body and delegates to the core service.updateDeviceFromMobile()method inPushDeviceManagementServicethat callsdeviceHandlerService.editDeviceMobile(deviceId, token)and handles exceptions.DevicesApiJAX-RS resource class with the new endpoint annotation and routing.User stories
As a mobile app user, I want to be able to update my device's push notification token or device name via the REST API so that push notifications continue to work correctly after token rotation or device rename.
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Added a new REST API endpoint
POST /devices/{deviceId}/updateto allow mobile devices to update their registered push device properties (device token, device name) using a signed JWT.Documentation
Will update
Training
N/A
Certification
N/A - New API endpoint addition with no impact on existing certification content.
Marketing
N/A
Automation tests
N/A - This layer is a thin REST API delegation to the core
identity-notification-pushcomponent, which has unit test coverage for the underlyingeditDeviceMobilelogic.N/A
Security checks
Samples
N/A
Migrations (if applicable)
No migrations required.
Learning
Followed the existing pattern used by the
removeDeviceFromMobileendpoint (/devices/{deviceId}/remove) for structuring the new update endpoint, request DTO, and service delegation.Summary by CodeRabbit