[localserver] add support for data messages#235
Conversation
WalkthroughThis update refactors message handling to support both text and data SMS types using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MessagesRoutes
participant MessagesService
participant MessagesRepository
participant AppDatabase
Client->>MessagesRoutes: POST /messages (with textMessage/dataMessage/message)
MessagesRoutes->>MessagesRoutes: Validate mutual exclusivity and content
MessagesRoutes->>MessagesService: enqueueMessage(SendRequest with MessageContent)
MessagesService->>MessagesRepository: enqueue(SendRequest)
MessagesRepository->>AppDatabase: Insert MessageWithRecipients (content as JSON)
MessagesRepository-->>MessagesService: Ack
loop Periodically
MessagesService->>MessagesRepository: getPending()
MessagesRepository->>AppDatabase: Query pending messages
AppDatabase-->>MessagesRepository: MessageWithRecipients (content as JSON)
MessagesRepository->>MessagesService: StoredSendRequest
MessagesService->>MessagesService: sendMessage(StoredSendRequest)
MessagesService->>MessagesService: sendSMS(StoredSendRequest)
MessagesService->>AppDatabase: Update state/delivery status
end
sequenceDiagram
participant MessagesRoutes
participant MessageContent
MessagesRoutes->>MessageContent: Construct Text or Data subclass based on input
MessageContent-->>MessagesRoutes: MessageContent instance
MessagesRoutes->>MessagesService: enqueueMessage(SendRequest with MessageContent)
sequenceDiagram
participant UI
participant MessageDetailsViewModel
participant MessagesRepository
UI->>MessageDetailsViewModel: Request message details
MessageDetailsViewModel->>MessagesRepository: get(id)
MessagesRepository->>AppDatabase: Query by id
AppDatabase-->>MessagesRepository: MessageWithRecipients (content as JSON)
MessagesRepository->>MessageDetailsViewModel: StoredSendRequest
MessageDetailsViewModel->>UI: LiveData<StoredSendRequest>
UI->>UI: Display message.content.toString()
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/PostMessageRequest.kt (1)
6-9: Consider validating port range for DataMessage.The
DataMessageclass design is clean, but consider adding validation to ensure the port value is within the valid range (0-65535).data class DataMessage( val data: String, // Base64-encoded payload - val port: Int, // Destination port (0-65535) + val port: Int // Destination port (0-65535) )Add validation in the constructor or validation logic:
init { require(port in 0..65535) { "Port must be in range 0-65535" } }app/src/main/java/me/capcom/smsgateway/data/Migrations.kt (1)
18-86: Ensure migration handles large datasets gracefully.This migration performs a full table copy which could be slow for large message tables. Consider adding progress logging or chunked processing for production environments with large datasets.
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt (1)
153-157: Remove defensive IllegalStateException.The
IllegalStateExceptionon line 155 is unreachable due to the validation logic above. This creates dead code that could confuse future maintainers.Consider removing the else branch or replacing with a more descriptive comment:
- else -> { - // This case should be caught by validation, but just in case - throw IllegalStateException("Unknown message type") - }app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt (1)
18-18: Consider configuring Gson for robustness.The default Gson configuration may not handle all edge cases in message content serialization.
Consider adding null safety and error handling:
- private val gson = GsonBuilder().create() + private val gson = GsonBuilder() + .serializeNulls() + .create()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/data/Migrations.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/PostMessageRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt(12 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/Message.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/SendRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/StoredSendRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/vm/MessageDetailsViewModel.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/ui/MessageDetailsFragment.kt(1 hunks)
🔇 Additional comments (20)
app/src/main/java/me/capcom/smsgateway/modules/messages/data/SendRequest.kt (1)
5-9: Verify impact of removing data class behavior.Converting from
data classtoopen classremoves auto-generatedequals(),hashCode(),toString(),copy(), and component functions. This could break existing code that relies on these behaviors.#!/bin/bash # Description: Search for SendRequest usage patterns that might be affected by the data class removal # Check for equality comparisons rg -A 2 -B 2 "SendRequest.*==" # Check for usage in hash-based collections (Set, Map) rg -A 2 -B 2 "Set.*SendRequest|Map.*SendRequest" # Check for copy() method usage rg -A 2 -B 2 "\.copy\(\)" --type kotlin # Check for destructuring assignments rg -A 2 -B 2 "val \(.*\) = .*SendRequest" # Check for toString() usage in logging rg -A 2 -B 2 "SendRequest.*toString|log.*SendRequest"app/src/main/java/me/capcom/smsgateway/ui/MessageDetailsFragment.kt (1)
43-43: Verify MessageContent.toString() provides user-friendly display.The change correctly adapts to the new MessageContent type. Ensure that
MessageContent.toString()returns appropriate text for UI display across all content types (Text, Data).#!/bin/bash # Description: Check MessageContent.toString() implementation and other UI usage # Find MessageContent class definition and toString implementation ast-grep --pattern $'sealed class MessageContent { $$$ }' # Check for other UI usages of message content rg -A 2 -B 2 "\.content\.toString\(\)|\.content\s" --type kotlinapp/src/main/java/me/capcom/smsgateway/modules/messages/data/Message.kt (2)
3-3: LGTM: Correct import for MessageContent.The import from the domain package is appropriate for the MessageContent abstraction.
8-8: Verify complete migration from text to content property.The change from
StringtoMessageContentis correct for supporting typed message content. Ensure all code referencingmessage.texthas been updated to usemessage.content.#!/bin/bash # Description: Check for any remaining references to the old text property # Search for any remaining usage of .text property on Message objects rg -A 2 -B 2 "\.text\b" --type kotlin # Look for Message construction with text parameter rg -A 2 -B 2 "Message\(" --type kotlinapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (2)
9-9: LGTM: Correct import for MessageContent.The import from the domain package is appropriate.
208-208: LGTM: Proper wrapping of text content.Correctly wraps the plain text message in
MessageContent.Text()to align with the new typed content system. This maintains compatibility with the existing API while supporting the new domain model.app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt (2)
27-27: LGTM: Database version increment for schema changes.Correctly incremented version to 14 for the MessageContent migration.
56-59: ```shell
#!/bin/bashDescription: Extract the full MIGRATION_13_14 implementation to verify data copy, drop, and rename steps
rg -n -A 100 -B 0 "val MIGRATION_13_14" --type kotlin app/src/main/java/me/capcom/smsgateway/data/Migrations.kt
</details> <details> <summary>app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt (1)</summary> `3-15`: **Well-designed domain model with proper abstractions.** The sealed class approach is excellent for representing different message content types. The implementation follows Kotlin best practices with appropriate toString() overrides for each subclass. </details> <details> <summary>app/src/main/java/me/capcom/smsgateway/modules/messages/vm/MessageDetailsViewModel.kt (2)</summary> `9-9`: **Import updated for new domain model.** The import change aligns with the broader refactoring to use `StoredSendRequest` instead of `MessageWithRecipients`. --- `15-16`: **ViewModel updated to use new domain model.** The type change from `MessageWithRecipients` to `StoredSendRequest` is consistent with the architectural changes described in the AI summary. </details> <details> <summary>app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt (2)</summary> `10-13`: **Clean enum design for message types.** The `MessageType` enum with `Text` and `Data` values provides clear type safety for distinguishing message content types. --- `39-42`: ```shell #!/bin/bash # Display the full MIGRATION_13_14 definition to verify CREATE, INSERT, DROP, and RENAME logic sed -n '1,200p' app/src/main/java/me/capcom/smsgateway/data/Migrations.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/data/StoredSendRequest.kt (1)
7-15: Well-structured domain class with clear inheritance.The
StoredSendRequestclass properly extendsSendRequestand adds persistence-related fields. The constructor delegation and field organization are clean and follow good Kotlin practices.app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/PostMessageRequest.kt (3)
11-13: Clean text message representation.The
TextMessageclass provides a clear abstraction for text content.
17-18: Good backward compatibility approach.Deprecating the old
messagefield while maintaining it as nullable provides smooth migration path for existing clients.
25-26: Verify mutual exclusivity validation for message types.The nullable
textMessageanddataMessagefields allow flexibility, but ensure validation exists to prevent both being set simultaneously or both being null.Check if validation logic exists for mutual exclusivity of message types:
#!/bin/bash # Search for validation logic that ensures only one message type is provided rg -A 5 -B 5 "textMessage.*dataMessage|message.*null|validation.*message" --type ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt (1)
87-93: LGTM! Port validation is correct.The port range validation (0-65535) correctly covers the full range of valid port numbers for SMS data messages.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt (2)
328-371: LGTM! Well-structured content type handling.The when statement properly handles both text and data message types with appropriate SMS API calls. The encryption/decryption logic is consistently applied to both content types.
290-291: ```shell
#!/usr/bin/env bash
set -eecho "Extracting MessagesService.kt context..."
sed -n '250,330p' app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktecho -e "\nSearching for repository usage in MessagesService.kt..."
rg -n "repository" -C3 app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/api/swagger.json (3)
1079-1081: Mark the deprecatedmessagefield explicitly
OpenAPI supports adeprecated: trueflag. Add"deprecated": trueto themessageproperty to signal tooling and clients that it should no longer be used.@@ -1079,3 +1079,4 @@ "type": "string", + "deprecated": true, "description": "The message text. This field is deprecated, use `textMessage` instead.", "maxLength": 65535 },
1144-1156: Consider disallowing extra fields inTextMessage
To tighten the schema, add"additionalProperties": falseto prevent unvalidated extra properties.@@ -1144,6 +1144,7 @@ "title": "TextMessage", "properties": { "text": { + "description": "Message text", "type": "string" } }, + "additionalProperties": false, "required": [ "text" ]
1157-1176: EnhanceDataMessageschema with strict validation
Add"additionalProperties": falseto disallow unspecified fields, and use OpenAPI'sformat: "byte"for the Base64 payload.@@ -1161,6 +1161,7 @@ "type": "string", + "format": "byte", "description": "Base64-encoded payload" }, "port": { @@ -1174,6 +1175,7 @@ ], + "additionalProperties": false, "required": [ "data", "port"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/swagger.json(2 hunks)
🔇 Additional comments (2)
docs/api/swagger.json (2)
1131-1134:textMessageproperty addition looks good
The newtextMessagefield with a clear reference and description is properly defined and aligns with the domain model.
1135-1138:dataMessageproperty addition looks good
The newdataMessagefield is correctly structured and its description clarifies mutual exclusivity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt (1)
26-27: Remove commented-out code.This commented line appears to be leftover from refactoring and should be removed to maintain code cleanliness.
- val firstMessage = messages.first() -// val text = messages.joinToString(separator = "") { it.displayMessageBody } - + val firstMessage = messages.first() +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/domain/WebHookEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/domain/WebHookEvent.kt
🔇 Additional comments (3)
app/src/main/AndroidManifest.xml (1)
43-48: Verify the choice of port 53739 for data SMS.The port number appears arbitrary. Please ensure this port is documented in your application documentation and doesn't conflict with other apps that might use data SMS.
Is port 53739 a standard or commonly used port for data SMS in Android applications?app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt (1)
40-46: LGTM!The new
SmsDataReceivedclass follows the established pattern and properly distinguishes data SMS payloads from text SMS payloads.app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
51-77: LGTM!The refactoring to handle both text and data messages is well-implemented. The use of Base64 encoding with
NO_WRAPflag is appropriate for transmitting binary data.
app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
feature/data-messages
branch
from
70967d8 to
56dbe2d
Compare
🤖 Pull request artifacts
|
capcom6
force-pushed
the
feature/data-messages
branch
from
3a1f5fc to
7395acd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt (1)
361-368: Base64 error handling properly implemented.The Base64 decoding error handling has been correctly implemented as suggested in the previous review, with proper exception catching and meaningful error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
README.md(3 hunks)app/schemas/me.capcom.smsgateway.data.AppDatabase/14.json(1 hunks)app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/data/Migrations.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/PostMessageRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt(12 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/Message.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/SendRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/data/StoredSendRequest.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/messages/vm/MessageDetailsViewModel.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/domain/WebHookEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/ui/MessageDetailsFragment.kt(1 hunks)app/src/test/java/me/capcom/smsgateway/data/entities/MessageWithRecipientsTest.kt(3 hunks)docs/api/swagger.json(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- app/src/main/java/me/capcom/smsgateway/modules/messages/data/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/domain/WebHookEvent.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
- app/src/main/java/me/capcom/smsgateway/ui/MessageDetailsFragment.kt
- app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt
- app/src/test/java/me/capcom/smsgateway/data/entities/MessageWithRecipientsTest.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/me/capcom/smsgateway/modules/messages/data/SendRequest.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/vm/MessageDetailsViewModel.kt
- app/src/main/java/me/capcom/smsgateway/data/Migrations.kt
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt
- app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt
- app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/data/StoredSendRequest.kt
- app/schemas/me.capcom.smsgateway.data.AppDatabase/14.json
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt
- README.md
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/PostMessageRequest.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt
- docs/api/swagger.json
- app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / build-apk
🔇 Additional comments (6)
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt (6)
15-15: Import additions look appropriate for the new functionality.The new imports for Base64, MessageContent, SendParams, StoredSendRequest, and MessagesRepository are all necessary for supporting data messages and the repository-based architecture.
Also applies to: 24-24, 34-34, 36-36, 38-38
91-95: Excellent refactoring to use repository pattern.The delegation to
messages.enqueue(request)and extraction of priority fromrequest.paramsrepresents a clean separation of concerns and simplifies the service logic.
171-171: Consistent repository usage and parameter extraction.The changes maintain the same logic flow while properly using the repository pattern and extracting priority from request parameters.
Also applies to: 174-174, 177-177, 187-187
214-215: Method signature updated correctly for new request structure.The change from
MessageWithRecipientstoStoredSendRequestaligns with the repository pattern, and the TTL check properly usesrequest.params.validUntil.
262-264: SIM selection logic properly adapted to new parameter structure.The changes correctly extract SIM number from
SendParamsand use the request ID for round-robin selection, maintaining the same logic with the new data structure.Also applies to: 274-274, 283-283, 286-286
328-378: Excellent implementation of MessageContent handling.The new message content abstraction is well-implemented with proper separation between Text and Data message types. Key strengths:
- Consistent encryption/decryption handling for both content types
- Proper multipart message handling for text messages
- Correct Base64 decoding with error handling for data messages
- Appropriate use of port parameter for data SMS
The implementation addresses the past review comment about Base64 error handling and provides a solid foundation for supporting data messages.
app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
Improvements
Database Migration