Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
WalkthroughAdds a query-template system (types, validation, substitution), RunQuery controller method and v2 HTTP endpoint, a queries package for schema/variable/filter handling, migrates several storage option types to public ledger types, updates pagination/cursor APIs, expands client SDK/types/docs, and adds a DB migration and tests. Changes
sequenceDiagram
participant Client as Client
participant API as API Handler
participant Ctrl as Controller
participant Store as Storage/Resource
participant Renderer as JSON Renderer
Client->>API: POST /v2/{ledger}/queries/{id}/run (schemaVersion, body)
API->>API: parse body -> storage.RunQuery
API->>Ctrl: RunQuery(ctx, schemaVersion, id, RunQuery, paginationConfig)
Ctrl->>Ctrl: resolve template (SchemaData.Queries) & substitute vars
Ctrl->>Store: execute resource-specific query (Transactions/Accounts/Logs/Volumes)
Store-->>Ctrl: return bunpaginate.Cursor[any]
Ctrl-->>API: (*queries.ResourceKind, Cursor)
API->>Renderer: map cursor items per resource kind and marshal
Renderer-->>Client: 200 OK JSON (resource + cursor)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Azorlogh
force-pushed
the
feat/query_templates
branch
2 times, most recently
from
15f387b to
29b4b4b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1235 +/- ##
==========================================
- Coverage 82.20% 81.31% -0.90%
==========================================
Files 198 205 +7
Lines 10224 10905 +681
==========================================
+ Hits 8405 8867 +462
- Misses 1326 1508 +182
- Partials 493 530 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Azorlogh
force-pushed
the
feat/query_templates
branch
from
29b4b4b to
4855dcc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/common/cursor.go (1)
73-102: Critical type mismatch inIteratefunction—querydeclared with wrong type parameter.Line 80 declares
queryasPaginatedQuery[OF]but should bePaginatedQuery[Options]. The variable is initialized frominitialQuery(typeInitialPaginatedQuery[Options]), passed toiteratorwhich expectsPaginatedQuery[Options], and reassigned fromUnmarshalCursor[Options]which returnsPaginatedQuery[Options]. TheOFtype parameter is only for the cursor data returned by the iterator callback, not for query state management. This will cause a compilation error whenOFandOptionsare different types (as they are in all observed call sites).Fix: Change line 80 to
var query PaginatedQuery[Options] = initialQuery
🤖 Fix all issues with AI agents
In `@docs/api/README.md`:
- Around line 2691-2694: The RunQuery response currently lists only
V2AccountsCursorResponse; update the OpenAPI doc table and schema for the
RunQuery endpoint to use a oneOf that includes V2AccountsCursorResponse plus the
other resource-specific cursor responses (e.g., V2TransactionsCursorResponse,
V2LogsCursorResponse, V2VolumesCursorResponse) and keep V2ErrorResponse as the
default error; modify the RunQuery response entry (the row that references
V2AccountsCursorResponse) and add a shared cursor-discriminator or oneOf wrapper
schema so the documented response matches actual behavior.
- Around line 2592-2620: The RunQuery endpoint's body schema only documents
"vars" but omits support for overriding template "params" (e.g., pageSize, sort,
expand, opts), so update the OpenAPI schema used to generate docs: add a body
property "params" (object) alongside "vars" with appropriate properties
(pageSize, sort, expand, opts, cursor, reverse, pit, etc.) and marks/typing
matching existing query parameters, then regenerate the docs so README's
RunQuery section and the table include "» params|body|object|false" and its
additionalProperties/fields; look for symbols like RunQuery, vars, and the body
schema in the OpenAPI spec or generator that produced the README and update
accordingly.
In `@internal/controller/ledger/controller_default_test.go`:
- Around line 23-24: There is a duplicate import of the same package (imported
both as "github.com/formancehq/ledger/internal/storage/common" and as
storagecommon) causing a compile error; remove the duplicate import so the
package is imported once (as common) and then update all references from
storagecommon.* to common.* (search for usages of storagecommon in tests like
controller_default_test.go and the other occurrences around lines 684-690) to
consolidate the alias and fix the build.
In `@internal/controller/ledger/controller_default.go`:
- Around line 674-729: RunQuery/ runQueryFromCursor currently executes an
incoming q.Cursor as-is (via storagecommon.UnmarshalCursor) letting a crafted
cursor bypass template filters; update runQueryFromCursor (and the RunQuery call
path) to validate and enforce the template's constraints by (1) including
template identity/nonce in the serialized cursor metadata and rejecting cursors
whose origin/tag doesn't match the QueryTemplate id, or (2) after
UnmarshalCursor, merge/enforce template.Body and template.Params onto the
deserialized resourceQuery (for each resource kind branch e.g., Transactions,
Accounts, Logs, Volumes) so any filters/options from template override or are
applied to the cursor payload; ensure you handle the different cursor generic
types used (any vs ledger.GetVolumesOptions) and return an error when validation
fails.
In `@internal/controller/ledger/controller_with_traces.go`:
- Around line 586-605: The RunQuery wrapper currently attributes traces/metrics
to "ListSchemas" and uses listSchemasHistogram; update
ControllerWithTraces.RunQuery to call tracing.TraceWithMetric with the correct
span name "RunQuery" and the appropriate metric (replace listSchemasHistogram
with runQueryHistogram or the correct RunQuery histogram field), keeping the
inner call to c.underlying.RunQuery unchanged; if the runQueryHistogram field
does not exist on ControllerWithTraces, add it (or wire the correct histogram
variable) so metrics are recorded under the proper name.
In `@internal/query_template_test.go`:
- Line 57: The test uses require.Equal with arguments reversed causing confusing
diffs; swap the arguments in the assertion so it follows testify's convention:
call require.Equal(t, expected, resolved) instead of require.Equal(t, resolved,
expected) in the test (look for the require.Equal line that compares resolved
and expected in internal/query_template_test.go).
In `@internal/query_template.go`:
- Around line 134-153: resolveFilterTemplate currently panics in the default
branch when it encounters json.Number (from decoder.UseNumber()) or other
primitive types; update the function to handle json.Number explicitly (case
json.Number) by returning it unchanged, and replace the panic in the default
branch with a safe return of m for other primitive types (e.g., bool, nil) so
only string, []any and map[string]any are recursively transformed while numeric
and other primitive values are preserved; reference the resolveFilterTemplate
function and its switch over m to implement these changes.
In `@internal/storage/bucket/default_bucket.go`:
- Around line 20-22: MinimalSchemaVersion is incorrectly set to 49 in
default_bucket.go while the migrations folder only contains migrations 0–48;
change the value of MinimalSchemaVersion to 48 or add a migration "49" to the
migrations directory. Locate the constant MinimalSchemaVersion in
internal/storage/bucket/default_bucket.go and either update its value to 48 or
create/commit the missing migration folder/file named "49" (ensuring it matches
the project's migration naming/format) so the schema version and migrations set
stay in sync.
In `@pkg/client/docs/models/components/querytemplateaccountparams.md`:
- Around line 8-10: The PageSize example value violates the documented maximum;
update the example for the PageSize field in querytemplateaccountparams (symbol:
PageSize) from 100 to 15 (or lower) so it matches the Cursor documentation's
"Maximum page size is set to 15" constraint; edit the Markdown table cell
showing the example default value to 15 and ensure any adjacent examples or
descriptions remain consistent.
In `@pkg/client/docs/models/components/querytemplatelogparams.md`:
- Around line 8-12: Fix the malformed type markup and the inconsistent page-size
documentation: replace occurrences of "`**string*`" and "`**int64*`" with proper
inline code/backtick types like "`string`" and "`int64`" for the Resource,
PageSize, Cursor, Expand fields, and update the PageSize example value to match
the stated max (change the example "100" to "15" or change the textual max to
100) so the maximum page size in the Cursor/PageSize descriptions is consistent;
verify and adjust the Cursor description to reference PageSize max=15 if you
choose 15.
In `@pkg/client/docs/models/components/querytemplatetransactionparams.md`:
- Around line 9-10: The docs are inconsistent: the PageSize example shows 100
while the Cursor text says "Maximum page size is set to 15." Update the
documentation for the QueryTemplateTransactionParams fields so they agree—either
change the PageSize example value to 15 or (preferred) change the Cursor
description to state "Maximum page size is set to 100." Edit the entries for the
PageSize and Cursor fields (symbols: PageSize, Cursor) so both the example value
and the pagination description match the actual API limit.
In `@pkg/client/docs/models/components/querytemplatevolumeparams.md`:
- Around line 11-12: The docs for PageSize and Cursor conflict on the allowed
page size; check the actual API/code that enforces pagination to determine the
correct maximum/default (e.g., 15 or 100), then update the documentation so both
fields are consistent: adjust the PageSize example/default and description or
remove the "Maximum page size is set to 15" clause in the Cursor description to
match PageSize; ensure references to PageSize and Cursor in the
querytemplatevolumeparams component reflect the verified limit and update any
example values accordingly.
In `@pkg/client/docs/models/operations/v2runqueryrequestbody.md`:
- Around line 6-8: The docs show the Vars field as map[string]*string* but the
model uses non-pointer values; update the Vars line in the v2RunQueryRequestBody
documentation so the Type column reads map[string]string (remove the pointer
asterisks) for the `Vars` field.
In `@pkg/client/models/operations/v2runquery.go`:
- Around line 166-170: The V2RunQueryResponse currently only models
V2AccountsCursorResponse which will fail for other templates; update the
response type V2RunQueryResponse to use a polymorphic oneOf (e.g.,
V2AccountsCursorResponse | V2TransactionsCursorResponse | V2LogsCursorResponse |
V2VolumesCursorResponse) or replace it with a generic cursor type (e.g.,
GenericCursorResponse) that can hold varied result items, then regenerate the
client so decoding covers accounts, transactions, logs, or volumes; search for
V2RunQueryResponse and V2AccountsCursorResponse to make the change across the
model and codegen input.
- Around line 37-39: The V2RunQueryRequestBody struct currently only exposes
Vars and therefore cannot override controller template params; update the model
by adding a Params field (e.g., Params map[string]interface{}
`json:"params,omitempty"` or the correct typed struct from the spec) to
V2RunQueryRequestBody so callers can set pagination/opts like groupLvl and
useInsertionDate, then regenerate the client from the OpenAPI spec to ensure the
new field and types are properly reflected across codegen artifacts.
🧹 Nitpick comments (6)
internal/storage/common/pagination.go (1)
41-63: Remove unnecessaryelseafterreturn.The
elseblock after the early return on line 48 is unnecessary and adds nesting. Consider simplifying.♻️ Suggested refactor
func UnmarshalInitialPaginatedQueryOpts[TO any](from InitialPaginatedQuery[map[string]any]) (*InitialPaginatedQuery[TO], error) { var opts TO marshalled, err := json.Marshal(from.Options.Opts) if err != nil { return nil, err } if err := json.Unmarshal(marshalled, &opts); err != nil { return nil, err - } else { - return &InitialPaginatedQuery[TO]{ - PageSize: from.PageSize, - Column: from.Column, - Order: from.Order, - Options: ResourceQuery[TO]{ - PIT: from.Options.PIT, - OOT: from.Options.OOT, - Builder: from.Options.Builder, - Expand: from.Options.Expand, - Opts: opts, - }, - }, nil } + return &InitialPaginatedQuery[TO]{ + PageSize: from.PageSize, + Column: from.Column, + Order: from.Order, + Options: ResourceQuery[TO]{ + PIT: from.Options.PIT, + OOT: from.Options.OOT, + Builder: from.Options.Builder, + Expand: from.Options.Expand, + Opts: opts, + }, + }, nil }pkg/client/docs/models/components/v2querytemplate.md (1)
6-12: Documentation could benefit from field descriptions.All fields show "N/A" for descriptions, which reduces documentation utility for SDK users. If this is auto-generated documentation, consider adding descriptions to the source OpenAPI spec so they propagate to the generated docs. Additionally, the
Namefield typed as*any*is unusual for what is typically a string identifier.pkg/client/docs/models/operations/v2runqueryrequest.md (1)
15-15: Spaces inside emphasis markers cause markdown lint warning.The deprecation notice contains
** DEPRECATED **with spaces inside the asterisks, which triggers the MD037 lint rule. If this documentation is auto-generated from an OpenAPI spec, consider fixing the source template.Additionally, several type fields (lines 11-13, 16-17) show
**int64*or**string*formatting which appears inconsistent—likely should be*int64*or*string*for optional pointer types.internal/api/v2/controllers_queries_run.go (2)
7-7: Remove unused blank import.The blank import of
github.com/pkg/errorshas no apparent side effects and noerrors.*calls exist in this file. This appears to be a leftover.Suggested fix
- _ "github.com/pkg/errors"
31-43: Type switch silently passes through unrecognized types.The default case returns
itemunchanged, which could result in unrendered internal types being exposed if new resource types are added without updating this switch. Consider adding a log statement or comment explaining this is intentional for forward compatibility.internal/query_template.go (1)
25-32: Variable shadowing: loop variabletshadows receivert.The loop variable has the same name as the receiver, which can cause confusion.
Suggested fix
func (t QueryTemplates) Validate() error { - for _, t := range t { - if err := t.Validate(); err != nil { + for _, tmpl := range t { + if err := tmpl.Validate(); err != nil { return err } } return nil }
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/api/README.md`:
- Around line 6796-6801: The README V2QueryParams table contains an invalid link
fragment for the sort parameter (`#/components/parameters/sort`); update the
`sort` entry to point to a valid anchor or remove the broken fragment and inline
the parameter description directly. Locate the `sort` row in the table (look for
the text
"sort|[`#/components/parameters/sort`](`#schema`#/components/parameters/sort)") and
either replace the link with an existing named anchor in the generated docs
(e.g., `#/components/schemas/<correct-anchor>` if available) or remove the
markdown link entirely and paste the full description ("Sort results using a
field name and order... Format: <field>:<order>, where <order> is asc or desc.")
so the README no longer references the invalid `#schema#` fragment.
- Around line 2625-2644: The RunQuery docs contain an H4 "Detailed descriptions"
that jumps from surrounding HTML headings (triggering MD001) and a blockquote
with a blank line (triggering MD028); update the section so heading levels are
consistent with surrounding headings (e.g., use the same level as the HTML
headings or convert the HTML to Markdown) and remove the empty line inside the
blockquote; adjust the "Detailed descriptions" / "Enumerated Values" markup and
any nearby headings (references: the "Detailed descriptions" heading text, the
"Enumerated Values" table, and the blockquote under "Example responses") to
ensure no heading-level jump and no blank lines inside blockquote content.
In `@internal/query_template.go`:
- Around line 39-52: The UnmarshalJSON for VarSpec can leave previous fields
(like Default) when decoding a string form; to fix, reset the receiver to its
zero value at the start of VarSpec.UnmarshalJSON (e.g., *p = VarSpec{}) before
attempting any json.Unmarshal, then proceed with the existing string-unmarshal
branch and the alias-unmarshal branch so decoding always clears stale values;
update the VarSpec.UnmarshalJSON function to perform this reset and keep the
rest of the logic intact.
In `@pkg/client/docs/sdks/v2/README.md`:
- Around line 1878-1883: The RunQuery example's import block in README.md
contains hard tab characters which trigger markdownlint rule MD010; locate the
RunQuery code fence and the import section (the lines listing "context", "os",
"github.com/formancehq/ledger/pkg/client/models/components",
"github.com/formancehq/ledger/pkg/client",
"github.com/formancehq/ledger/pkg/client/models/operations", "log") and replace
all tab characters with spaces so the imports are space-indented (no tabs
anywhere in that code block); re-run markdownlint (MD010) to verify the fix.
♻️ Duplicate comments (1)
internal/query_template.go (1)
116-153: Avoid panics on boolean/null literals in filter templates.
Filters can legitimately includetrue,false, ornull. These currently hit the default branch and panic.🐛 Proposed fix
case map[string]any: for key, value := range v { v[key] = resolveFilterTemplate(value, vars) } - case json.Number: - default: - panic(fmt.Sprintf("unexpected filter shape: %v", v)) + case json.Number, bool, nil: + return v + default: + return v } return m }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/api/v2/controllers_queries_run.go`:
- Around line 42-48: Move the response header setup so headers are sent before
the body is written: set w.Header().Set("Content-Type", "application/json") and
call w.WriteHeader(http.StatusOK) before invoking getJsonResponse(r, w,
*resource, *cursor), and remove the subsequent w.Header().Set / w.WriteHeader
calls after getJsonResponse; this ensures the Content-Type and status are sent
prior to json.NewEncoder(w).Encode() inside getJsonResponse (refer to
getJsonResponse, r, w, resource, cursor).
In `@internal/resources/schema.go`:
- Around line 26-34: The function GetFieldByNameOrAlias returns a pointer to the
range loop variable (field), which can lead to accidental reuse of the same
stack variable; fix by making a distinct heap-backed copy before returning:
inside the loop, when you find a match, assign the map value to a new local
variable (e.g., f := field) and return fieldName, &f instead of &field;
alternatively, convert the map type s.Fields to map[string]*Field and return the
stored pointer directly (adjusting callers), but the quick fix is to create a
new local copy (f) and return its address.
🧹 Nitpick comments (5)
internal/storage/common/paginator_column.go (1)
256-262: Optional: Add pointer type to switch case for defensive consistency.While the codebase currently passes only value types (all field types are constructed via constructors like
NewTypeDate()returning values), the type switch could defensively handle pointers as well, since Go's method sets allow*TypeDateto implement theFieldTypeinterface. Consider updating if pointer types might be introduced in future:Suggested update
func convertPaginationIDToSQLType(fieldType resources.FieldType, id *big.Int) any { switch fieldType.(type) { - case resources.TypeDate: + case resources.TypeDate, *resources.TypeDate: return libtime.UnixMicro(id.Int64()) default: return id } }internal/query_template.go (2)
22-29: Variable shadowing:tused as both receiver and range variable.The range variable
tshadows the receiverton line 23. While this works, it's confusing and could lead to maintenance errors.♻️ Suggested fix
func (t QueryTemplates) Validate() error { - for _, t := range t { - if err := t.Validate(); err != nil { + for _, tmpl := range t { + if err := tmpl.Validate(); err != nil { return err } } return nil }
242-248: Address the TODO comment for nested variable cases.The TODO at line 243 notes that nested cases like
"foo<ba<baz>r>"are not rejected. This could lead to unexpected behavior if users accidentally nest placeholders.Do you want me to help implement validation to reject malformed nested placeholders, or open an issue to track this task?
test/e2e/api_queries_run_test.go (1)
133-165: Avoid taking the address of the range variable in the transaction loop.
&txrefers to the reused loop variable; if the client call ever retains the pointer beyond the call, all iterations could serialize the last value. Please confirm this can’t happen, or use a per‑iteration copy.🛠️ Proposed change
for _, tx := range []components.V2PostTransactionScript{ { Template: pointer.For("DEPOSIT"), Vars: map[string]string{ "dest": "foo:000", "mon": "COIN 42", }, }, { Template: pointer.For("DEPOSIT"), Vars: map[string]string{ "dest": "foo:001", "mon": "COIN 7", }, }, { Template: pointer.For("DEPOSIT"), Vars: map[string]string{ "dest": "bar:000", "mon": "COIN 52", }, }, } { + tx := tx _, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ Ledger: "default", SchemaVersion: &schemaVersion, V2PostTransaction: components.V2PostTransaction{ Force: pointer.For(true), Script: &tx, }, })internal/api/v2/controllers_queries_run.go (1)
19-26: Avoid the marshal/unmarshal round‑trip when addingresource.
You already defineRenderedResponse; embeddingapi.BaseResponselets you encode directly and avoids extra allocations. Please verify the JSON shape stays identical.♻️ Proposed refactor
type RenderedResponse struct { - any + api.BaseResponse[any] Resource resources.ResourceKind `json:"resource,omitempty"` } - -// func renderResponse(json.Raw) { - -// }- { - v := api.BaseResponse[any]{ - Cursor: &renderedCursor, - } - s, err := json.Marshal(v) - if err != nil { - return err - } - var fields map[string]any - err = json.Unmarshal(s, &fields) - if err != nil { - return err - } - fields["resource"] = resource - if err := json.NewEncoder(w).Encode(fields); err != nil { - return err - } - } - return nil + resp := RenderedResponse{ + BaseResponse: api.BaseResponse[any]{Cursor: &renderedCursor}, + Resource: resource, + } + return json.NewEncoder(w).Encode(resp)Also applies to: 53-83
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/query_template.go`:
- Around line 90-113: The Validate method on QueryTemplate returns early when
q.Params is empty, allowing unknown q.Resource values to pass; update
QueryTemplate.Validate so it always checks q.Resource even if len(q.Params) == 0
(e.g. move the resource switch before the params-empty shortcut or add a guard
that verifies the resource kind when params are empty), and for unknown resource
kinds return fmt.Errorf("unknown resource kind: %v", q.Resource) from the
QueryTemplate.Validate function.
- Around line 244-251: Remove the stray stdout logging in parseAccess: delete
the fmt.Printf("%s\n", input) call so the function no longer writes to stdout;
leave the accessRegex.FindStringSubmatch handling and the existing return/error
behavior unchanged (function name: parseAccess, regex variable: accessRegex).
In `@internal/resources/substitution.go`:
- Around line 76-96: The parseVar function currently returns a nil error on EOF
which accepts unterminated templates like "<foo"; modify parserState.parseVar so
that if p.isEOF() is reached before encountering the closing '>' it returns a
descriptive error (e.g. "unterminated variable") instead of returning buf,nil;
ensure the error path is taken after the loop (or when consume indicates EOF)
and keep use of p.consume(), p.isEOF(), and parseVar intact so callers can
detect malformed templates.
- Around line 22-29: The code currently ignores placeholders when a key is
missing (the branch with "if v, ok := vars[varRefs[i]]; ok { ... }"), causing
silent drops; change this to fail fast by returning a clear error when a varRefs
entry is not found in vars (include the missing key name in the error message),
so the caller can detect undeclared variables; keep using jsonToString to
serialize values and buf.WriteString to append when present, but add an explicit
else branch that returns an error referencing the missing varRefs[i] and the
surrounding function name for context.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/query_template.go`:
- Around line 228-237: The function validateParam[Opts any] currently ignores
the generic type and always unmarshals into GetVolumesOptions; change it to
actually use the type parameter by declaring var x Opts and passing &x to
unmarshalWithNumber so the function validates into the generic Opts type
(alternatively remove the generic parameter and keep GetVolumesOptions if
generic behavior isn't needed). Update references to validateParam and remove
the hard-coded GetVolumesOptions usage.
- Around line 189-199: The placeholder "else if true" block must be replaced
with real validation: inspect the actual literal/constant being processed and,
inside the switch on *valueType (cases resources.ValueTypeBoolean,
ValueTypeDate, ValueTypeInt, ValueTypeString), validate the literal matches the
declared type (e.g., parse boolean with strconv.ParseBool for ValueTypeBoolean,
parse integers with strconv.ParseInt for ValueTypeInt, parse dates with
time.Parse using your expected layout for ValueTypeDate, accept/validate string
for ValueTypeString); on mismatch return or propagate an error (or panic
consistently with surrounding code) instead of leaving empty cases, and keep the
default case to surface an unexpected resources.ValueType. Ensure you replace
the "else if true" branch with this validation logic and call out the variable
that holds the literal value in your code.
🧹 Nitpick comments (4)
internal/resources/resources.go (1)
83-96: Consider returning an error instead of panicking on unknown resource kind.Panicking on an unknown
ResourceKindcould cause runtime crashes if input isn't validated upstream. Consider returning an error or using a validation check before calling this function.♻️ Alternative with error return
-func GetResourceSchema(kind ResourceKind) EntitySchema { +func GetResourceSchema(kind ResourceKind) (EntitySchema, error) { switch kind { case ResourceKindAccount: - return AccountSchema + return AccountSchema, nil case ResourceKindLog: - return LogSchema + return LogSchema, nil case ResourceKindTransaction: - return TransactionSchema + return TransactionSchema, nil case ResourceKindVolume: - return VolumeSchema + return VolumeSchema, nil default: - panic(fmt.Sprintf("unexpected resources.ResourceKind: %#v", kind)) + return EntitySchema{}, fmt.Errorf("unexpected resources.ResourceKind: %#v", kind) } }internal/query_template.go (2)
109-113: Remove or address TODO with commented-out code.This commented-out validation logic suggests incomplete implementation. Either implement the common param validation or remove the dead code with an explanatory comment.
366-374: Variable name regex is overly restrictive compared to standard template conventions.The
varRegexpattern^<([a-z_]+)>$only allows lowercase letters and underscores, excluding uppercase letters and digits. This is more restrictive than common template languages (Jinja, Django, Handlebars, Liquid), which typically allow lowercase, uppercase, digits, and underscores. If this restriction is intentional for naming conventions, add a comment explaining the rationale. Otherwise, expand the pattern to^<([a-zA-Z_][a-zA-Z0-9_]*)>$to align with standard identifier rules.internal/query_template_test.go (1)
24-230: Consider usingt.Run()for table-driven subtests.While
t.Parallel()is called, the table-driven tests don't uset.Run(). This means:
- All test cases run sequentially within each function
- Failure messages only show the
tc.namein the assertion message, not in the test output hierarchy- You can't run individual test cases with
-run♻️ Example refactor for TestQueryTemplateValidation
for _, tc := range []testCase{ // ... test cases ... - } { + } { + tc := tc // capture range variable + t.Run(tc.name, func(t *testing.T) { + t.Parallel() var template QueryTemplate err := unmarshalWithNumber([]byte(tc.source), &template) require.NoError(t, err) err = template.Validate() if tc.expectedError == "" { - require.Equal(t, tc.expectedTemplate, template, tc.name) + require.Equal(t, tc.expectedTemplate, template) } else { - require.ErrorContains(t, err, tc.expectedError, tc.name) + require.ErrorContains(t, err, tc.expectedError) } + }) }
Azorlogh
force-pushed
the
feat/query_templates
branch
from
05e72f0 to
6a94d94
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@internal/controller/ledger/controller_default.go`:
- Around line 736-801: The inner switch on template.Resource (handling
ResourceKindTransaction, ResourceKindAccount, ResourceKindLog,
ResourceKindVolume) lacks a default branch so unknown resource kinds silently
yield a nil result; add a default case that returns a descriptive error (e.g.,
fmt.Errorf("unknown resource kind: %s", template.Resource)) to the surrounding
function so callers receive a clear failure instead of a nil cursor. Make the
same change in the parallel non-cursor path so both code paths (the block that
builds resourceQuery/ctrl.store.*().Paginate and the non-cursor equivalent)
return an error for unrecognized template.Resource values; reference the switch
on template.Resource, the result variable, ToInitialPaginatedQuery, and the
ctrl.store.*().Paginate/ bunpaginate.MapCursor usages to locate where to insert
the default error return.
- Around line 675-720: The switch in runQueryFromCursor does not handle unknown
template.Resource values which can lead to returning (&template.Resource, nil,
nil) silently; modify runQueryFromCursor to include a default branch after the
existing cases that returns a clear error (e.g., fmt.Errorf("unsupported
resource kind: %v", template.Resource) or a package-level error) instead of
leaving result nil, so callers get an explicit failure when template.Resource is
not Transaction, Account, Log, or Volume.
In `@internal/queries/filter_template.go`:
- Around line 192-206: The loop currently only resolves string values for
non-operator keys so array values (e.g., for $in) are skipped; update the
non-operator branch in the loop that iterates over v (variables key, value) to
also detect slice/array types and iterate their elements, calling
resolveNestedFilter(schema, key, element, vars) for each string element (and
replacing the element in v[key] with the resolved value), and for nested object
elements handle them appropriately (e.g., call resolveFilterTemplate or recurse)
so arrays of strings used with $in are resolved; keep the existing error
handling and use the same vars/schema/context.
- Around line 272-296: The varRegex in extractVariableName is too restrictive
(^[a-z_]+$) and rejects names allowed by ParseTemplate; update the regex
varRegex to permit digits and uppercase letters (e.g. change to
regexp.MustCompile(`^\${([A-Za-z0-9_]+)}$`) or to the exact same character class
used by ParseTemplate), then keep extractVariableName and extractVariable
behavior the same so variable extraction accepts the broader set of names;
ensure the updated varRegex is used in extractVariableName and tests (if any)
reflect the expanded valid names.
- Around line 61-70: The $in branch currently only validates when operand is
[]any and otherwise silently skips; update the block where operator == "$in"
(the code using variables operator, operand, valueType and calling
validateValue) to return an error if operand is not an array type: after the
type assertion if set, ok := operand.([]any); ok { ... } add an else that
returns a descriptive error (e.g., via fmt.Errorf) stating that $in requires an
array operand and include the actual operand type/value for debugging.
In `@internal/queries/schema.go`:
- Around line 37-56: GetFieldType currently ignores the parsed subcomponent from
parseAccess, allowing accesses like "address[foo]" to pass even when the field
isn't indexable; update EntitySchema.GetFieldType to call parseAccess (already
done) but then check the returned subcomponent (the second return value) and if
it's non-empty verify the resolved field (from GetFieldByNameOrAlias) is
indexable (or has the appropriate property, e.g., field.Indexable or similar)
and return a descriptive error if not; use the existing symbols parseAccess,
accessRegex, EntitySchema.GetFieldType, GetFieldByNameOrAlias and ValueType to
locate the logic and return an error instead of silently accepting bracketed
access for non-indexable fields.
In `@internal/queries/substitution_test.go`:
- Around line 21-22: In substitution_test.go there are duplicated assertions
require.Nil(t, err) repeated back-to-back; remove the duplicate occurrences so
each error check only has a single require.Nil(t, err) call (apply this to the
other duplicated pairs with the same assertion), leaving one require.Nil(t, err)
per test checkpoint (e.g., inside the relevant test functions where require.Nil
is called), then run the tests to ensure no regressions.
- Around line 124-129: The test "invalid var c" is a duplicate of "invalid var
head char"; replace it with a distinct case: update the t.Run name to something
like "invalid var uppercase", call ParseTemplate with an input that triggers an
uppercase-letter error (e.g., "abc$A"), and assert the expected error string
(e.g., "i was expecting a lowercase char, but I got 'A' instead") so the test
covers a different invalid-variable scenario; locate this in the same test
function where ParseTemplate is used.
In `@internal/queries/variables.go`:
- Around line 33-38: The error formatting in ValidateVars uses the whole VarSpec
struct (`spec`) with a `%s` verb causing formatting errors; update the
fmt.Errorf call in function ValidateVars to reference the field `spec.Type` (and
keep printing ValueTypes with `%v`) so the message becomes something like
"variable `%s` has invalid type `%s`, expected one of `%v`" using name,
spec.Type, ValueTypes; ensure the unique symbols ValidateVars, VarSpec,
spec.Type and ValueTypes are used to locate and fix the call.
In `@internal/storage/common/paginator_column.go`:
- Around line 256-259: The type switch in convertPaginationIDToSQLType only
handles the concrete value type queries.TypeDate but not pointer types; update
the switch to include a case for *queries.TypeDate alongside queries.TypeDate so
pointer implementations of FieldType (as asserted by var _ FieldType =
(*TypeDate)(nil)) are handled identically (returning
libtime.UnixMicro(id.Int64())); locate convertPaginationIDToSQLType and add the
pointer case to the switch to ensure robustness when a *queries.TypeDate is
passed.
In `@internal/storage/common/resource.go`:
- Around line 21-34: ConvertOperatorToSQL currently lacks a handler for
queries.OperatorIn which causes a panic when ResolveFilter or resource handlers
pass an `$in` operator; add support by either adding a case in
ConvertOperatorToSQL: case queries.OperatorIn: return "IN", or alternatively
detect queries.OperatorIn early in ResolveFilter and in the specific resource
handlers (logsResourceHandler, schemasResourceHandler, ledgersResourceHandler)
and pre-handle it the same way transactions/accounts do; update
ConvertOperatorToSQL or each handler consistently so calls with OperatorIn never
reach the switch default.
🧹 Nitpick comments (3)
internal/queries/resources.go (1)
16-21: AddResourceKindSchemaor refactor schema handling for consistency.
SchemaSchemais defined and actively used inschemasResourceHandler(internal/storage/ledger/resource_schemas.go), butResourcesandGetResourceSchemadon't expose a schema kind. Either addResourceKindSchemato the constants and wire it intoResources/GetResourceSchema, or refactor schema querying to avoid the architectural gap between the schema handler and the standard query resource system.internal/query_template.go (2)
21-28: Variable shadowing: loop variable shadows receiver.The loop variable
tshadows the method receivert QueryTemplates, which can be confusing.♻️ Suggested rename
func (t QueryTemplates) Validate() error { - for _, t := range t { - if err := t.Validate(); err != nil { + for _, tmpl := range t { + if err := tmpl.Validate(); err != nil { return err } } return nil }
62-75: Consider validating sort column is not empty.If the sort string is just
:desc(starts with colon),parts[0]would be empty, resulting in an emptySortColumn. This might cause unexpected behavior downstream.🛡️ Suggested validation
if x.Sort != "" { parts := strings.SplitN(x.Sort, ":", 2) + if parts[0] == "" { + return fmt.Errorf("invalid sort: column name cannot be empty") + } p.SortColumn = strcase.ToSnake(parts[0])
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/queries/filter_template.go`:
- Around line 51-71: The "$in" branch in the switch erroneously reports the type
of the entire map (`value`) when the operand isn't a slice; update the error to
report the actual operand's type (the variable `operand`) so the message
reflects the unexpected type in the `"$in"` case. Locate the case handling "$in"
in the switch inside the function using singleKey and schema.GetFieldType, and
change the fmt.Errorf to include the type of `operand` instead of `value`; keep
existing validation via validateValue for each element of the operand when it is
a []any.
In `@internal/queries/variables.go`:
- Around line 48-66: The int-default validation in validateValueType (via
validateVariableDefault fallback to float64) currently accepts fractional floats
like 1.5; update validateValueType to reject non-integer float64s by adding a
check in the float64 fallback path that ensures the float64 value has no
fractional part (e.g., value == math.Trunc(value) or using math.Mod) and return
an error if it does; keep using validateVariableDefault for parsing but validate
the parsed float64 is an integer before treating it as an int so templates never
receive fractional ints (refer to validateValueType and validateVariableDefault
to locate the logic).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/query_template.go`:
- Around line 85-99: The loop in QueryTemplateParams.Overwrite currently runs
the unmarshal when the input is literally "null" — invert the check so it skips
empty or "null" inputs and processes real JSON: change the condition to require
len(other) != 0 && !bytes.Equal(bytes.TrimSpace(other), []byte("null")). Inside
that block call unmarshalWithNumber on other for q and q.Opts and return any
errors as before. This fixes the logic in QueryTemplateParams.Overwrite so it
only unmarshals non-null payloads.
🧹 Nitpick comments (2)
internal/queries/filter_template.go (1)
193-204: Error messages show type instead of actual value.The error messages at lines 196 and 203 include
valueType(e.g.,"ValueTypeInt") rather than the actual value that failed parsing. This makes debugging harder.Suggested improvement
- if acc != big.Exact { - return nil, fmt.Errorf("provided number should be an integer: %#v", valueType) - } + if acc != big.Exact { + return nil, fmt.Errorf("provided number should be an integer, got: %v", *v) + } return bigInt, nil } if x, ok := new(big.Int).SetString(string(*v), 10); ok { return x, nil } else { - return nil, fmt.Errorf("provided number should be an integer: %#v", valueType) + return nil, fmt.Errorf("provided number should be an integer, got: %s", *v) }internal/query_template.go (1)
21-28: Variable shadowing reduces readability.The loop variable
tshadows the receivert, which can be confusing.Suggested fix
func (t QueryTemplates) Validate() error { - for _, t := range t { - if err := t.Validate(); err != nil { + for _, qt := range t { + if err := qt.Validate(); err != nil { return err } } return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/queries/field.go`:
- Around line 10-23: The error message in FieldTypeFromString is inconsistent
with the accepted input names: update the return error in FieldTypeFromString to
list the exact valid strings used in the switch (e.g., "boolean", "date", "int",
"string") or include both variants if intended; locate FieldTypeFromString and
change the fmt.Errorf message accordingly so it references "boolean" (and other
exact tokens) instead of "`bool`".
🧹 Nitpick comments (6)
internal/query_template_test.go (2)
259-268: Potential test logic issue:require.NoErrorcalled unconditionally before error check.The test unmarshals the JSON and then calls
require.NoError(t, err)on line 261. However, for error test cases (like "invalid variable type"), if theunmarshalWithNumberitself fails, this assertion will incorrectly fail the test. The error check should account for the possibility that unmarshaling could also be the source of expected errors.Additionally, consider using
t.Run(tc.name, ...)to create subtests for better test isolation and debugging.♻️ Proposed refactor to use subtests
- for _, tc := range []testCase{ + testCases := []testCase{ // ... test cases ... - } { + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() var template QueryTemplate err := unmarshalWithNumber([]byte(tc.source), &template) - require.NoError(t, err) + if err != nil { + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + return + } + require.NoError(t, err) + } err = template.Validate() if tc.expectedError == "" { - require.Equal(t, tc.expectedTemplate, template, tc.name) + require.Equal(t, tc.expectedTemplate, template) } else { - require.ErrorContains(t, err, tc.expectedError, tc.name) + require.ErrorContains(t, err, tc.expectedError) } + }) }
147-179: Missingnamefield in test case.This test case has a description in the JSON source (
"$in filter") but lacks thenamefield in the testCase struct, which could make test output less clear if using subtests.♻️ Add name field
{ + name: "$in filter", source: `{ "description": "$in filter",internal/queries/filter_template_test.go (2)
25-257: Consider using subtests for better test isolation.Similar to the other test file, wrapping each test case in
t.Run(tc.name, ...)would provide better isolation and clearer output when tests fail.
45-47: VarSpec declarations missingTypefield.Several test cases declare
VarSpecwithout aTypefield (e.g.,VarSpec{}). While this may work for resolution tests that rely on runtime value types, it doesn't fully exercise the type validation path. Consider adding explicitTypevalues for more comprehensive coverage.internal/queries/filter_template.go (2)
72-78: Unreachablereturn nilstatement.Line 77 is unreachable because all branches in the switch statement (including the
defaultcase) return before reaching it. This is dead code.♻️ Remove unreachable code
default: return fmt.Errorf("unexpected operator: %s", operator) } - return nil }) }
226-245: Consider improving error message for numeric precision loss.The error message on line 237 (
"provided number should be an integer") doesn't clearly indicate that the issue is precision loss from float64 conversion. Users might be confused when their integer value is rejected.♻️ Improve error message
bigInt, acc := bigFloat.Int(nil) if acc != big.Exact { - return nil, fmt.Errorf("provided number should be an integer: %#v", fieldType) + return nil, fmt.Errorf("numeric value has fractional part or lost precision: %v", *v) }
Azorlogh
force-pushed
the
feat/query_templates
branch
from
c303c37 to
1ea65a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/query_template_test.go`:
- Around line 259-268: The test currently calls template.Validate() even when
unmarshalWithNumber returned an error; change the flow so that after calling err
:= unmarshalWithNumber([]byte(tc.source), &template) you check if err != nil and
in that case assert the expected error (using tc.expectedError) and skip the
Validate/assert-equality checks (e.g., return or continue the test case),
otherwise proceed to call template.Validate() and compare to
tc.expectedTemplate; reference unmarshalWithNumber, the template variable,
QueryTemplate.Validate, tc.expectedError and tc.expectedTemplate to locate the
places to adjust control flow.
🧹 Nitpick comments (3)
internal/query_template_test.go (1)
147-179: Missingnamefield in test case.This test case for
$in filteris missing thenamefield, making it harder to identify in test output. Other test cases include a name for clarity.💡 Proposed fix
{ + name: "$in filter", source: `{ "description": "$in filter",internal/queries/filter_template.go (2)
33-36: Unused variablefilter.The
filtervariable is unmarshalled but never used. The actual filter parsing is done viaquery.ParseJSONon line 39.♻️ Proposed fix to remove dead code
- var filter map[string]any - if err := unmarshalWithNumber(body, &filter); err != nil { - return err - } schema := GetResourceSchema(resource)
196-204: Misleading comment.The comment on line 197 says "we expect the field to be a map" but this case handles
$match,$like,$lt,$gt,$lte,$gteoperators which don't require map fields.📝 Proposed fix
case OperatorMatch, OperatorLike, OperatorLT, OperatorGT, OperatorLTE, OperatorGTE: - // we expect the field to be a map, and the value to match its underlying type + // resolve any variable placeholders in the value if valueStr, ok := value.(string); ok {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/queries/filter_template.go`:
- Around line 230-241: The error messages currently log the variable fieldType
with "%#v" instead of the actual input value that failed validation; update the
two fmt.Errorf calls to include the input value (use *v or string(*v) as
appropriate) and a simple formatting verb like "%v" so the error shows the
actual provided number (e.g., replace fmt.Errorf("provided number should be an
integer: %#v", fieldType) with fmt.Errorf("provided number should be an integer:
%v", *v) in the bigFloat branch and similarly use %v with string(*v) or *v in
the big.Int SetString branch); keep the rest of the logic unchanged.
🧹 Nitpick comments (6)
internal/queries/variables.go (1)
93-106: Minor style: unnecessary else after return.The
elsebranches can be removed since theifblocks return early. This is a minor readability improvement.♻️ Suggested refactor
func castAndValidateValue[T any](value any, validate func(T) error) error { if value == nil { return nil } - if v, ok := value.(T); ok { - if validate != nil { - return validate(v) - } else { - return nil - } - } else { - return errors.New("value doesn't match expected type") + v, ok := value.(T) + if !ok { + return errors.New("value doesn't match expected type") } + if validate != nil { + return validate(v) + } + return nil }internal/queries/filter_template.go (3)
32-35: Remove unusedfiltervariable.The
filtervariable is declared and populated but never used. The JSON parsing is done again viaquery.ParseJSONon line 38.♻️ Suggested fix
- var filter map[string]any - if err := unmarshalWithNumber(body, &filter); err != nil { + var tmp any + if err := unmarshalWithNumber(body, &tmp); err != nil { return err }Alternatively, if the unmarshal is only for validation, consider removing it entirely if
query.ParseJSONalready validates the JSON structure.
121-133: Consider warning or erroring on undeclared callVars.Currently, variables passed in
callVarsthat are not declared invarDeclarationsare silently ignored. This could mask bugs where callers pass misspelled variable names. Consider either returning an error or logging a warning for undeclared variables.♻️ Optional: Add validation for undeclared callVars
for k, v := range callVars { if spec, ok := varDeclarations[k]; ok { fieldType, err := FieldTypeFromString(spec.Type) if err != nil { return nil, err } if err := validateValueType(fieldType, v); err != nil { return nil, err } else { vars[k] = v } + } else { + return nil, fmt.Errorf("undeclared variable: %s", k) } }
192-200: Fix misleading comment.The comment says "we expect the field to be a map" but this case handles all comparison operators on any field type, not just maps.
♻️ Suggested fix
case OperatorMatch, OperatorLike, OperatorLT, OperatorGT, OperatorLTE, OperatorGTE: - // we expect the field to be a map, and the value to match its underlying type + // resolve string values that may contain variable placeholders if valueStr, ok := value.(string); ok {internal/queries/filter_template_test.go (2)
111-119: Consider usingt.Runfor test case isolation.Running test cases directly in a loop without
t.Runmeans all cases share the same test context. Usingt.Run(tc.name, ...)provides better isolation, parallel execution within the test, and clearer failure reporting.♻️ Suggested refactor
- } { - err := ValidateFilterBody(tc.resource, json.RawMessage(tc.source), tc.varDeclarations) - - if tc.expectedError != "" { - require.ErrorContains(t, err, tc.expectedError, tc.name) - } else { - require.NoError(t, err, tc.name) + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := ValidateFilterBody(tc.resource, json.RawMessage(tc.source), tc.varDeclarations) + + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + } else { + require.NoError(t, err) + } + }) } - } }
317-331: Consider usingt.Runfor test case isolation (same as validation tests).Same recommendation as the validation tests - using
t.Runwould improve test organization and failure reporting.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/queries/variables.go`:
- Around line 12-35: The UnmarshalJSON for VarDecl must clear any previous state
before attempting decoding so a prior Default isn't retained when the
string-form path succeeds; at the start of VarDecl.UnmarshalJSON reset the
receiver (e.g. reinitialize p by setting it to its zero value) or explicitly set
p.Default = nil and p.Type = zero before calling unmarshalWithNumber (both in
the string branch and prior to the object decode) so the decoded result never
carries stale defaults; update the VarDecl.UnmarshalJSON function accordingly,
referencing VarDecl.UnmarshalJSON, unmarshalWithNumber, and FieldTypeFromString.
In `@internal/query_template_test.go`:
- Around line 136-145: The test currently ignores the error returned by
template.Validate(); when tc.expectedError == "" you must assert that Validate()
returned no error before comparing templates. Change the success path to call
require.NoError(t, err) (checking the err returned by template.Validate()) and
then require.Equal(t, tc.expectedTemplate, template, tc.name); ensure you
reference the existing variables template, err, tc.expectedTemplate and tc.name
and the call template.Validate() when making this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/api/v2/controllers_queries_run.go`:
- Around line 33-36: The current error handling after calling
getJsonResponse(resource, cursor, r, w) is ineffective because getJsonResponse
may have already written headers/body so calling
w.WriteHeader(http.StatusInternalServerError) does nothing; update the flow so
the response is prepared/validated before writing — either change
getJsonResponse to return the response bytes/body (and any headers) instead of
writing directly so the caller can handle errors and only write to w on success,
or if keeping getJsonResponse as-is, detect if headers have been committed (e.g.
by tracking a written flag inside getJsonResponse or using a responseWriter
wrapper) and on error log the detailed error (using the existing logger) and
return without attempting to WriteHeader; reference getJsonResponse,
w.WriteHeader, and the handler's use of resource/cursor when implementing this
fix.
🧹 Nitpick comments (9)
internal/query_template.go (1)
21-28: Variable shadowing: loop variabletshadows receivert.The loop variable
tshadows the method receivertof typeQueryTemplates. While this doesn't cause a bug in the current implementation, it reduces readability and could lead to confusion in future maintenance.♻️ Suggested fix to avoid shadowing
func (t QueryTemplates) Validate() error { - for _, t := range t { - if err := t.Validate(); err != nil { + for _, tmpl := range t { + if err := tmpl.Validate(); err != nil { return err } } return nil }internal/controller/ledger/controller_default_test.go (1)
24-25: Duplicate import of the same package with different aliases.The package
github.com/formancehq/ledger/internal/storage/commonis imported twice with different aliases (commonandstoragecommon). While Go allows this, it's redundant and reduces code clarity. Consider using a single alias consistently.♻️ Proposed fix to consolidate imports
- "github.com/formancehq/ledger/internal/storage/common" - storagecommon "github.com/formancehq/ledger/internal/storage/common" + "github.com/formancehq/ledger/internal/storage/common"Then update line 698 to use
common.PaginationConfiginstead ofstoragecommon.PaginationConfig:- }, storagecommon.PaginationConfig{ + }, common.PaginationConfig{internal/api/v2/controllers_queries_run_test.go (3)
11-11: Remove debug import before merge.The
spewimport is used only for debugging on line 85 and should be removed for production code.🧹 Proposed fix
- "github.com/davecgh/go-spew/spew"
83-85: Remove ineffective debug code.The slice
bis initialized with zero capacity, sorec.Body.Read(b)reads nothing. Thespew.Dumpthen prints an empty string. This appears to be debug code that should be removed.🧹 Proposed fix
- b := []byte{} - _, _ = rec.Body.Read(b) - spew.Dump(string(b))
88-88: Swaprequire.JSONEqarguments to follow convention.The
require.JSONEqsignature isJSONEq(t, expected, actual, ...). The arguments appear swapped—expectedResponseshould be the second argument.🔧 Proposed fix
- require.JSONEq(t, rec.Body.String(), string(expectedResponse)) + require.JSONEq(t, string(expectedResponse), rec.Body.String())internal/storage/common/pagination.go (1)
52-67: Remove unnecessaryelseafterreturn.The
elseblock is unnecessary since the error case already returns. This is a minor style improvement.🧹 Proposed fix
if err := json.Unmarshal(marshalled, &opts); err != nil { return nil, err - } else { - return &InitialPaginatedQuery[TO]{ - PageSize: from.PageSize, - Column: from.Column, - Order: from.Order, - Options: ResourceQuery[TO]{ - PIT: from.Options.PIT, - OOT: from.Options.OOT, - Builder: from.Options.Builder, - Expand: from.Options.Expand, - Opts: opts, - }, - }, nil } + return &InitialPaginatedQuery[TO]{ + PageSize: from.PageSize, + Column: from.Column, + Order: from.Order, + Options: ResourceQuery[TO]{ + PIT: from.Options.PIT, + OOT: from.Options.OOT, + Builder: from.Options.Builder, + Expand: from.Options.Expand, + Opts: opts, + }, + }, nilinternal/controller/ledger/controller_with_too_many_client_handling.go (1)
17-19: Remove duplicate import alias.The same package
github.com/formancehq/ledger/internal/storage/commonis imported twice with different aliases (commonon line 18 andstoragecommonon line 19). Use a single alias consistently.🧹 Proposed fix
"github.com/formancehq/ledger/internal/queries" - "github.com/formancehq/ledger/internal/storage/common" - storagecommon "github.com/formancehq/ledger/internal/storage/common" + "github.com/formancehq/ledger/internal/storage/common"Then update line 176 to use
common.PaginationConfiginstead ofstoragecommon.PaginationConfig.internal/api/v2/controllers_queries_run.go (1)
8-8: Remove unused blank import.The blank import
_ "github.com/pkg/errors"serves no purpose since it's not used for side effects in this file.🧹 Proposed fix
- _ "github.com/pkg/errors"internal/controller/ledger/controller_with_traces.go (1)
15-17: Remove duplicate import alias.Same as in
controller_with_too_many_client_handling.go, the package is imported twice with different aliases (commonon line 16 andstoragecommonon line 17).🧹 Proposed fix
"github.com/formancehq/ledger/internal/queries" "github.com/formancehq/ledger/internal/storage/common" - storagecommon "github.com/formancehq/ledger/internal/storage/common"Then update line 593 to use
common.PaginationConfiginstead ofstoragecommon.PaginationConfig.
Azorlogh
force-pushed
the
feat/query_templates
branch
from
26cc507 to
db400d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/ledger/controller_with_traces.go`:
- Around line 14-18: The file imports the same package twice
("github.com/formancehq/ledger/internal/storage/common") under two aliases
(`common` and `storagecommon`), causing a compile error; remove the duplicate
alias and use the single `common` import everywhere (including updating the
RunQuery function signature and any references that use `storagecommon`) so all
occurrences reference `common` consistently (e.g., fix the RunQuery signature
and internal usages to use `common`).
In `@internal/query_template.go`:
- Around line 40-71: In UnmarshalJSON for QueryTemplateParams (function
UnmarshalJSON), validate the parsed sort column before assigning p.SortColumn:
after splitting x.Sort into parts, ensure parts[0] is non-empty (trim
whitespace) and return a descriptive error if it is empty (e.g., when x.Sort is
":" or ":desc"); only then set p.SortColumn = strcase.ToSnake(parts[0]) and
continue with the existing SortOrder handling for parts[1]; this prevents silent
acceptance of empty sort columns that produce invalid queries.
🧹 Nitpick comments (5)
internal/query_template_test.go (1)
60-99: Addnamefield to test cases for better failure diagnostics.Two test cases are missing the
namefield, which will result in empty test names in failure output, making debugging harder:
- The
$in filtertest case (lines 60-92)- The
unknown resource kindtest case (lines 93-99)✏️ Suggested fix
{ + name: "$in filter", source: `{ "description": "$in filter", ... { + name: "unknown resource kind", source: `{ "description": "unknown resource kind", "resource": "doesntexist"internal/controller/ledger/controller_with_too_many_client_handling.go (1)
17-19: Redundant import alias.
storagecommonon line 19 aliases the same package already imported ascommonon line 18. This creates confusion and inconsistency—the newRunQuerymethod uses bothcommon.RunQueryandstoragecommon.PaginationConfig.Consider using a single import for the
internal/storage/commonpackage.♻️ Suggested fix
ledger "github.com/formancehq/ledger/internal" "github.com/formancehq/ledger/internal/queries" - "github.com/formancehq/ledger/internal/storage/common" - storagecommon "github.com/formancehq/ledger/internal/storage/common" + "github.com/formancehq/ledger/internal/storage/common" )Then update line 176 to use
common.PaginationConfiginstead ofstoragecommon.PaginationConfig.internal/queries/substitution.go (1)
186-218: Minor inefficiency: string concatenation in loop.Line 210 uses
currentStr += string(b)inside a loop, which creates a new string allocation on each iteration. Consider using astrings.Builderfor better performance with long templates.Proposed fix
func ParseTemplate(str string) ([]string, []string, *ParsingError) { p := parserState{str: str} // The following state is modelled as local scope by design var strs []string var vars []string - currentStr := "" + var currentStr strings.Builder for !p.isEOF() { b := p.consume() switch b { case '$': // TODO do we append even empty str? - strs = append(strs, currentStr) - currentStr = "" + strs = append(strs, currentStr.String()) + currentStr.Reset() var_, err := p.parseSimpleVar() if err != nil { return nil, nil, err } vars = append(vars, var_) default: - currentStr += string(b) + currentStr.WriteByte(b) } } - if currentStr != "" { - strs = append(strs, currentStr) + if currentStr.Len() > 0 { + strs = append(strs, currentStr.String()) } return strs, vars, nil }internal/api/v2/controllers_queries_run.go (1)
8-8: Remove unused blank import.The blank import
_ "github.com/pkg/errors"appears to have no side effects and is not used in this file.Proposed fix
- _ "github.com/pkg/errors"internal/api/router.go (1)
146-150: Consider aligning function naming with v2 router.This file uses
WithPaginationConfigurationwhileinternal/api/v2/routes.gousesWithPaginationConfig. While both are valid and serve different router scopes, aligning the naming would improve consistency across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/queries/filter_template.go`:
- Around line 136-152: query.ParseJSON can return a nil builder (e.g.,
empty/"null" filter), so before calling builder.Walk you must guard against that
to avoid a panic: after builder, err := query.ParseJSON(string(body)) check if
err != nil as today and then add if builder == nil { return nil, nil } (or
return an appropriate empty-filter result) before invoking builder.Walk; update
the code around ParseJSON/builder.Walk (the builder variable and the Walk call)
to perform this nil check and return early instead of calling builder.Walk when
builder is nil.
- Around line 80-95: In validateValue, when value is a string and expectedType
equals TypeString{}, ensure template variable references inside the string are
validated instead of skipping them: detect if the string contains template
syntax (e.g. using existing ParseTemplate on the string) and for each extracted
variable call validateVarRef(expectedType, varName, vars); if no template refs
are present, fall back to validateValueType(expectedType, value) to confirm it's
a plain string. Update validateValue to handle both branches (non-string,
string-with-templates, string-without-templates) using ParseTemplate and
validateVarRef while keeping existing validateValueType checks for non-template
strings.
🧹 Nitpick comments (1)
internal/query_template_test.go (1)
60-92: Name the$intest case for clearer failures.
tc.nameis used in assertions, but this case has an empty name which makes failures harder to trace.💡 Suggested change
{ + name: "in filter", source: `{ "description": "$in filter", "resource": "accounts",
Azorlogh
force-pushed
the
feat/query_templates
branch
from
1bc6f9a to
4d60404
Compare
2 participants
todo: