Add device-level association and validation for webhooks in SMS gateway #140
Conversation
coderabbitai
bot
changed the title
WalkthroughThis change introduces device-level association for webhooks in the SMS gateway system. It updates the database schema, models, and repository filters to support an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MobileController
participant WebhooksService
participant DevicesService
participant WebhooksRepo
participant DevicesRepo
participant DB
Client->>MobileController: GET /webhooks (device context)
MobileController->>WebhooksService: Select(userID, WithDeviceID(deviceID, false))
WebhooksService->>WebhooksRepo: Select(userID, WithDeviceID(deviceID, false))
WebhooksRepo->>DB: Query webhooks WHERE user_id AND (device_id=deviceID OR device_id IS NULL)
DB-->>WebhooksRepo: Webhook records
WebhooksRepo-->>WebhooksService: Webhook records
WebhooksService-->>MobileController: Webhook DTOs
MobileController-->>Client: Webhook DTOs
sequenceDiagram
participant Client
participant WebhooksService
participant DevicesService
participant DevicesRepo
participant WebhooksRepo
participant DB
Client->>WebhooksService: Replace(userID, webhook{DeviceID})
alt webhook.DeviceID provided
WebhooksService->>DevicesService: Exists(userID, WithID(DeviceID))
DevicesService->>DevicesRepo: Exists(userID, WithID(DeviceID))
DevicesRepo->>DB: Query device WHERE user_id AND id=DeviceID
DB-->>DevicesRepo: Device exists?
DevicesRepo-->>DevicesService: true/false
DevicesService-->>WebhooksService: true/false
alt not exists
WebhooksService-->>Client: Error (device not found)
end
end
WebhooksService->>WebhooksRepo: Replace webhook (with DeviceID)
WebhooksRepo->>DB: Upsert webhook row
WebhooksService->>WebhooksService: notifyDevices(userID, DeviceID)
WebhooksService->>DevicesService: Select(userID, WithID(DeviceID))
DevicesService->>DevicesRepo: Select(userID, WithID(DeviceID))
DevicesRepo->>DB: Query device(s)
DB-->>DevicesRepo: Device(s)
DevicesRepo-->>DevicesService: Device(s)
DevicesService-->>WebhooksService: Device(s)
WebhooksService->>Devices: Send push notification (if push token)
WebhooksService-->>Client: Success
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ 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: 1
🧹 Nitpick comments (4)
internal/sms-gateway/models/migrations/mysql/20250516022136_webhooks_by_device.sql (1)
1-20: Well-structured migration with appropriate constraints and index.The migration correctly:
- Adds a nullable
device_idcolumn with the appropriate data type- Creates a foreign key constraint with CASCADE delete behavior to maintain referential integrity
- Adds an index on the device_id column to improve query performance
- Provides a proper down migration that removes constraints before removing the column
Consider whether a composite index on
(user_id, device_id)might be beneficial if queries frequently filter on both user and device, but only if the webhooks table already has a user_id column.internal/sms-gateway/modules/webhooks/service.go (3)
101-102: Unmanaged goroutine may leak / hide errorsSpawning a goroutine without a context or error handling means:
• the process can’t be shut down gracefully (goroutine outlives request)
• any panic inside the goroutine will crash the program
• failures are only logged, never propagated to callers/testsIf the notification is not strictly fire-and-forget, pass a
context.Contextand let callers decide whether they care about completion. At minimum expose a channel/WaitGroup so the application can drain before exiting.
120-130: Shadowing the importeddevicespackage hampers readabilityThe local variable
deviceslater declared on line 135 shadows the importeddevicespackage, which can confuse readers and static analysis tools.-var devices, err := s.devicesSvc.Select(userID, filters...) +devList, err := s.devicesSvc.Select(userID, filters...)(You’ll need to update subsequent references.)
146-154: Ensure consistent structured logging across branchesMost log statements use the pre-built
logFields, but the “no push token” / “Failed to send push” paths rebuild fields ad-hoc. This slightly diverges field ordering and risks missing context if new fields are added tologFieldslater.- s.logger.Info("Device has no push token", zap.String("user_id", userID), zap.String("device_id", device.ID)) + s.logger.Info("Device has no push token", + append(logFields, zap.String("device_id", device.ID))..., + )Apply the same pattern for the error log at line 153.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.mod(1 hunks)internal/sms-gateway/handlers/webhooks/mobile.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20250516022136_webhooks_by_device.sql(1 hunks)internal/sms-gateway/modules/devices/repository.go(1 hunks)internal/sms-gateway/modules/devices/service.go(1 hunks)internal/sms-gateway/modules/webhooks/converters.go(1 hunks)internal/sms-gateway/modules/webhooks/models.go(1 hunks)internal/sms-gateway/modules/webhooks/repository_filter.go(2 hunks)internal/sms-gateway/modules/webhooks/service.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/sms-gateway/handlers/webhooks/mobile.go (1)
internal/sms-gateway/modules/webhooks/repository_filter.go (1)
WithDeviceID(41-46)
internal/sms-gateway/modules/devices/repository.go (2)
internal/sms-gateway/modules/devices/repository_filter.go (1)
SelectFilter(5-5)internal/sms-gateway/models/models.go (1)
Device(31-42)
internal/sms-gateway/modules/webhooks/models.go (1)
internal/sms-gateway/models/models.go (2)
User(23-29)Device(31-42)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/sms-gateway/modules/devices/repository.go (1)
33-47: Well-implemented repository method for device existence check.This implementation follows good repository patterns by:
- Using the established filter application mechanism
- Efficiently checking existence without retrieving all device data
- Properly distinguishing between "not found" (false, nil) and actual errors (false, err)
- Including clear documentation that explains the return values
go.mod (1)
9-9:✅ Verification successful
Verify that the specific client-go dependency commit exists and is stable.
The dependency has been updated to a pseudo-version (
v1.5.8-0.20250516025314-5876d8deb355) which references a specific commit rather than a tagged release. Ensure this specific commit exists and contains the necessary features to support the device-specific webhook functionality being added.
🏁 Script executed:
#!/bin/bash # Check if the module version exists and get its info go list -m -versions github.com/android-sms-gateway/client-go go mod why github.com/android-sms-gateway/client-goLength of output: 8907
Pseudo-version dependency verified
The pseudo-versionv1.5.8-0.20250516025314-5876d8deb355was successfully downloaded bygo mod—the referenced commit exists. No blocking issues remain.• Optional: consider tagging an official
v1.5.8release to avoid relying on a commit-based pseudo-version.internal/sms-gateway/modules/webhooks/converters.go (1)
8-13: Appropriate update to include DeviceID in the webhook DTO.The addition of the DeviceID field to the webhook DTO is necessary to properly propagate the device association through to API responses. This change is aligned with the overall goal of adding device-level association for webhooks.
internal/sms-gateway/handlers/webhooks/mobile.go (1)
41-41: Great implementation of device-specific webhook filtering!The change to include the device ID filter in
webhooksSvc.Select()is well implemented. Usingfalsefor the exact match parameter correctly ensures that both device-specific webhooks and device-agnostic webhooks (with null device ID) are returned to the mobile client.internal/sms-gateway/modules/webhooks/models.go (2)
14-14: Well-structured model field addition.The
DeviceIDfield is properly defined as an optional pointer to string, which aligns with the device ID type in theDevicemodel. The GORM index tag will help with query performance, and the JSON tag includesomitemptywhich is appropriate for optional fields.
19-20: Properly configured foreign key relationships.The foreign key relationships are correctly established with cascade delete behavior. This ensures that when devices are deleted, their associated webhooks are properly cleaned up.
internal/sms-gateway/modules/devices/service.go (1)
55-64: Well-documented and secure device existence check.The
Existsmethod is well documented with clear explanations of return values and error conditions. Appending the user ID filter ensures the device existence check is properly scoped to the specified user, which is a good security practice that prevents unauthorized access to device information.internal/sms-gateway/modules/webhooks/repository_filter.go (3)
22-23: Good extension of the filter struct.The additions to the
selectFilterstruct are appropriate for supporting device-based filtering.
38-46: Well-documented filter constructor.The
WithDeviceIDfilter constructor is clearly documented and follows the established pattern of other filter constructors in this file. The booleanexactparameter provides flexibility in how filtering is applied.
53-59: Properly implemented conditional query logic.The conditional query logic correctly implements the documented behavior:
- When
deviceIDExactis true, only records with the exact device ID are matched- When
deviceIDExactis false, records with either the specified device ID or with a null device ID are matchedThis flexible approach allows both device-specific and device-agnostic webhooks to be retrieved when needed.
capcom6
force-pushed
the
webhooks/by-device-management
branch
from
4951241 to
86247ac
Compare
capcom6
force-pushed
the
webhooks/by-device-management
branch
from
86247ac to
a57c523
Compare
Summary by CodeRabbit
New Features
Improvements
Database Changes