filter: Add FilterTime #145
Conversation
WalkthroughThe change introduces time-based filtering by CreatedAt across the stack. A new FilterTime type and related constructors (FilterByCreatedAtAfter/Before) are added to filter.go, with accessors on recordFilters. Tests in filter_test.go cover enablement, values, and matching. The in-memory adapter (memrecordstore) applies CreatedAtAfter/Before during List. The SQL adapter adds whereBuilder.AddCondition and applies created_at > / < conditions in List, importing fmt. Adapter tests add a List test case seeding 100 records and verifying CreatedAt filter scenarios. No exported signatures changed except the new public filtering API and whereBuilder method. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
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: 0
🧹 Nitpick comments (4)
filter.go (2)
83-106: Avoid magic numbers and prefer Before/After over Compare for clarity and wider Go compatibilityRelying on time.Time.Compare with sentinel values -1/+1 is less readable and may limit compatibility depending on the Go toolchain. Using Before/After communicates intent and removes the need to remember the compare contract.
Apply this diff to make Matches self-documenting and avoid magic numbers:
func (f FilterTime) Matches(findValue time.Time) bool { - if f.value.Compare(findValue) == f.compare { - return true - } - return false + switch f.compare { + case -1: // match strictly after f.value + return findValue.After(f.value) + case 1: // match strictly before f.value + return findValue.Before(f.value) + default: + return false + } }Optional, but recommended: add short doc comments to the exported type and helpers to clarify exclusivity semantics and compare meanings:
-func makeFilterTime(compare int, value time.Time) FilterTime { +// makeFilterTime constructs a FilterTime. +// compare: -1 means "match strictly after value"; +1 means "match strictly before value". +func makeFilterTime(compare int, value time.Time) FilterTime { return FilterTime{ Enabled: true, compare: compare, value: value, } } -type FilterTime struct { +// FilterTime represents an enabled time-based filter. +// It matches values either strictly after or strictly before the configured value. +type FilterTime struct { Enabled bool compare int value time.Time } +// Value returns the configured boundary time for this filter. func (f FilterTime) Value() time.Time { return f.value }
167-178: Constructors read well; consider a convenience “between” helper and document exclusivityThe After/Before constructors are fine. Consider adding a combined convenience to reduce caller boilerplate and make intent explicit. Also, a brief doc comment stating the comparisons are exclusive would prevent ambiguity.
Proposed helper (outside these lines, for your consideration):
// FilterByCreatedAtBetween returns records with CreatedAt strictly after start and strictly before end. func FilterByCreatedAtBetween(start, end time.Time) RecordFilter { return func(filters *recordFilters) { filters.byCreatedAtAfter = makeFilterTime(-1, start) filters.byCreatedAtBefore = makeFilterTime(1, end) } }filter_test.go (1)
63-75: Add explicit boundary assertions to lock in exclusive semanticsThe production code uses strict comparisons. Add assertions that equality does not match to prevent future regressions.
Apply this diff:
t.Run("Matches", func(t *testing.T) { t0 := time.Date(2025, time.August, 1, 0, 0, 0, 0, time.UTC) t1 := time.Date(2025, time.August, 10, 0, 0, 0, 0, time.UTC) filter := workflow.MakeFilter(workflow.FilterByStatus(9), workflow.FilterByCreatedAtAfter(t0), workflow.FilterByCreatedAtBefore(t1)) require.True(t, filter.ByStatus().Matches("9")) match := time.Date(2025, time.August, 5, 0, 0, 0, 0, time.UTC) require.True(t, filter.ByCreatedAtAfter().Matches(match)) require.True(t, filter.ByCreatedAtBefore().Matches(match)) + + // Boundaries are exclusive by design. + require.False(t, filter.ByCreatedAtAfter().Matches(t0)) + require.False(t, filter.ByCreatedAtBefore().Matches(t1)) })adapters/sqlstore/sqlstore.go (1)
207-211: Add whitespace around the placeholder in AddCondition to avoid dialect quirksSome SQL dialects/parsers are finicky about "?>". Adding spaces improves readability and avoids edge parsing issues.
Apply this diff:
func (fq *whereBuilder) AddCondition(field string, comparison string, value any) { - condition := fmt.Sprintf("(%s %s?)", field, comparison) + condition := fmt.Sprintf("(%s %s ?)", field, comparison) fq.conditions = append(fq.conditions, condition) fq.params = append(fq.params, value) }Optional hardening: if this builder ever accepts external input, restrict comparison to a small allowlist (e.g., ">", "<") before formatting the SQL.
📜 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 (5)
adapters/adaptertest/recordstore.go(1 hunks)adapters/memrecordstore/memrecordstore.go(1 hunks)adapters/sqlstore/sqlstore.go(3 hunks)filter.go(5 hunks)filter_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
adapters/adaptertest/recordstore.go (4)
adapters/memrecordstore/memrecordstore.go (1)
Store(51-66)adapters/webui/internal/api/list.go (1)
List(43-110)order.go (1)
OrderTypeAscending(7-7)filter.go (2)
FilterByCreatedAtAfter(168-172)FilterByCreatedAtBefore(174-178)
filter_test.go (1)
filter.go (4)
MakeFilter(11-18)FilterByCreatedAtAfter(168-172)FilterByCreatedAtBefore(174-178)FilterByStatus(128-146)
🔇 Additional comments (6)
filter.go (2)
6-6: Import looks goodtime is correctly imported for the new time-based filters.
20-26: Clean integration of CreatedAt filter fields and accessorsThe new byCreatedAtAfter/byCreatedAtBefore fields and their accessors are coherent with existing filter patterns and keep the API consistent.
Also applies to: 40-46
adapters/memrecordstore/memrecordstore.go (1)
228-234: LGTM: Correct application of time filters in the in-memory List pathThe new checks short-circuit correctly and preserve existing behaviour. Ordering/pagination paths remain unchanged.
filter_test.go (1)
49-61: Solid coverage for constructor enablement and valuesThese tests validate enablement and the configured boundary. Clear and deterministic by using a fixed UTC date.
adapters/sqlstore/sqlstore.go (1)
173-179: Correct mapping of CreatedAt filters to SQL predicatesUsing created_at > ? and created_at < ? mirrors the exclusive semantics of the in-memory filters.
adapters/adaptertest/recordstore.go (1)
390-438: Good end-to-end coverage for CreatedAt filtering in adaptersThe three scenarios exercise the AND-combination, lower-bound only, and upper-bound only cases. Using a ±1m window avoids flakiness and matches both in-memory and SQL adapters.
|
LGTM |
Adding filter time function to retrieve specific records within time range
For External Submissions:
Summary by CodeRabbit
New Features
Tests