[mobile] messages processing order param #162
Conversation
WalkthroughAdds a MobileController and routes for device message endpoints; renames third‑party message query param types; introduces MessagesOrder (lifo/fifo) and FIFO support with maxPendingBatch in SelectPending; changes base handler validation call and some error propagation; wires controller into FX and updates Swagger for the new order query and 400 response. Changes
Sequence Diagram(s)sequenceDiagram
participant Device
participant Fiber
participant MobileController
participant MessagesService
participant MessagesRepo
Device->>Fiber: GET /mobile/v1/message?order={lifo|fifo}
Fiber->>MobileController: list(device, order)
MobileController->>MessagesService: SelectPending(deviceID, order)
MessagesService->>MessagesRepo: SelectPending(deviceID, order)
MessagesRepo-->>MessagesService: []Message
MessagesService-->>MobileController: []MessageOut
MobileController-->>Device: 200 JSON (messages)
sequenceDiagram
participant Device
participant Fiber
participant MobileController
participant MessagesService
Device->>Fiber: PATCH /mobile/v1/message (updates[])
Fiber->>MobileController: patch(device, updates)
loop for each update
MobileController->>MessagesService: UpdateState(messageID, state)
MessagesService-->>MobileController: OK / NotFound / Error
end
MobileController-->>Device: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
🧹 Nitpick comments (11)
pkg/swagger/docs/swagger.json (2)
985-996: Order query param added: consider declaring the default explicitlyThe enum and description look good. To make the default unambiguous to client generators, add an explicit default.
Apply this diff:
"enum": [ "lifo", "fifo" ], - "type": "string", + "type": "string", + "default": "lifo", "description": "Message processing order: lifo (default) or fifo", "name": "order", "in": "query"
1007-1012: Unify 400 response description wordingMost endpoints use "Invalid request" for 400. Here it's "Bad request". Align for consistency across the API docs.
- "400": { - "description": "Bad request", + "400": { + "description": "Invalid request", "schema": { "$ref": "#/definitions/smsgateway.ErrorResponse" } },internal/sms-gateway/modules/messages/repository_filter.go (1)
5-10: Document exported type and constantsThese are exported API surface in the module. Add doc comments to satisfy linters and clarify semantics (including defaults).
-type MessagesOrder string +// MessagesOrder defines supported ordering for message selection. +// Valid values: "lifo" (default), "fifo". +type MessagesOrder string @@ -const ( - MessagesOrderLIFO MessagesOrder = "lifo" - MessagesOrderFIFO MessagesOrder = "fifo" -) +const ( + // MessagesOrderLIFO orders messages newest-first within the same priority (default). + MessagesOrderLIFO MessagesOrder = "lifo" + // MessagesOrderFIFO orders messages oldest-first within the same priority. + MessagesOrderFIFO MessagesOrder = "fifo" +)pkg/swagger/docs/swagger.yaml (2)
1420-1427: Explicitly set default for order paramMirrors JSON change; improves clarity for code generators.
- description: 'Message processing order: lifo (default) or fifo' enum: - lifo - fifo in: query name: order - type: string + type: string + default: lifo
1437-1441: Align 400 description with the rest of the APIUse "Invalid request" for consistency.
- "400": - description: Bad request + "400": + description: Invalid request schema: $ref: '#/definitions/smsgateway.ErrorResponse'internal/sms-gateway/modules/messages/repository.go (2)
73-77: FIFO/LIFO implemented; consider split Order calls and optionally CreatedAt for semantic FIFO.
- Using id ASC as the FIFO tiebreaker is fine in most cases. If backfills/imports can decouple id from creation time, consider ordering by created_at instead for semantic FIFO.
- Also, prefer chaining two Order calls for clarity and to let GORM compose the clause.
Apply either of the following diffs.
Option A: keep id, split Order calls
- if options.OrderBy == MessagesOrderFIFO { - query = query.Order("messages.priority DESC, messages.id ASC") - } else { - query = query.Order("messages.priority DESC, messages.id DESC") - } + if options.OrderBy == MessagesOrderFIFO { + query = query. + Order("messages.priority DESC"). + Order("messages.id ASC") + } else { + query = query. + Order("messages.priority DESC"). + Order("messages.id DESC") + }Option B: use created_at as secondary key
- if options.OrderBy == MessagesOrderFIFO { - query = query.Order("messages.priority DESC, messages.id ASC") - } else { - query = query.Order("messages.priority DESC, messages.id DESC") - } + if options.OrderBy == MessagesOrderFIFO { + query = query. + Order("messages.priority DESC"). + Order("messages.created_at ASC") + } else { + query = query. + Order("messages.priority DESC"). + Order("messages.created_at DESC") + }
98-106: Nit: extract the pending batch limit into a named constant.
Avoid magic numbers and make the limit self-documenting.}, MessagesSelectOptions{ WithRecipients: true, - Limit: 100, + Limit: MaxPendingBatch, OrderBy: order, })Outside this range, define:
const MaxPendingBatch = 100internal/sms-gateway/handlers/messages/mobile.go (3)
55-58: Remove unnecessary cast by returning a typed order from params.
Have OrderOrDefault return messages.MessagesOrder and use the constant for default, avoiding ad-hoc strings and type casts.Apply this diff here:
- msgs, err := h.messagesSvc.SelectPending(device.ID, messages.MessagesOrder(params.OrderOrDefault())) + msgs, err := h.messagesSvc.SelectPending(device.ID, params.OrderOrDefault())And pair it with the change suggested in internal/sms-gateway/handlers/messages/params.go (OrderOrDefault return type and default constant).
84-88: Use BodyParserValidator for consistency and input validation.
Other handlers already use BodyParserValidator; do the same here to keep consistent validation behavior.- if err := c.BodyParser(&req); err != nil { + if err := h.BodyParserValidator(c, &req); err != nil { return fiber.NewError(fiber.StatusBadRequest, err.Error()) }
97-101: Include message ID in error logs for easier diagnostics.
Adds context to logs when an update fails.- if err != nil && !errors.Is(err, messages.ErrMessageNotFound) { - h.Logger.Error("Can't update message status", zap.Error(err)) - } + if err != nil && !errors.Is(err, messages.ErrMessageNotFound) { + h.Logger.Error("Can't update message status", + zap.Error(err), + zap.String("message_id", v.ID), + ) + }internal/sms-gateway/handlers/messages/params.go (1)
77-87: Type the order param and default via constants to avoid stringly-typed code.
Return messages.MessagesOrder from OrderOrDefault and use MessagesOrderLIFO as the default.-type mobileGetQueryParams struct { - Order string `query:"order" validate:"omitempty,oneof=lifo fifo"` -} +type mobileGetQueryParams struct { + Order messages.MessagesOrder `query:"order" validate:"omitempty,oneof=lifo fifo"` +} -func (p *mobileGetQueryParams) OrderOrDefault() string { - if p.Order != "" { - return p.Order - } - return "lifo" -} +func (p *mobileGetQueryParams) OrderOrDefault() messages.MessagesOrder { + if p.Order != "" { + return p.Order + } + return messages.MessagesOrderLIFO +}Note: Pair with the small call-site change in handlers/messages/mobile.go to drop the cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/sms-gateway/handlers/messages/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(2 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)internal/sms-gateway/modules/messages/service.go(1 hunks)pkg/swagger/docs/swagger.json(2 hunks)pkg/swagger/docs/swagger.yaml(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
internal/sms-gateway/modules/messages/repository.go (2)
internal/sms-gateway/modules/messages/repository_filter.go (4)
MessagesOrderFIFO(9-9)MessagesOrder(5-5)MessagesSelectFilter(12-19)MessagesSelectOptions(21-30)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)ProcessingStatePending(16-16)
internal/sms-gateway/modules/messages/service.go (2)
internal/sms-gateway/modules/messages/repository_filter.go (2)
MessagesOrder(5-5)MessagesOrderLIFO(8-8)internal/sms-gateway/modules/messages/domain.go (1)
MessageOut(25-29)
internal/sms-gateway/handlers/module.go (5)
internal/sms-gateway/handlers/messages/mobile.go (1)
NewMobileController(111-119)internal/sms-gateway/handlers/events/mobile.go (1)
NewMobileController(19-27)internal/sms-gateway/handlers/settings/mobile.go (1)
NewMobileController(56-64)internal/sms-gateway/handlers/webhooks/mobile.go (1)
NewMobileController(53-60)internal/sms-gateway/app.go (1)
In(67-78)
internal/sms-gateway/handlers/messages/mobile.go (9)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/models/models.go (1)
Device(25-36)internal/sms-gateway/modules/messages/repository_filter.go (1)
MessagesOrder(5-5)internal/sms-gateway/handlers/converters/messages.go (1)
MessageToMobileDTO(8-43)internal/sms-gateway/modules/messages/domain.go (1)
MessageStateIn(31-40)internal/sms-gateway/modules/messages/models.go (1)
ProcessingState(12-12)internal/sms-gateway/modules/messages/repository.go (1)
ErrMessageNotFound(17-17)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (1)
WithDevice(83-87)internal/sms-gateway/handlers/events/mobile.go (3)
NewMobileController(19-27)Handler(13-17)h(49-51)
internal/sms-gateway/handlers/messages/params.go (1)
internal/sms-gateway/modules/messages/repository_filter.go (2)
MessagesSelectFilter(12-19)MessagesSelectOptions(21-30)
internal/sms-gateway/handlers/mobile.go (6)
internal/sms-gateway/handlers/messages/mobile.go (1)
MobileController(29-33)internal/sms-gateway/handlers/events/mobile.go (1)
MobileController(13-17)internal/sms-gateway/handlers/settings/mobile.go (1)
MobileController(25-30)internal/sms-gateway/handlers/webhooks/mobile.go (1)
MobileController(23-27)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/app.go (1)
In(67-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/sms-gateway/handlers/module.go (1)
29-29: DI wiring for messages.MobileController looks goodProvider registration is correctly grouped with other controllers and scoped private. No issues spotted.
internal/sms-gateway/handlers/messages/3rdparty.go (1)
193-193: Switched to thirdPartyGetQueryParams — looks correctThe new type aligns with the params refactor and downstream ToFilter/ToOptions usage.
internal/sms-gateway/modules/messages/service.go (1)
95-101: LGTM: sensible defaulting and order pass-through.
Defaulting to LIFO when empty and delegating to the repo matches the new contract and keeps behavior predictable.internal/sms-gateway/handlers/messages/mobile.go (1)
49-53: LGTM: query parsing/validation with proper 400 on error.
Using QueryParserValidator ensures the order param is constrained to lifo/fifo and returns a Bad Request on invalid input.internal/sms-gateway/handlers/mobile.go (4)
28-41: LGTM: DI params now accept controllers; aligns with controller-based routing.
Clean separation of concerns and consistent with other handler modules.
49-53: LGTM: handler struct updated to use messages controller.
This removes direct coupling to the messages service at the top-level handler.
259-261: Confirm intent to keep both /message and /messages routes.
You register the same controller under two paths. If this is an intentional alias for backward compatibility, consider adding a deprecation notice and documenting it. If not required, drop one to avoid surface duplication.Would you like me to prepare a follow-up patch to:
- add a deprecation comment and TODO for the alias, or
- remove the duplicate registration?
270-281: LGTM: constructor wiring updated correctly.
Base handler, controllers, and dependencies are cleanly injected; idGen kept intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/sms-gateway/modules/messages/repository.go (3)
16-16: Hard-coded pending batch cap: document rationale and consider configurabilityGood to cap batches, but 100 may not fit all deployments. At minimum, document the intent. Optionally, make it configurable via env/config.
Apply this diff to document the constant:
-const maxPendingBatch = 100 +// maxPendingBatch caps the number of pending messages returned by SelectPending +// to avoid large scans and excessive payloads to devices. +// Consider making this configurable if product requirements vary by deployment. +const maxPendingBatch = 100
74-78: Minor: split into chained Order calls and ensure supporting DB indexChaining Order calls is a bit clearer and plays nicer with query builders. Also, ensure there’s an index to support ORDER BY priority,id (commonly (device_id, state, priority, id) for the pending path).
Apply this diff for readability:
- if options.OrderBy == MessagesOrderFIFO { - query = query.Order("messages.priority DESC, messages.id ASC") - } else { - query = query.Order("messages.priority DESC, messages.id DESC") - } + if options.OrderBy == MessagesOrderFIFO { + query = query.Order("messages.priority DESC").Order("messages.id ASC") + } else { + query = query.Order("messages.priority DESC").Order("messages.id DESC") + }DB index suggestion (adjust names/columns to your schema):
-- For pending/device fetch: WHERE device_id = ? AND state = 'Pending' ORDER BY priority, id CREATE INDEX IF NOT EXISTS idx_messages_device_state_priority_id ON messages (device_id, state, priority, id);
99-99: Add a doc comment clarifying semantics of SelectPendingMake it explicit that ordering is within priority, and that the method returns up to maxPendingBatch.
+// SelectPending returns up to maxPendingBatch pending messages for a device, +// ordered by priority DESC, then by ID in FIFO/LIFO order as specified. func (r *repository) SelectPending(deviceID string, order MessagesOrder) ([]Message, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/modules/messages/repository.go(3 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)pkg/swagger/docs/swagger.json(2 hunks)pkg/swagger/docs/swagger.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/swagger/docs/swagger.json
- internal/sms-gateway/handlers/messages/mobile.go
- pkg/swagger/docs/swagger.yaml
- internal/sms-gateway/modules/messages/repository_filter.go
- internal/sms-gateway/handlers/messages/params.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/sms-gateway/modules/messages/repository.go (1)
74-78: Confirm priority-first sorting across all message selectsThe new ORDER BY (
messages.priority DESC, messages.id ASC|DESC) is now applied to everySelect(…)call, including those using defaultMessagesSelectOptions. Please audit the following consumers to ensure they still behave as expected, or explicitly setOrderByif they should retain pure LIFO ordering:• internal/sms-gateway/modules/messages/service.go
– UpdateState:s.messages.Get(..., MessagesSelectOptions{})(line 109)
– GetState:s.messages.Get(..., MessagesSelectOptions{WithRecipients:true,WithDevice:true,WithStates:true})(lines 151–153)
– List/Paginated fetch:s.messages.Select(filter, options)(line 142)• internal/sms-gateway/handlers/messages/params.go
–ToOptions()buildsMessagesSelectOptions{WithRecipients:true,WithStates:true}(starting at line 58)Updated audit script to locate all zero-OrderBy usages:
#!/usr/bin/env bash set -euo pipefail # List every MessagesSelectOptions literal so you can check for missing OrderBy rg -n -A2 'MessagesSelectOptions\s*{' internal/sms-gateway
capcom6
force-pushed
the
mobile/messages-processing-order
branch
from
ec34ad6 to
3906e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/sms-gateway/modules/messages/repository.go (1)
99-107: SelectPending wiring looks correct; add FIFO/LIFO tests and consider bypassing Count() for performanceThe signature change and propagation of OrderBy and the batch cap look good. Two follow-ups:
- Add tests asserting FIFO vs LIFO within equal priority, and that at most maxPendingBatch items are returned.
- Select() always performs Count(), which SelectPending doesn’t use (only the slice capacity benefits). This adds unnecessary load. Consider a fast path that skips Count() for pending fetches (e.g., an option like SkipCount or a dedicated query in SelectPending).
Run this to check for tests covering the new order behavior and batch cap:
#!/bin/bash set -euo pipefail # Order semantics tests present? rg -n "(MessagesOrder|OrderFIFO|OrderLIFO|fifo|lifo)" -g "*_test.go" || true rg -n "SelectPending\\(" -g "*_test.go" || true # Any tests asserting batch size (100) or similar? rg -n "maxPendingBatch|Limit:\\s*100|len\\(.*\\)\\s*<=?\\s*100" -g "*_test.go" || true
🧹 Nitpick comments (2)
internal/sms-gateway/modules/messages/repository.go (2)
16-16: Hard-coded pending batch cap: add doc comment and consider making it configurable100 as a hard cap is reasonable, but hard-coding removes flexibility for tuning in different environments or A/B tests. At minimum, document the intent; ideally, inject via config or constructor.
Apply this diff to document the constant:
-const maxPendingBatch = 100 +// maxPendingBatch caps the number of pending messages returned by SelectPending +// to avoid large payloads and keep memory/network usage bounded. +const maxPendingBatch = 100
74-78: Explicitly handle invalid/zero order values; default to LIFO only when appropriateThe current else branch treats any non-FIFO, including invalid values, as LIFO. That can mask upstream validation gaps. Be explicit about supported values and default the zero value to LIFO as documented.
Apply this diff to make handling explicit:
- if options.OrderBy == MessagesOrderFIFO { - query = query.Order("messages.priority DESC, messages.id ASC") - } else { - query = query.Order("messages.priority DESC, messages.id DESC") - } + switch options.OrderBy { + case MessagesOrderFIFO: + query = query.Order("messages.priority DESC, messages.id ASC") + case "", MessagesOrderLIFO: + // Zero value and explicit LIFO default to DESC on id. + query = query.Order("messages.priority DESC, messages.id DESC") + default: + // Unknown value: fall back to LIFO to avoid surprising reordering. + // Consider surfacing a validation error earlier in the stack (handler/service). + query = query.Order("messages.priority DESC, messages.id DESC") + }Performance note: Given the WHERE and ORDER BY used for pending selection (typically by device_id and state), consider adding or verifying a composite index such as (device_id, state, priority, id) to support this sort order efficiently at scale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/sms-gateway/handlers/messages/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(3 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)internal/sms-gateway/modules/messages/service.go(1 hunks)pkg/swagger/docs/swagger.json(2 hunks)pkg/swagger/docs/swagger.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/sms-gateway/handlers/module.go
- pkg/swagger/docs/swagger.json
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/modules/messages/repository_filter.go
- pkg/swagger/docs/swagger.yaml
- internal/sms-gateway/handlers/messages/3rdparty.go
- internal/sms-gateway/handlers/messages/mobile.go
- internal/sms-gateway/handlers/mobile.go
- internal/sms-gateway/handlers/messages/params.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/sms-gateway/modules/messages/repository.go (2)
internal/sms-gateway/modules/messages/repository_filter.go (4)
MessagesOrderFIFO(13-13)MessagesOrder(7-7)MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)ProcessingStatePending(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
|
Staging... |
|
Deplyed... |
|
Rollback due to messages status update issue |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/sms-gateway/handlers/base/handler.go(1 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/messages/mobile.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/sms-gateway/handlers/base/handler.go (2)
internal/sms-gateway/handlers/mobile.go (5)
Handler(30-42)In(317-330)h(136-152)h(194-215)h(254-267)internal/sms-gateway/handlers/upstream.go (3)
Handler(17-22)h(55-82)In(24-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
internal/sms-gateway/handlers/base/handler_test.go (1)
126-129: Also check app.Test error in body testsFor consistency and to avoid nil dereferences on failure, check
errreturned byapp.Test.Apply this diff:
- resp, _ := app.Test(req) + resp, err := app.Test(req) + if err != nil { + t.Fatalf("app.Test failed: %v", err) + } if test.expectedStatus != resp.StatusCode { t.Errorf("Expected status code %d, got %d", test.expectedStatus, resp.StatusCode) }
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/base/handler_test.go (1)
149-174: Add a test for type coercion failures in query parsingCurrently, you test missing/too-low values. Add a case for non-integer
ageto verify that parsing errors are surfaced as 400.Apply this diff to add the test case:
tests := []struct { description string path string expectedStatus int }{ + { + description: "Invalid query parameters - non-integer age", + path: "/test?name=John&age=abc", + expectedStatus: fiber.StatusBadRequest, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/sms-gateway/handlers/base/handler_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/sms-gateway/handlers/base/handler_test.go (1)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/sms-gateway/handlers/base/handler_test.go (5)
27-31: Query struct: tags and required rules look correctGood use of Fiber's
querytags and validator'srequiredtags to drive parsing and validation.
32-36: Params struct: tags and required rules look correctThe
paramstags align with Fiber's path param parsing; validations are appropriately declared.
44-49: Custom Validatable implementation for query is appropriateCovers the Validatable path (age >= 18) alongside tag validation. Nice separation of concerns.
51-56: Custom Validatable implementation for params is appropriateProvides a deterministic way to simulate business-rule failures (e.g., invalid ID).
308-321: Clarify/design: behavior when validator is nil and only tags are presentWhen
Validatoris nil, tag-based rules (e.g.,validate:"required") are skipped. Only theValidatableinterface is enforced. If that’s intentional, consider adding a test that documents this explicitly (e.g., missing requiredNamewithhandlerWithoutValidatorshould pass). If unintentional, adjustValidateStructto still enforce "required" for basic presence when no validator is set.Do you want me to:
- add a test documenting the current behavior, or
- propose a minimal change in
ValidateStructto enforce basic presence checks even without a validator?
capcom6
force-pushed
the
mobile/messages-processing-order
branch
from
b9571c0 to
c8487cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/sms-gateway/handlers/base/handler.go (1)
44-49: Restore Struct-based validation; Var("required,dive") skips struct field tagsUsing Validator.Var on a struct bypasses its field
validatetags and makes validation ineffective for query/body/params. This will let invalid values (e.g., unknown order) through and will also cause the updated tests to fail.Apply this diff:
func (h *Handler) ValidateStruct(out any) error { if h.Validator != nil { - if err := h.Validator.Var(out, "required,dive"); err != nil { + if err := h.Validator.Struct(out); err != nil { return fiber.NewError(fiber.StatusBadRequest, err.Error()) } }internal/sms-gateway/handlers/base/handler_test.go (2)
219-238: Deflake NotFound tests: these paths can still route-matchPaths like "/test//John" and "/test/123/" may still match the route, returning 400 instead of 404. Use unambiguous non-matching paths.
Apply this diff:
{ description: "Invalid path parameters - missing id", - path: "/test//John", + path: "/test/John", expectedStatus: fiber.StatusNotFound, }, { description: "Invalid path parameters - missing name", - path: "/test/123/", + path: "/test/123", expectedStatus: fiber.StatusNotFound, },
278-289: This test will fail until ValidateStruct uses Struct(...)“Invalid struct with validator - missing required field” expects the validator to catch missing Name, but current ValidateStruct uses Var("required,dive") which won’t apply field tags. Fix in base/handler.go as suggested to make this test pass.
🧹 Nitpick comments (4)
internal/sms-gateway/handlers/messages/mobile.go (3)
51-53: Return parser error directly; it’s already a Fiber errorQueryParserValidator returns a Fiber error with the correct status. Re-wrapping it as a new 400 loses original context and is redundant.
Apply this diff:
- if err := h.QueryParserValidator(c, ¶ms); err != nil { - return fiber.NewError(fiber.StatusBadRequest, err.Error()) - } + if err := h.QueryParserValidator(c, ¶ms); err != nil { + return err + }
85-87: Return parser error directly; it’s already a Fiber errorSame here for body parsing — no need to wrap again.
- if err := h.BodyParserValidator(c, &req); err != nil { - return fiber.NewError(fiber.StatusBadRequest, err.Error()) - } + if err := h.BodyParserValidator(c, &req); err != nil { + return err + }
97-104: Confirm desired semantics: partial failures currently return 204Non-not-found update errors are logged but the endpoint still returns 204. If partial updates must fail the request, consider accumulating errors and returning 207 Multi-Status or 500/400 accordingly. If 204 on partial failure is intentional, consider documenting this behavior in the Swagger description.
internal/sms-gateway/handlers/mobile.go (1)
259-261: Double-mounting /message and /messages — verify intentYou register both singular and plural routes. If this is for backward compatibility, great; if not, it introduces duplicate endpoints. Confirm and, if needed, drop one to reduce surface area.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
internal/sms-gateway/handlers/base/handler.go(1 hunks)internal/sms-gateway/handlers/base/handler_test.go(3 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(2 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/mobile.go(3 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(3 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)internal/sms-gateway/modules/messages/service.go(1 hunks)pkg/swagger/docs/swagger.json(2 hunks)pkg/swagger/docs/swagger.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/sms-gateway/handlers/module.go
- pkg/swagger/docs/swagger.json
- internal/sms-gateway/modules/messages/repository.go
- internal/sms-gateway/handlers/messages/params.go
- internal/sms-gateway/handlers/messages/3rdparty.go
- pkg/swagger/docs/swagger.yaml
- internal/sms-gateway/modules/messages/service.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/sms-gateway/handlers/base/handler.go (1)
internal/sms-gateway/handlers/upstream.go (3)
h(55-82)Handler(17-22)In(24-32)
internal/sms-gateway/handlers/messages/mobile.go (8)
internal/sms-gateway/modules/messages/service.go (1)
Service(48-61)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/models/models.go (1)
Device(25-36)internal/sms-gateway/handlers/converters/messages.go (1)
MessageToMobileDTO(8-43)internal/sms-gateway/modules/messages/domain.go (1)
MessageStateIn(31-40)internal/sms-gateway/modules/messages/models.go (1)
ProcessingState(12-12)internal/sms-gateway/modules/messages/repository.go (1)
ErrMessageNotFound(18-18)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (1)
WithDevice(83-87)
internal/sms-gateway/handlers/base/handler_test.go (1)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)
internal/sms-gateway/handlers/mobile.go (7)
internal/sms-gateway/modules/messages/service.go (1)
Service(48-61)internal/sms-gateway/handlers/messages/mobile.go (1)
MobileController(29-33)internal/sms-gateway/handlers/events/mobile.go (1)
MobileController(13-17)internal/sms-gateway/handlers/settings/mobile.go (1)
MobileController(25-30)internal/sms-gateway/handlers/webhooks/mobile.go (1)
MobileController(23-27)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/app.go (1)
In(67-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/sms-gateway/modules/messages/repository_filter.go (2)
5-14: Clear, typed order enum with documented defaults — LGTMGood introduction of a dedicated MessagesOrder type and explicit LIFO/FIFO constants with concise docs. This makes ordering intent explicit across layers.
30-33: Documented default on OrderBy is helpful — LGTMThe inline comment about defaulting to LIFO when empty clarifies behavior and prevents ambiguity for callers.
internal/sms-gateway/handlers/base/handler_test.go (2)
126-129: Good fix: don’t ignore app.Test errorCapturing and failing on app.Test error prevents nil dereferences and surfaces routing/parsing failures properly. Thanks for tightening this up.
27-35: Validatable test scaffolding — LGTMThe query/params DTOs with Validate methods provide solid coverage of the Validatable path in the handler. Nicely structured.
Also applies to: 44-56
internal/sms-gateway/handlers/messages/mobile.go (2)
55-59: Ordering integration — LGTMPassing params.OrderOrDefault() through to SelectPending aligns the API surface with repository/service changes for FIFO/LIFO retrieval.
109-122: Controller registration and DI wiring — LGTMNeat encapsulation via MobileController with deviceauth middleware applied at the controller boundary.
internal/sms-gateway/handlers/mobile.go (1)
28-41: FX params + handler wiring — LGTMClean migration to controller-based design with explicit DI. Base.Handler gets Logger and Validator as expected.
Also applies to: 269-281
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/sms-gateway/handlers/messages/3rdparty.go(3 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/mobile.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/messages/3rdparty.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/sms-gateway/handlers/messages/mobile.go (8)
internal/sms-gateway/modules/messages/service.go (1)
Service(48-61)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/models/models.go (1)
Device(25-36)internal/sms-gateway/handlers/converters/messages.go (1)
MessageToMobileDTO(8-43)internal/sms-gateway/modules/messages/domain.go (1)
MessageStateIn(31-40)internal/sms-gateway/modules/messages/repository.go (1)
ErrMessageNotFound(18-18)internal/sms-gateway/handlers/middlewares/deviceauth/deviceauth.go (1)
WithDevice(83-87)internal/sms-gateway/handlers/events/mobile.go (2)
NewMobileController(19-27)Handler(13-17)
internal/sms-gateway/handlers/mobile.go (11)
internal/sms-gateway/modules/messages/service.go (2)
Service(48-61)In(33-46)internal/sms-gateway/modules/sse/service.go (1)
Service(17-25)internal/sms-gateway/modules/settings/service.go (1)
Service(19-25)internal/sms-gateway/modules/webhooks/service.go (1)
Service(28-37)internal/sms-gateway/modules/devices/service.go (1)
Service(29-38)internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)internal/sms-gateway/handlers/messages/mobile.go (1)
MobileController(29-33)internal/sms-gateway/handlers/events/mobile.go (1)
MobileController(13-17)internal/sms-gateway/handlers/settings/mobile.go (1)
MobileController(25-30)internal/sms-gateway/handlers/webhooks/mobile.go (1)
MobileController(23-27)internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/sms-gateway/handlers/mobile.go (5)
28-41: LGTM! Clean separation of concerns.The refactoring to introduce
mobileHandlerParamswith dependency injection is well-structured. Moving message handling to a dedicated controller follows the single responsibility principle and improves modularity.
98-99: Good improvement in error handling consistency.Returning the raw error from
BodyParserValidatoris the correct approach, as the base handler already wraps validation errors appropriately. This change aligns with the error handling pattern used elsewhere in the codebase.
152-153: Consistent error handling improvement.Good consistency with the other error handling changes in this file.
207-208: Consistent error handling improvement.Good consistency with the other error handling changes in this file.
269-280: LGTM! Clean dependency injection setup.The constructor properly initializes all dependencies using the injected parameters. The structure maintains good separation of concerns.
internal/sms-gateway/handlers/messages/mobile.go (5)
48-68: LGTM! Clean implementation of message retrieval with ordering support.The implementation properly validates query parameters and delegates to the service layer with the appropriate order parameter. Good use of functional mapping for DTO conversion.
98-99: Good error handling for missing messages.The code correctly tolerates
ErrMessageNotFounderrors, which is appropriate when processing batch updates where some messages might have been already processed or deleted. This prevents unnecessary failures for valid race conditions.
109-112: LGTM! Clean route registration.The route registration is simple and follows RESTful conventions with GET for retrieval and PATCH for updates.
114-122: LGTM! Well-structured constructor.The constructor properly initializes the base handler with a named logger for better log tracing and includes all necessary dependencies.
83-107: No change needed — UpdateState already enforces device ownershipUpdateState in internal/sms-gateway/modules/messages/service.go filters by DeviceID before modifying the message, so a device cannot update messages belonging to another device.
- internal/sms-gateway/modules/messages/service.go:108-110 — existing, err := s.messages.Get(MessagesSelectFilter{ExtID: message.ID, DeviceID: deviceID}, ...)
- internal/sms-gateway/modules/messages/repository.go:141-146 — repository UpdateState persists the change
capcom6
force-pushed
the
mobile/messages-processing-order
branch
from
3bd5cc7 to
7ad51af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/sms-gateway/handlers/base/handler.go (1)
46-49: Critical: Var("required,dive") bypasses struct field tags — use Struct(...) for tag-based validationvalidator.Var(out, "required,dive") does not traverse struct fields nor apply their
validate:"..."tags. This effectively disables field-level validation for request DTOs (e.g.,oneof=lifo fifo,required,min, etc.). Replace with Struct(...) to enforce tags.Apply this diff:
- if err := h.Validator.Var(out, "required,dive"); err != nil { + if err := h.Validator.Struct(out); err != nil { return fiber.NewError(fiber.StatusBadRequest, err.Error()) }internal/sms-gateway/modules/messages/repository.go (1)
99-107: Signature change: ensure all call sites updated and add tests for FIFO vs LIFO behaviorNow that SelectPending requires an
orderargument and appliesmaxPendingBatch, please:
- Verify no call sites still use the old signature.
- Add unit tests asserting FIFO vs LIFO ordering within equal priority buckets.
You can run this to locate invocations and spot-test for missing args:
#!/usr/bin/env bash set -euo pipefail # Find all non-definition invocations of SelectPending( rg -n 'SelectPending\(' | grep -vP '^\s*func\s+\([\w*]+\)\s+SelectPending\(' || trueIf you want, I can draft repo/service tests that insert messages with mixed priorities and assert returned order for both FIFO and LIFO.
internal/sms-gateway/handlers/base/handler_test.go (1)
219-233: Make NotFound test cases unambiguous; double slashes may still match routesPaths like "/test//John" and "/test/123/" can still route-match in Fiber, causing 400 instead of 404. Use paths that won’t match the pattern to make these tests deterministic.
Apply this diff:
{ description: "Invalid path parameters - missing id", - path: "/test//John", + path: "/test/John", expectedStatus: fiber.StatusNotFound, }, { description: "Invalid path parameters - missing name", - path: "/test/123/", + path: "/test/123", expectedStatus: fiber.StatusNotFound, },
🧹 Nitpick comments (1)
internal/sms-gateway/modules/messages/repository.go (1)
16-16: maxPendingBatch hard-coded: consider configurabilityA fixed batch size of 100 is reasonable, but making it configurable (e.g., via app config/env with sane defaults) would help adapt to varying device throughput or DB capacity without code changes.
Happy to propose a small config plumbing change if you want this wired through fx params.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
internal/sms-gateway/handlers/base/handler.go(1 hunks)internal/sms-gateway/handlers/base/handler_test.go(3 hunks)internal/sms-gateway/handlers/messages/3rdparty.go(3 hunks)internal/sms-gateway/handlers/messages/mobile.go(1 hunks)internal/sms-gateway/handlers/messages/params.go(4 hunks)internal/sms-gateway/handlers/mobile.go(6 hunks)internal/sms-gateway/handlers/module.go(1 hunks)internal/sms-gateway/modules/messages/repository.go(3 hunks)internal/sms-gateway/modules/messages/repository_filter.go(2 hunks)internal/sms-gateway/modules/messages/service.go(1 hunks)pkg/swagger/docs/swagger.json(2 hunks)pkg/swagger/docs/swagger.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/handlers/module.go
- internal/sms-gateway/modules/messages/service.go
- pkg/swagger/docs/swagger.json
- internal/sms-gateway/modules/messages/repository_filter.go
- internal/sms-gateway/handlers/messages/mobile.go
- pkg/swagger/docs/swagger.yaml
- internal/sms-gateway/handlers/mobile.go
- internal/sms-gateway/handlers/messages/params.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/sms-gateway/handlers/base/handler.go (1)
internal/sms-gateway/handlers/upstream.go (3)
h(55-82)Handler(17-22)In(24-32)
internal/sms-gateway/modules/messages/repository.go (2)
internal/sms-gateway/modules/messages/repository_filter.go (4)
MessagesOrderFIFO(13-13)MessagesOrder(7-7)MessagesSelectFilter(16-23)MessagesSelectOptions(25-36)internal/sms-gateway/modules/messages/models.go (2)
Message(35-55)ProcessingStatePending(16-16)
internal/sms-gateway/handlers/messages/3rdparty.go (1)
internal/sms-gateway/modules/messages/models.go (1)
Message(35-55)
internal/sms-gateway/handlers/base/handler_test.go (3)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/app.go (1)
Run(54-65)test/e2e/messages_test.go (1)
name(33-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/sms-gateway/handlers/messages/3rdparty.go (1)
63-66: Propagating parser/validator errors directly is fine — verify they remain Fiber errors (400) at call sitesReturning the underlying error preserves status codes provided by BodyParserValidator/QueryParserValidator. Confirm these paths always return fiber.NewError(400) for parse/validation failures, so clients consistently receive 400s.
If you want a quick check, ensure ValidateStruct uses Struct-based validation returning fiber.NewError(400) (see base/handler.go fix). Also verify no non-Fiber errors slip through in these early-return paths.
Also applies to: 69-71, 193-196, 255-256
internal/sms-gateway/modules/messages/repository.go (1)
74-78: FIFO/LIFO ordering logic looks correct — confirm ID is an acceptable proxy for creation timeOrdering by
priority DESCthenid ASC|DESCachieves FIFO/LIFO among equal priorities. Verify thatidmonotonically increases with insert order in your environment; if inserts can be non-monotonic, considercreated_atas the tie-breaker instead.internal/sms-gateway/handlers/base/handler_test.go (2)
126-129: Good fix: assert and fail on app.Test errors to avoid nil deref and hidden failuresCapturing and checking the error from app.Test prevents panics and surfaces routing/handler errors clearly in tests.
Also applies to: 188-191, 245-248
256-335: ValidateStruct tests implicitly require Struct-based validation — align base handler accordinglyThese cases (e.g., missing required fields) will only pass if ValidateStruct uses
Validator.Struct(out); the current implementation withVar(..., "required,dive")won’t enforce struct field tags. Ensure the base handler is updated as suggested.
|
Deployed... |
Summary by CodeRabbit
New Features
Documentation
Refactor
Behavior