refactor: introduce entity schemas #973
Conversation
WalkthroughThis change refactors resource filtering and schema validation by replacing the previous filter-based system with a structured schema approach. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RepositoryHandler
participant EntitySchema
participant FieldType
Client->>RepositoryHandler: Call Schema()
RepositoryHandler->>EntitySchema: Return schema with Fields
loop For each filter in request
Client->>EntitySchema: Lookup Field by name
EntitySchema->>FieldType: Validate operator and value
FieldType-->>EntitySchema: Validation result
EntitySchema-->>Client: Accept or reject filter
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #973 +/- ##
==========================================
+ Coverage 82.86% 82.96% +0.09%
==========================================
Files 142 142
Lines 8050 8089 +39
==========================================
+ Hits 6671 6711 +40
- Misses 1054 1059 +5
+ Partials 325 319 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5280d0c to
7d4b5b6
Compare
internal/storage/common/resource.go
Outdated
| if property.Type.IsIndexable() { | ||
| key = strings.Split(key, "[")[0] | ||
| } | ||
| match := func() bool { |
There was a problem hiding this comment.
Since this is in a for loop it might be nice to have this inline function defined somewhere else. It can be harder to track the scope of returns and breaks when there are nested for loops and functions, so moving the function definition outside would improve readability / maintainability.
There was a problem hiding this comment.
I have added a function matchKey to the Field structure.
Is it better?
0c36ccb to
0889870
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
internal/storage/ledger/resource_logs.go (1)
14-21: Schema looks good but consider exposing aliasesThe minimal schema (
date,id) is correct, yet callers often use"timestamp"or"seq"for these columns elsewhere in the code-base. If you expect those keys to be accepted by the automatic validator, addWithAliases()now to avoid scattered manual alias handling later.internal/storage/ledger/resource_accounts.go (2)
16-25: Schema is fine but could mirrorbalance:<asset>pattern
balanceis defined as a numeric map – great.
For asset-scoped keys (balance:USD) the filter implementation relies on a regexp, not on the schema.
If you foresee more typed map fields, consider extendingFieldTypeMapto expose those sub-keys, so this handler does not need its own regex later.
28-29: Chained builder reads better – nitpick on variable naming
ret := ...shadows the package-wideretconvention used elsewhere.qryorqwould remove any doubt that this is a local builder.internal/storage/ledger/resource_aggregated_balances.go (1)
112-129: Operator argument ignored – document intent
ResolveFilternow discards theoperatorparameter (_).
For maintainers it would help to add a short comment explaining that only equality semantics are supported here; this prevents future confusion when someone triesaddress[neq].internal/storage/ledger/resource_transactions.go (3)
15-27: Comprehensive schema – one potential aliasConsider adding
WithAliases("txid")onid– that name crops up in API requests and would plug straight into the new validator layer.
84-114: Filter logic mostly unchanged – minor bool/operator mismatchFor
reverted, theoperatorargument is silently ignored; only the Boolean value is honoured.
Example:reverted[neq]=truewill still be treated as equality.
Either document this special-case or reject any operator excepteqto avoid surprising clients.
120-151: Expand query is heavy – cache or pre-compute?The
effectiveVolumesexpansion nests twoDISTINCT ONwindows plus an aggregation. On large move tables it will be expensive.
Investigate whether a materialised view or a temporary cache keyed by the PIT could off-load this work.internal/storage/system/resource_ledgers.go (2)
30-34: Consider exploiting incoming filters for early push-down
BuildDatasetignores theRepositoryHandlerBuildContext; leveragingctx.UseFilter("bucket"), etc., to add earlyWHEREclauses would reduce dataset size and improve performance.
56-57:LIKEshould probably beILIKEfor case-insensitive matchesIf user queries are meant to be case-insensitive, replace
"like"with"ilike"(or parameterise) to avoid surprising results.internal/storage/common/resource.go (1)
101-126: Minor readability: avoid shadowingkeyRedeclaring
key := keyinside the loop and mutating it later is confusing. Use a distinct variable (lookupKeye.g.) to improve clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/controller/ledger/mocks_test.go(2 hunks)internal/storage/common/resource.go(5 hunks)internal/storage/ledger/resource_accounts.go(2 hunks)internal/storage/ledger/resource_aggregated_balances.go(2 hunks)internal/storage/ledger/resource_logs.go(2 hunks)internal/storage/ledger/resource_transactions.go(3 hunks)internal/storage/ledger/resource_volumes.go(1 hunks)internal/storage/ledger/store.go(0 hunks)internal/storage/system/resource_ledgers.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/ledger/store.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
internal/storage/ledger/resource_logs.go (1)
internal/storage/common/resource.go (5)
EntitySchema(59-61)Field(426-429)NewDateField(462-464)NewNumericField(472-474)ResourceQuery(343-349)
internal/storage/ledger/resource_accounts.go (1)
internal/storage/common/resource.go (8)
EntitySchema(59-61)Field(426-429)NewStringField(457-459)NewDateField(462-464)NewNumericMapField(487-489)NewStringMapField(482-484)RepositoryHandlerBuildContext(63-66)ResourceQuery(343-349)
internal/storage/ledger/resource_volumes.go (1)
internal/storage/common/resource.go (6)
EntitySchema(59-61)Field(426-429)NewStringField(457-459)NewNumericMapField(487-489)NewDateField(462-464)NewStringMapField(482-484)
internal/storage/ledger/resource_aggregated_balances.go (2)
internal/storage/common/resource.go (5)
EntitySchema(59-61)Field(426-429)NewStringField(457-459)NewStringMapField(482-484)ResourceQuery(343-349)internal/controller/ledger/store.go (1)
GetAggregatedVolumesOptions(141-143)
internal/storage/system/resource_ledgers.go (1)
internal/storage/common/resource.go (7)
EntitySchema(59-61)Field(426-429)NewStringField(457-459)NewStringMapField(482-484)RepositoryHandlerBuildContext(63-66)ResourceQuery(343-349)JoinCondition(36-39)
internal/storage/ledger/resource_transactions.go (1)
internal/storage/common/resource.go (9)
EntitySchema(59-61)Field(426-429)NewBooleanField(477-479)NewStringField(457-459)NewDateField(462-464)NewStringMapField(482-484)NewNumericField(472-474)ResourceQuery(343-349)JoinCondition(36-39)
internal/controller/ledger/mocks_test.go (1)
internal/storage/common/resource.go (1)
EntitySchema(59-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (7)
internal/storage/ledger/resource_logs.go (1)
43-45: Project helper – LGTMThe refactor to ignore the now-unused query argument is clean and self-explanatory.
internal/storage/ledger/resource_volumes.go (1)
17-27: Nice, alias “account” is baked inGood use of
WithAliases("account"); the schema is well-aligned with the existing manual filter logic. No further remarks.internal/storage/ledger/resource_accounts.go (1)
108-110: Project – no issuesThe projection remains a simple
*, consistent with earlier behaviour.internal/storage/ledger/resource_aggregated_balances.go (1)
15-22: Lean schema : OKOnly
addressandmetadataare required for aggregated balances; definition is concise and correct.internal/storage/ledger/resource_transactions.go (1)
116-118: Project – no change, fineinternal/controller/ledger/mocks_test.go (1)
144-156: Generated mocks look fine – skipping detailed reviewNo issues spotted in the added
Schema()stub or the newMockFieldType; both follow gomock conventions.Also applies to: 281-345
internal/storage/system/resource_ledgers.go (1)
19-28: Schema definition is clear and consistentFields and types align with downstream filter logic. LGTM.
| if !slices.Contains(property.Type.Operators(), operator) { | ||
| return NewErrInvalidQuery("operator '%s' is not allowed for property '%s'", operator, name) | ||
| } | ||
| ret[property.Name] = value | ||
| break | ||
| } | ||
|
|
||
| if !found { | ||
| return NewErrInvalidQuery("unknown key '%s' when building query", key) | ||
| if err := property.Type.ValidateValue(value); err != nil { | ||
| return NewErrInvalidQuery("invalid value '%v' for property '%s': %s", value, name, err) | ||
| } |
There was a problem hiding this comment.
$exists value is still validated against underlying type
For map fields, ValidateValue delegates to the underlying type and will reject typical $exists payloads (true, false, or nil). Either:
- Skip value-validation in
validateFilterswhenoperator == OperatorExists, or - Make
TypeMap.ValidateValueaccept boolean / nil.
Current behaviour makes legitimate $exists queries fail.
🤖 Prompt for AI Agents
In internal/storage/common/resource.go around lines 112 to 118, the code
validates the value for all operators including the $exists operator, which
causes legitimate $exists queries to fail because the underlying type validation
rejects boolean or nil values. To fix this, modify the validateFilters function
to skip the value validation step when the operator is OperatorExists, ensuring
$exists queries are not incorrectly rejected.
| case OperatorMatch: | ||
| return "=" | ||
| case "$lt": | ||
| case OperatorLT: | ||
| return "<" | ||
| case "$gt": | ||
| case OperatorGT: | ||
| return ">" | ||
| case "$lte": | ||
| case OperatorLTE: | ||
| return "<=" | ||
| case "$gte": | ||
| case OperatorGTE: | ||
| return ">=" | ||
| case "$like": | ||
| case OperatorLike: | ||
| return "like" | ||
| } | ||
| panic("unreachable") |
There was a problem hiding this comment.
$exists not handled – call will panic
ConvertOperatorToSQL omits OperatorExists. If a caller mistakenly routes $exists through this helper, execution reaches panic("unreachable").
+ case OperatorExists:
+ return "is not null"Or guard callers to never invoke this helper with $exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case OperatorMatch: | |
| return "=" | |
| case "$lt": | |
| case OperatorLT: | |
| return "<" | |
| case "$gt": | |
| case OperatorGT: | |
| return ">" | |
| case "$lte": | |
| case OperatorLTE: | |
| return "<=" | |
| case "$gte": | |
| case OperatorGTE: | |
| return ">=" | |
| case "$like": | |
| case OperatorLike: | |
| return "like" | |
| } | |
| panic("unreachable") | |
| case OperatorMatch: | |
| return "=" | |
| case OperatorLT: | |
| return "<" | |
| case OperatorGT: | |
| return ">" | |
| case OperatorLTE: | |
| return "<=" | |
| case OperatorGTE: | |
| return ">=" | |
| case OperatorLike: | |
| return "like" | |
| case OperatorExists: | |
| return "is not null" | |
| } | |
| panic("unreachable") |
🤖 Prompt for AI Agents
In internal/storage/common/resource.go around lines 20 to 33, the
ConvertOperatorToSQL function does not handle the OperatorExists case, causing a
panic if $exists is passed. To fix this, add a case for OperatorExists in the
switch statement and return the appropriate SQL representation or handle it
safely. Alternatively, ensure callers never pass OperatorExists to this function
by adding validation before calling it.
| func (t TypeMap) ValidateValue(value any) error { | ||
| return t.underlyingType.ValidateValue(value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow boolean / nil in TypeMap.ValidateValue
func (t TypeMap) ValidateValue(value any) error {
- return t.underlyingType.ValidateValue(value)
+ // `$exists` usually carries a boolean or is nil
+ if value == nil {
+ return nil
+ }
+ if _, ok := value.(bool); ok {
+ return nil
+ }
+ return t.underlyingType.ValidateValue(value)
}Prevents false negatives on $exists filters without touching other operators.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (t TypeMap) ValidateValue(value any) error { | |
| return t.underlyingType.ValidateValue(value) | |
| } | |
| func (t TypeMap) ValidateValue(value any) error { | |
| // `$exists` usually carries a boolean or is nil | |
| if value == nil { | |
| return nil | |
| } | |
| if _, ok := value.(bool); ok { | |
| return nil | |
| } | |
| return t.underlyingType.ValidateValue(value) | |
| } |
🤖 Prompt for AI Agents
In internal/storage/common/resource.go around lines 566 to 568, the
ValidateValue method on TypeMap currently calls underlyingType.ValidateValue
directly, which does not allow boolean or nil values. Modify the method to
explicitly allow boolean and nil values by returning nil (no error) when the
value is a boolean or nil, and otherwise call underlyingType.ValidateValue. This
change prevents false negatives on $exists filters while preserving validation
for other operators.
No description provided.