[messages] add sort order option#254
Conversation
🤖 Pull request artifacts
|
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a ChangesMessage list sorting feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ThirdPartyHandler
participant QueryParams
participant MessagesRepository
Client->>ThirdPartyHandler: GET /3rdparty/v1/messages?sort=-created_at
ThirdPartyHandler->>ThirdPartyHandler: set default Sort if nil (CreatedAtDescending)
ThirdPartyHandler->>QueryParams: ToOptions()
QueryParams->>QueryParams: map Sort to SortField (created_at ASC/DESC)
ThirdPartyHandler->>MessagesRepository: SelectStates(options with SortField)
MessagesRepository->>MessagesRepository: ORDER BY SortField or fallback FIFO order
MessagesRepository-->>ThirdPartyHandler: ordered messages
ThirdPartyHandler-->>Client: message list response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/sms-gateway/modules/messages/repository.go (1)
74-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
SortFieldstring concatenation into SQL ORDER BY is safe today but fragile.
"messages." + options.SortFielddirectly interpolates a raw string into the SQL clause. The current code path only produces"created_at ASC"/"created_at DESC"from a controlled switch inparams.go, so there's no immediate injection. However, the publicSortFieldfield (defined inrepository_filter.go) could be set to arbitrary values by future callers. See the related comment onrepository_filter.gofor a typed-enum refactor suggestion.The switch logic itself (SortField → FIFO → default) is correct and matches the documented override behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/messages/repository.go` around lines 74 - 81, The `MessagesRepository` ORDER BY logic is currently concatenating `options.SortField` directly into the SQL clause, which is fragile even though today it comes from controlled inputs. Update the `MessagesRepository` query-building switch to avoid raw string interpolation by validating `SortField` against an allowlist or using a typed enum from `repository_filter.go`/`params.go`, while preserving the existing `SortField -> FIFO -> default` override behavior.internal/sms-gateway/modules/messages/repository_filter.go (1)
61-64: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider using a typed enum instead of a raw SQL string for
SortField.The field is public and directly concatenated into SQL in
repository.go("messages." + options.SortField). While the current code path only sets it from a controlled switch inparams.go, any future caller could introduce SQL injection by assigning arbitrary user input. A typed enum or validated allowlist would be safer.♻️ Suggested refactor: typed sort field
type SelectOptions struct { WithRecipients bool WithDevice bool WithStates bool WithContent bool // OrderBy sets the retrieval order for pending messages. // Empty (zero) value defaults to "lifo". OrderBy Order - // SortField is a raw ORDER BY clause (e.g. "created_at ASC"). - // When non-empty, it overrides OrderBy. - SortField string + // SortField overrides OrderBy when set. + SortField SortField + Limit int Offset int } + +type SortField string + +const ( + SortByCreatedAtAsc SortField = "created_at ASC" + SortByCreatedAtDesc SortField = "created_at DESC" +) + +// Valid reports whether the SortField is a known safe value. +func (s SortField) Valid() bool { + switch s { + case SortByCreatedAtAsc, SortByCreatedAtDesc: + return true + } + return false +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/modules/messages/repository_filter.go` around lines 61 - 64, Replace the public raw SQL `SortField` on the repository options with a typed, validated sort enum/allowlist so callers cannot inject arbitrary ORDER BY text. Update the sort-selection flow in `params.go` to map only approved values into the new type, and adjust `repository.go` to build the `messages.` ordering clause from that safe enum instead of concatenating `options.SortField`. Keep `repository_filter.go` as the place where the new typed sort field is defined and document the accepted values there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/requests.http`:
- Line 121: The sample request for the messages endpoint uses an invalid
inverted date range, so update the request in the 3rdparty/v1/messages example
to make the from value earlier than the to value. Keep the rest of the query
parameters unchanged and adjust the dates in the GET example so the window is
valid and non-empty.
In `@internal/sms-gateway/handlers/messages/params.go`:
- Around line 48-52: The limit handling in params.go only caps the maximum via
min(*p.Limit, maxLimit), but it still allows 0 or negative values to flow into
options.Limit. Update the limit normalization in the params parsing logic to
enforce a minimum of 1 while still respecting maxLimit, and keep the existing
default of 50 when p.Limit is nil. Use the existing p.Limit and options.Limit
assignment block to apply the lower-bound validation before setting the final
limit.
---
Nitpick comments:
In `@internal/sms-gateway/modules/messages/repository_filter.go`:
- Around line 61-64: Replace the public raw SQL `SortField` on the repository
options with a typed, validated sort enum/allowlist so callers cannot inject
arbitrary ORDER BY text. Update the sort-selection flow in `params.go` to map
only approved values into the new type, and adjust `repository.go` to build the
`messages.` ordering clause from that safe enum instead of concatenating
`options.SortField`. Keep `repository_filter.go` as the place where the new
typed sort field is defined and document the accepted values there.
In `@internal/sms-gateway/modules/messages/repository.go`:
- Around line 74-81: The `MessagesRepository` ORDER BY logic is currently
concatenating `options.SortField` directly into the SQL clause, which is fragile
even though today it comes from controlled inputs. Update the
`MessagesRepository` query-building switch to avoid raw string interpolation by
validating `SortField` against an allowlist or using a typed enum from
`repository_filter.go`/`params.go`, while preserving the existing `SortField ->
FIFO -> default` override behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce60a8d7-0d69-44fb-b0b3-7f981b09b7da
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
api/requests.httpgo.modinternal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/params.gointernal/sms-gateway/modules/messages/repository.gointernal/sms-gateway/modules/messages/repository_filter.gointernal/sms-gateway/openapi/docs.go
capcom6
force-pushed
the
messages/add-sort-order-option
branch
from
c3cde29 to
568c8c9
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes