feat: adding filter for segment/portfolio ID for account and creating count transaction endpoint#1911
feat: adding filter for segment/portfolio ID for account and creating count transaction endpoint#1911
Conversation
Extends GET /accounts to support segmentId and portfolioId query filters, enabling the billing plugin to resolve accounts for maintenance billing packages dynamically at calculation time. X-Lerian-Ref: 0x1
Adds GET /transactions/count endpoint with route, status, and date range filters. Includes PostgreSQL query, mock, and route registration to support billing volume calculation in the fees plugin. X-Lerian-Ref: 0x1
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional segment-based filtering across onboarding: Sequence DiagramsequenceDiagram
participant Client
participant HTTPHandler as "HTTP Handler\nCountTransactionsByRoute"
participant Service as "Query Service\nCountTransactionsByRoute"
participant Repository as "Transaction Repo\nCountByRoute"
participant Database as PostgreSQL
Client->>HTTPHandler: GET /v1/.../transactions/count?route=&status=&start_date=&end_date=
HTTPHandler->>HTTPHandler: parse & validate route, status, start_date, end_date
alt validation fails
HTTPHandler->>Client: 400 Bad Request (error)
else
HTTPHandler->>Service: CountTransactionsByRoute(orgID, ledgerID, route, status, from, to)
Service->>Repository: CountByRoute(ctx, orgID, ledgerID, route, status, from, to)
Repository->>Database: SELECT COUNT(*) ... WHERE route=? AND status=? AND created_at BETWEEN ? AND ? AND deleted_at IS NULL
Database->>Repository: count
Repository->>Service: count
Service->>HTTPHandler: CountResponse{period, route, status, totalCount}
HTTPHandler->>Client: 200 OK + JSON CountResponse
end
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/onboarding/internal/adapters/http/in/account.go`:
- Around line 125-126: The `segmentID` declaration is "cuddled" with the
previous declaration `portfolioID`, violating wsl_v5; separate them into
distinct declarations (either add a blank line between `var portfolioID
*uuid.UUID` and `var segmentID *uuid.UUID` or place each in its own `var`
statement) so `segmentID` is not on a cuddled declaration line and the linter
will pass.
In `@components/onboarding/internal/services/query/get-all-metadata-accounts.go`:
- Line 23: GetAllMetadataAccounts currently accepts segmentID but never applies
it, causing metadata queries to return accounts from other segments; update the
query-building logic inside UseCase.GetAllMetadataAccounts to include a filter
for segmentID when segmentID != nil (e.g., add WHERE account.segment_id =
:segmentID or equivalent ORM/DB call), ensure the parameter is bound to the
SQL/ORM call, and mirror the same change in the related helper/path used between
lines ~50-75 so all metadata+segment queries consistently filter by segment;
reference the GetAllMetadataAccounts method and the query construction code
paths to locate where to inject the segmentID predicate and parameter binding.
In `@components/transaction/internal/adapters/http/in/transaction_count_test.go`:
- Around line 24-149: Add a new table-driven test case inside
TestTransactionHandler_CountTransactionsByRoute mirroring the existing "invalid
start_date format returns 400" case: name it "invalid end_date format returns
400", set queryParams to include a valid route and status and start_date but
end_date=bad-date, leave setupMocks as an empty func, set expectedStatus to
nethttp.StatusBadRequest, and add a validateBody that unmarshals the response
and asserts the error response contains "code" (same pattern used by other
validation cases).
In `@components/transaction/internal/adapters/http/in/transaction_count.go`:
- Around line 96-105: Replace the misleading ErrMissingFieldsInRequest used when
uuid.Parse(route) fails with a more specific validation error constant (e.g.,
create or reuse constant.ErrInvalidUUIDFormat or a generic
constant.ErrInvalidFieldFormat similar to ErrInvalidDateFormat) and update the
pkg.ValidateBusinessError call accordingly; ensure the
libOpentelemetry.HandleSpanBusinessErrorEvent invocation, logger.Errorf("Invalid
route UUID": route) and the http.WithError return still use the new
validationErr so telemetry, logs and the HTTP response reflect the correct
"invalid UUID format" semantics (locate the uuid.Parse(route) check and the
ValidateBusinessError call to apply this change).
- Around line 107-129: After parsing startDate and endDate in
transaction_count.go, add a validation that startDate is strictly before
endDate; if not, construct a business error via pkg.ValidateBusinessError (e.g.,
reuse constant.ErrMissingFieldsInRequest or a more appropriate error code) with
a message like "start_date must be before end_date", call
libOpentelemetry.HandleSpanBusinessErrorEvent(&span, "Invalid date range",
validationErr), log the invalid range with logger.Errorf including both
startDateStr and endDateStr, and return http.WithError(c, validationErr) so
inverted ranges are rejected before proceeding.
In
`@components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go`:
- Around line 1168-1169: The count query uses an exclusive upper bound
(squirrel.Expr("created_at < ?", to)) while list queries use an inclusive upper
bound (created_at <= ?), causing mismatches; decide whether the API's documented
semantics should be exclusive or inclusive, then make both the count-building
code (the Where(squirrel.Expr("created_at >= ?",
from)).Where(squirrel.Expr("created_at < ?", to)) call) and the list-building
code that currently uses created_at <= ? use the same operator, update the API
docs to match that chosen semantics, and adjust any tests that assert
counts/lists to reflect the unified behavior (i.e., change "<" to "<=" in the
count or change "<=" to "<" in the list/building functions so both queries are
consistent).
In `@components/transaction/internal/services/query/count-transactions.go`:
- Around line 41-51: Add validation to ensure Period.From is strictly before
Period.To to prevent invalid ranges; implement a Validate() method on the Period
struct (or validate at the callsite that consumes Period) that checks
From.Before(To) and returns an error if not, referencing the Period type and its
From and To fields; invoke this validation wherever Periods are
constructed/accepted (e.g., request parsing or the count transactions
entrypoint) so invalid ranges are rejected with a clear error message.
In
`@components/transaction/migrations/000018_add_idx_transaction_route_status.up.sql`:
- Around line 1-3: Change the index creation in migration
000018_add_idx_transaction_route_status.up.sql to create the index concurrently
to avoid write-blocking: update the CREATE INDEX statement for
idx_transaction_route_status to use CREATE INDEX CONCURRENTLY (preserving the IF
NOT EXISTS, columns organization_id, ledger_id, route, status, created_at, and
the WHERE deleted_at IS NULL clause) so it matches the pattern used by other
migrations (000017/000015/000013) and satisfies the migration linter rule;
ensure any corresponding down migration handles the non-transactional nature of
CONCURRENTLY if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0852977c-84d3-4248-87cf-d4c0b0a5e038
📒 Files selected for processing (23)
components/onboarding/internal/adapters/http/in/account.gocomponents/onboarding/internal/adapters/http/in/account_test.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql_mock.gocomponents/onboarding/internal/services/query/get-all-accounts.gocomponents/onboarding/internal/services/query/get-all-accounts_integration_test.gocomponents/onboarding/internal/services/query/get-all-accounts_test.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts_test.gocomponents/onboarding/migrations/000009_add_account_segment_portfolio_indexes.down.sqlcomponents/onboarding/migrations/000009_add_account_segment_portfolio_indexes.up.sqlcomponents/transaction/internal/adapters/http/in/routes.gocomponents/transaction/internal/adapters/http/in/transaction_count.gocomponents/transaction/internal/adapters/http/in/transaction_count_test.gocomponents/transaction/internal/adapters/postgres/transaction/transaction.postgresql.gocomponents/transaction/internal/adapters/postgres/transaction/transaction.postgresql_mock.gocomponents/transaction/internal/services/query/count-transactions.gocomponents/transaction/internal/services/query/count-transactions_test.gocomponents/transaction/migrations/000018_add_idx_transaction_route_status.down.sqlcomponents/transaction/migrations/000018_add_idx_transaction_route_status.up.sqlpkg/net/http/httputils.gopkg/net/http/httputils_test.go
components/onboarding/internal/services/query/get-all-metadata-accounts.go
Show resolved
Hide resolved
components/transaction/internal/adapters/http/in/transaction_count_test.go
Show resolved
Hide resolved
components/transaction/internal/adapters/http/in/transaction_count.go
Outdated
Show resolved
Hide resolved
components/transaction/internal/adapters/http/in/transaction_count.go
Outdated
Show resolved
Hide resolved
components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go
Outdated
Show resolved
Hide resolved
components/transaction/migrations/000026_add_idx_transaction_route_status.up.sql
Outdated
Show resolved
Hide resolved
# Conflicts: # components/onboarding/internal/services/query/get-all-accounts.go # components/onboarding/internal/services/query/get-all-metadata-accounts.go # components/transaction/internal/adapters/http/in/routes.go # components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go # pkg/net/http/httputils.go # pkg/net/http/httputils_test.go
…-commons to v4 Change PortfolioID and SegmentID from string to *string in QueryHeader to match the RouteID pointer pattern from develop. Migrate transaction count endpoint imports from lib-commons/v3 to v4, fixing logger and span API usage. Renumber migration 000018 to 000026 to resolve conflict with develop. X-Lerian-Ref: 0x1
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 87.1% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/grpc/out |
11.1% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/http/in |
94.6% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/mongodb |
66.7% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/account |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/accounttype |
66.7% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/asset |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/ledger |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/organization |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/portfolio |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/adapters/postgres/segment |
100.0% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/services/command |
88.6% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/services/query |
90.9% |
github.com/LerianStudio/midaz/v3/components/onboarding/internal/services |
0.0% |
Generated by Go PR Analysis workflow
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 85.5% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/grpc/in |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/http/in |
78.8% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/mongodb |
66.7% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/assetrate |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/balance |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/operation |
90.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/operationroute |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/transaction |
97.4% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/postgres/transactionroute |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/rabbitmq |
93.1% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/adapters/redis/balance |
100.0% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/services/command |
90.4% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/services/query |
95.4% |
github.com/LerianStudio/midaz/v3/components/transaction/internal/services |
100.0% |
Generated by Go PR Analysis workflow
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go (1)
1208-1209:⚠️ Potential issue | 🟠 MajorCount window upper-bound semantics remain inconsistent with list queries.
Line 1209 uses
created_at < towhile list flows in this same file use inclusive upper bound (<=). This can still produce count/list mismatches at the exact boundary timestamp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go` around lines 1208 - 1209, The count query uses an exclusive upper bound Where(squirrel.Expr("created_at < ?", to)) which is inconsistent with list queries that use an inclusive upper bound; update the count query to use an inclusive upper bound Where(squirrel.Expr("created_at <= ?", to)) so the count and list flows (refer to the Where(squirrel.Expr("created_at >= ?", from)) / Where(squirrel.Expr("created_at < ?", to)) calls in the transaction PostgreSQL adapter) produce matching semantics at boundary timestamps.components/transaction/internal/adapters/http/in/transaction_count.go (2)
109-131:⚠️ Potential issue | 🟡 MinorValidate that
start_dateis beforeend_date.After parsing both timestamps, inverted ranges are still accepted. Reject invalid ranges before invoking the query layer.
Suggested fix
endDate, err := time.Parse(time.RFC3339, endDateStr) if err != nil { validationErr := pkg.ValidateBusinessError(constant.ErrMissingFieldsInRequest, "Transaction", "end_date must be in RFC3339 format") @@ return http.WithError(c, validationErr) } + + if !startDate.Before(endDate) { + validationErr := pkg.ValidateBusinessError(constant.ErrInvalidDateFormat, "Transaction", "start_date must be before end_date") + + libOpentelemetry.HandleSpanBusinessErrorEvent(span, "Invalid date range", validationErr) + logger.Log(ctx, libLog.LevelError, fmt.Sprintf("Invalid date range: start_date=%s end_date=%s", startDateStr, endDateStr)) + + return http.WithError(c, validationErr) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/internal/adapters/http/in/transaction_count.go` around lines 109 - 131, After successfully parsing startDate and endDate, add a validation that startDate is strictly before endDate; if not, construct a business validation error (similar to existing validationErr patterns) e.g. using pkg.ValidateBusinessError with a message like "start_date must be before end_date", call libOpentelemetry.HandleSpanBusinessErrorEvent(span, "Invalid date range", validationErr), log the problem with logger.Log(ctx, libLog.LevelError, ...) including the parsed values, and return http.WithError(c, validationErr); place this check after the endDate parsing block and follow the same error-handling pattern used for start_date/end_date format errors.
99-125:⚠️ Potential issue | 🟡 MinorUse a format/validation error code instead of “missing field” for malformed values.
Lines 100, 112, and 124 classify malformed UUID/RFC3339 inputs as
ErrMissingFieldsInRequest, but these fields are present and invalid. This makes error semantics misleading for clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/internal/adapters/http/in/transaction_count.go` around lines 99 - 125, The three validation branches that call pkg.ValidateBusinessError for route, startDateStr, and endDateStr currently pass constant.ErrMissingFieldsInRequest (treating malformed UUID/RFC3339 as “missing”); update those calls in the UUID and date parsing blocks (where route is validated via uuid.Parse and where start_date/end_date are parsed) to use a proper validation/format error code instead (e.g., constant.ErrInvalidFieldsInRequest or constant.ErrInvalidFormat); if such a constant does not exist, add a dedicated validation error constant and use it in the three pkg.ValidateBusinessError(...) invocations so the error semantics correctly reflect malformed/invalid values rather than missing fields.components/onboarding/internal/services/query/get-all-metadata-accounts.go (1)
24-24:⚠️ Potential issue | 🟠 Major
segmentIDis accepted but not applied in metadata account retrieval.When
filter.Metadatais used, this path still fetches by IDs + portfolio only, sosegmentIDrequests can leak accounts from other segments.🔧 Minimal fix in this function
func (uc *UseCase) GetAllMetadataAccounts(ctx context.Context, organizationID, ledgerID uuid.UUID, portfolioID, segmentID *uuid.UUID, filter http.QueryHeader) ([]*mmodel.Account, error) { @@ accounts, err := uc.AccountRepo.ListByIDs(ctx, organizationID, ledgerID, portfolioID, uuids) if err != nil { @@ } + + if segmentID != nil { + segment := segmentID.String() + filtered := accounts[:0] + for _, acc := range accounts { + if acc.SegmentID != nil && *acc.SegmentID == segment { + filtered = append(filtered, acc) + } + } + accounts = filtered + } for i := range accounts { if data, ok := metadataMap[accounts[i].ID]; ok { accounts[i].Metadata = data } }Also applies to: 58-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/onboarding/internal/services/query/get-all-metadata-accounts.go` at line 24, GetAllMetadataAccounts currently ignores the segmentID parameter causing metadata queries (when filter.Metadata is provided) to return accounts across segments; update the implementation in GetAllMetadataAccounts to include segmentID in the retrieval criteria (e.g., pass segmentID into the repository call or add it to the constructed filter) so that when segmentID != nil the account lookup filters by organizationID, ledgerID, portfolioID and segmentID; apply the same fix to the related branch/logic referenced between lines 58-83 so both metadata and non-metadata paths respect segmentID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/transaction/internal/adapters/http/in/routes.go`:
- Line 79: The GET route for
"/v1/organizations/:organization_id/ledgers/:ledger_id/transactions" is
registered twice with different auth middleware (protectedMidaz vs
auth.Authorize), which likely unintentionally stacks two auth systems; decide
which auth mechanism should apply and remove the duplicate registration (or
explicitly combine them into a single registration if both are required),
updating the route that references http.ParseUUIDPathParameters("transaction")
and th.GetAllTransactions so only the intended middleware chain (either
protectedMidaz or auth.Authorize(midazName, "transactions", "get")) is attached.
---
Duplicate comments:
In `@components/onboarding/internal/services/query/get-all-metadata-accounts.go`:
- Line 24: GetAllMetadataAccounts currently ignores the segmentID parameter
causing metadata queries (when filter.Metadata is provided) to return accounts
across segments; update the implementation in GetAllMetadataAccounts to include
segmentID in the retrieval criteria (e.g., pass segmentID into the repository
call or add it to the constructed filter) so that when segmentID != nil the
account lookup filters by organizationID, ledgerID, portfolioID and segmentID;
apply the same fix to the related branch/logic referenced between lines 58-83 so
both metadata and non-metadata paths respect segmentID.
In `@components/transaction/internal/adapters/http/in/transaction_count.go`:
- Around line 109-131: After successfully parsing startDate and endDate, add a
validation that startDate is strictly before endDate; if not, construct a
business validation error (similar to existing validationErr patterns) e.g.
using pkg.ValidateBusinessError with a message like "start_date must be before
end_date", call libOpentelemetry.HandleSpanBusinessErrorEvent(span, "Invalid
date range", validationErr), log the problem with logger.Log(ctx,
libLog.LevelError, ...) including the parsed values, and return
http.WithError(c, validationErr); place this check after the endDate parsing
block and follow the same error-handling pattern used for start_date/end_date
format errors.
- Around line 99-125: The three validation branches that call
pkg.ValidateBusinessError for route, startDateStr, and endDateStr currently pass
constant.ErrMissingFieldsInRequest (treating malformed UUID/RFC3339 as
“missing”); update those calls in the UUID and date parsing blocks (where route
is validated via uuid.Parse and where start_date/end_date are parsed) to use a
proper validation/format error code instead (e.g.,
constant.ErrInvalidFieldsInRequest or constant.ErrInvalidFormat); if such a
constant does not exist, add a dedicated validation error constant and use it in
the three pkg.ValidateBusinessError(...) invocations so the error semantics
correctly reflect malformed/invalid values rather than missing fields.
In
`@components/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go`:
- Around line 1208-1209: The count query uses an exclusive upper bound
Where(squirrel.Expr("created_at < ?", to)) which is inconsistent with list
queries that use an inclusive upper bound; update the count query to use an
inclusive upper bound Where(squirrel.Expr("created_at <= ?", to)) so the count
and list flows (refer to the Where(squirrel.Expr("created_at >= ?", from)) /
Where(squirrel.Expr("created_at < ?", to)) calls in the transaction PostgreSQL
adapter) produce matching semantics at boundary timestamps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c115e39a-16fe-4695-95cf-c954ac3af4ce
📒 Files selected for processing (12)
components/onboarding/internal/adapters/http/in/account.gocomponents/onboarding/internal/adapters/http/in/observability.gocomponents/onboarding/internal/adapters/http/in/observability_test.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.gocomponents/onboarding/internal/services/query/get-all-accounts.gocomponents/onboarding/internal/services/query/get-all-accounts_integration_test.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts_test.gocomponents/transaction/internal/adapters/http/in/routes.gocomponents/transaction/internal/adapters/http/in/transaction_count.gocomponents/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go
Remove duplicate route registration for GET /transactions. Use CONCURRENTLY in migration index creation. Align count query date boundary with list queries (inclusive). Add start_date < end_date validation. Use specific error constants for format validation. Add missing test cases for invalid end_date and inverted date range. X-Lerian-Ref: 0x1
Add segmentID parameter to ListByIDs interface and implementation so that metadata-based account queries correctly filter by segment. Previously segmentID was accepted but silently ignored, allowing cross-segment results. X-Lerian-Ref: 0x1
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/transaction/internal/services/query/count-transactions.go (1)
56-81:⚠️ Potential issue | 🟡 MinorEnforce the date-range invariant in the use-case too.
components/transaction/internal/adapters/http/in/transaction_count.go, Lines 133-141 protect the Fiber entrypoint, but this method still forwards inverted ranges straight to the repository. Any non-HTTP caller can therefore bypass the validation and get inconsistent behavior for the same contract.🛠️ Proposed fix
import ( "context" "fmt" "time" libCommons "github.com/LerianStudio/lib-commons/v4/commons" libLog "github.com/LerianStudio/lib-commons/v4/commons/log" libOpentelemetry "github.com/LerianStudio/lib-commons/v4/commons/opentelemetry" + "github.com/LerianStudio/midaz/v3/pkg" + "github.com/LerianStudio/midaz/v3/pkg/constant" "github.com/google/uuid" ) // CountTransactionsByRoute counts transactions for a given route, status, and date range. func (uc *UseCase) CountTransactionsByRoute(ctx context.Context, organizationID, ledgerID uuid.UUID, route, status string, from, to time.Time) (*CountResponse, error) { logger, tracer, _, _ := libCommons.NewTrackingFromContext(ctx) ctx, span := tracer.Start(ctx, "query.count_transactions_by_route") defer span.End() + + if !libCommons.IsInitialDateBeforeFinalDate(from, to) { + validationErr := pkg.ValidateBusinessError(constant.ErrInvalidFinalDate, "") + libOpentelemetry.HandleSpanBusinessErrorEvent(span, "start_date must be before end_date", validationErr) + logger.Log(ctx, libLog.LevelError, fmt.Sprintf("start_date (%s) is not before end_date (%s)", from.Format(time.RFC3339), to.Format(time.RFC3339))) + + return nil, validationErr + } logger.Log(ctx, libLog.LevelInfo, fmt.Sprintf("Counting transactions by route: route=%s status=%s", route, status))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/internal/services/query/count-transactions.go` around lines 56 - 81, CountTransactionsByRoute in UseCase forwards inverted date ranges to the repository without validating the from/to invariant; add a guard at the start of CountTransactionsByRoute to check that from is not after to (e.g., if from.After(to) return a clear error) and return before calling TransactionRepo.CountByRoute so all callers (not just the HTTP entrypoint) get consistent validation and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.go`:
- Line 991: Add integration tests that assert segmentID filtering for both
FindAll and ListByIDs: create accounts across different segmentIDs (e.g., segA,
segB), call AccountRepository.FindAll (the test named
TestIntegration_AccountRepository_FindAll_FiltersByPortfolio can be used as a
template) with a segmentID filter and verify only accounts with that segmentID
are returned, and add analogous assertions for ListByIDs (call repo.ListByIDs
with the new segmentID parameter and ensure results exclude accounts from other
segmentIDs); reference the functions ListByIDs and FindAll and the existing test
TestIntegration_AccountRepository_FindAll_FiltersByPortfolio when adding these
cases.
In
`@components/onboarding/internal/services/query/get-all-metadata-accounts_test.go`:
- Line 231: Add unit tests that exercise the new segmentID parameter for
GetAllMetadataAccounts: update the table-driven tests in
get-all-metadata-accounts_test.go to include cases where the segmentID argument
(replace the current nil passed to uc.GetAllMetadataAccounts) is a non-nil value
that matches some mock metadata accounts and a non-matching value that should
return none; ensure you construct mock accounts with a segment identifier, call
uc.GetAllMetadataAccounts(ctx, tt.organizationID, tt.ledgerID, tt.portfolioID,
tt.segmentID, tt.filter), assert expected filtered results and errors, and
include a combined case verifying segmentID + existing filter behavior (e.g.,
both must match) so GetAllMetadataAccounts filtering logic is covered.
In `@components/transaction/internal/adapters/http/in/transaction_count_test.go`:
- Around line 35-54: The CountByRoute mock and test assertions currently use
gomock.Any() for the period bounds and never check period.from/period.to; update
the setupMocks in transaction_count_test.go so the
transactionRepo.EXPECT().CountByRoute call matches specific from/to values (not
gomock.Any()) for the period arguments and add explicit assertions in
validateBody to unmarshal and compare period.from and period.to strings (or
their expected truncated/normalized form) so the handler must pass the exact
start_date/end_date you expect; reference the setupMocks function and the
CountByRoute expectation and the validateBody closure to implement these checks
(also apply the same change to the other test case around lines 155-163).
- Around line 141-150: Add a new table-driven test case in
transaction_count_test.go mirroring the "start_date after end_date returns 400"
case but with name "start_date equals end_date returns 400" (or similar), set
queryParams to use identical start_date and end_date timestamps (e.g.
start_date=2026-03-01T00:00:00Z&end_date=2026-03-01T00:00:00Z), keep setupMocks
as a no-op, expectedStatus as nethttp.StatusBadRequest, and reuse the same
validateBody closure that unmarshals into errResp and asserts the presence of
"code"; ensure the new entry is added to the test cases slice so the equality
boundary is covered.
---
Duplicate comments:
In `@components/transaction/internal/services/query/count-transactions.go`:
- Around line 56-81: CountTransactionsByRoute in UseCase forwards inverted date
ranges to the repository without validating the from/to invariant; add a guard
at the start of CountTransactionsByRoute to check that from is not after to
(e.g., if from.After(to) return a clear error) and return before calling
TransactionRepo.CountByRoute so all callers (not just the HTTP entrypoint) get
consistent validation and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 849e1fc8-2996-4b03-90c4-07e5fa5cd395
📒 Files selected for processing (11)
components/onboarding/internal/adapters/http/in/account_test.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.gocomponents/onboarding/internal/adapters/postgres/account/account.postgresql_mock.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts.gocomponents/onboarding/internal/services/query/get-all-metadata-accounts_test.gocomponents/transaction/internal/adapters/http/in/transaction_count.gocomponents/transaction/internal/adapters/http/in/transaction_count_test.gocomponents/transaction/internal/adapters/postgres/transaction/transaction.postgresql.gocomponents/transaction/internal/services/query/count-transactions.gocomponents/transaction/migrations/000026_add_idx_transaction_route_status.up.sql
components/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.go
Show resolved
Hide resolved
components/onboarding/internal/services/query/get-all-metadata-accounts_test.go
Show resolved
Hide resolved
components/transaction/internal/adapters/http/in/transaction_count_test.go
Show resolved
Hide resolved
components/transaction/internal/adapters/http/in/transaction_count_test.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ledger/api/docs.go`:
- Around line 6792-6818: Update the OpenAPI param and struct format annotations:
in the HTTP handler parameters in transaction_count.go (the `@Param` annotations
for "route", "start_date", and "end_date") add format(uuid) to the route param
and format(date-time) to both start_date and end_date; and in the CountResponse
struct (components/transaction/internal/services/query/count-transactions.go)
add format: uuid to the Route field's JSON/schema tag so it matches the Period
struct's date-time formatting.
In `@components/ledger/api/swagger.json`:
- Around line 9548-9559: The CountResponse schema's fields lack stricter
constraints: add "format: \"uuid\"" to the "route" property (since it's
described as a UUID) and enforce non-negativity on "totalCount" by adding a
"minimum": 0 to that integer property; update the schema that defines these
properties (look for the CountResponse or the object containing "route" and
"totalCount") so "route" uses the uuid format and "totalCount" has minimum: 0 to
ensure contract correctness.
- Around line 6768-6795: The OpenAPI parameter schemas for the query params
route, start_date, and end_date are currently plain strings but should declare
explicit formats: add "format": "uuid" to the route parameter schema, and add
"format": "date-time" to both start_date and end_date (RFC3339) so validation
and SDK generation pick up UUID and RFC3339 date-time types while keeping their
existing "type", "name", "in", and "required" properties.
In `@components/ledger/api/swagger.yaml`:
- Around line 4614-4617: The OpenAPI parameter definitions for the query
parameter named "route" (description: "Transaction route UUID") are missing an
explicit UUID format; update each "route" parameter schema where name: route and
in: query is defined (including the block with description "Transaction route
UUID" and the similar block around the second occurrence) to keep type: string
and add format: uuid so generated clients and validators treat it as a UUID.
- Around line 4624-4633: The OpenAPI schema for the query params start_date and
end_date currently declares them as plain strings despite stating RFC3339;
update the parameter schemas for name: start_date and name: end_date (in the
swagger.yaml diff) to include format: date-time so the OpenAPI spec exposes
RFC3339 datetime types instead of plain strings.
In `@components/transaction/api/openapi.yaml`:
- Around line 4230-4247: The Period schema currently allows missing boundaries;
make both Period.from and Period.to required by adding a required array to the
Period object schema containing "from" and "to" (i.e., add required:
["from","to"] under the Period definition that contains the properties for from
and to) so the OpenAPI contract enforces both fields are present.
- Around line 2176-2199: The OpenAPI parameter schemas for query params route,
start_date, and end_date lack machine-readable formats; update the parameter
definitions for name: route to include schema.format: uuid, and for name:
start_date and name: end_date to include schema.format: date-time (RFC3339) so
generated tooling/SDKs enforce the UUID and RFC3339 datetime constraints; ensure
you modify the parameter objects named route, start_date, and end_date in the
transaction openapi.yaml to add those format fields without changing their
existing type:string.
- Around line 3407-3433: The CountResponse schema's example uses a string for
period and lacks required declarations; update the example to use an object for
period consistent with the Period schema (e.g., {} or a valid Period object),
set route to a UUID string (e.g., 550e8400-e29b-41d4-a716-446655440010), status
to a sample status value (e.g., APPROVED), and totalCount to an integer (e.g.,
773), and add a required array to the CountResponse schema that includes period,
route, status, and totalCount so these fields are enforced.
In `@components/transaction/api/transaction_docs.go`:
- Around line 4107-4109: The OpenAPI schema for the response property
"totalCount" is missing the format for a 64-bit integer; update the "totalCount"
schema in transaction_docs.go to include "format": "int64" alongside "type":
"integer" so it matches the Go struct field TotalCount (int64) declared in
count-transactions.go, preventing generators from defaulting to 32-bit ints.
- Around line 2751-2778: The OpenAPI docs for the transaction count endpoint are
missing explicit formats: add "format":"uuid" to the "route" query parameter and
"format":"date-time" to both "start_date" and "end_date" fields so client
generators know to validate/encode them as UUID and RFC3339 datetimes; locate
the parameters in components/transaction/api/transaction_docs.go (the block
describing route/status/start_date/end_date) and update the JSON schema for
those entries to include the formats to match the runtime validations in
components/transaction/internal/adapters/http/in/transaction_count.go (see
uuid.Parse(route) and time.Parse(time.RFC3339, ...)).
In `@components/transaction/api/transaction_swagger.json`:
- Around line 4093-4096: The `route` property in transaction_swagger.json is
missing a UUID format specifier; update the `route` field (the "route" parameter
in the transaction schema) to include "format": "uuid" so it matches other UUID
fields like `routeId` in the Operation schema and ensures consistent validation
and documentation; locate the `route` property definition in the JSON and add
the format attribute with the value "uuid".
- Around line 2745-2751: The "route" query parameter object in
transaction_swagger.json is missing the UUID format specifier; update the
parameter with "format": "uuid" (i.e., add a format field to the JSON object for
the parameter named "route") so it matches other UUID parameters like "route_id"
and improves client generation/validation.
In `@components/transaction/api/transaction_swagger.yaml`:
- Around line 346-369: Update the CountResponse and Period schemas so required
fields are explicit: in the CountResponse definition (symbol CountResponse) add
a required array that includes "period", "route", "status", and "totalCount"; in
the Period definition (symbol Period) add a required array that includes "from"
and "to" (referenced as period.from and period.to); and enforce totalCount as
non-negative by adding a minimum: 0 constraint to the totalCount property.
Ensure the referenced symbols CountResponse and Period are the ones updated so
generated clients treat these fields as mandatory.
- Around line 4107-4126: Update the OpenAPI parameter definitions for the
/v1/organizations/{organization_id}/ledgers/{ledger_id}/transactions/count
endpoint so the parameters include explicit formats: add format: uuid to the
"route" query parameter and add format: date-time to both "start_date" and
"end_date" query parameters (leave "status" as string); locate the parameter
blocks for route, status, start_date, and end_date in the
transaction_swagger.yaml and add the format keys to improve client validation
and SDK typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbb2d769-fab7-4ad1-8bf0-4061efcbf354
📒 Files selected for processing (8)
components/ledger/api/docs.gocomponents/ledger/api/swagger.jsoncomponents/ledger/api/swagger.yamlcomponents/transaction/api/openapi.yamlcomponents/transaction/api/transaction_docs.gocomponents/transaction/api/transaction_swagger.jsoncomponents/transaction/api/transaction_swagger.yamlcomponents/transaction/internal/adapters/http/in/routes.go
# Conflicts: # components/ledger/api/docs.go # components/ledger/api/swagger.json # components/ledger/api/swagger.yaml # components/transaction/api/openapi.yaml # components/transaction/api/transaction_docs.go # components/transaction/api/transaction_swagger.json # components/transaction/api/transaction_swagger.yaml
|
This PR is very large (32 files, 1774 lines changed). Consider breaking it into smaller PRs for easier review. |
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 87.1% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/http/in |
86.8% |
Generated by Go PR Analysis workflow
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
components/ledger/api/swagger.yaml (1)
4614-4633:⚠️ Potential issue | 🟡 MinorAdd explicit schema formats for UUID and RFC3339 fields.
Line 4615 and Line 7155 describe UUIDs, and Line 4625/Line 4630 describe RFC3339 datetimes, but the schema currently uses plain
string. Please addformat: uuidandformat: date-timeto keep generated clients/validators strict.Suggested patch
- type: string + format: uuid description: Transaction route UUID name: route in: query required: true @@ - type: string + format: date-time description: Start date (RFC3339 format) name: start_date in: query required: true - type: string + format: date-time description: End date (RFC3339 format) name: end_date in: query required: true @@ route: description: 'Route UUID used as filter @@ type: string + format: uuidAlso applies to: 7154-7159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.yaml` around lines 4614 - 4633, The parameters in the OpenAPI spec use plain string types for UUID and RFC3339 datetime fields; update the parameter schemas for the query params named "route" to include "format: uuid" and for "start_date" and "end_date" to include "format: date-time" (leave "status" as-is), and apply the same changes to the duplicate parameters around the other occurrence (params named route, start_date, end_date in the block referenced near the 7154-7159 section) so generated clients/validators enforce UUID and RFC3339 formats.components/transaction/api/openapi.yaml (2)
3399-3425:⚠️ Potential issue | 🟠 MajorAlign
CountResponseexample and required fields with the schema contract.Line 3400 sets
periodas a string ('{}'), not an object. Also,period,route,status, andtotalCountshould be required to avoid contract drift.Suggested schema fix
CountResponse: @@ example: - period: '{}' - route: route - totalCount: 0 - status: status + period: + from: 2026-01-01T00:00:00Z + to: 2026-02-01T00:00:00Z + route: 550e8400-e29b-41d4-a716-446655440010 + totalCount: 773 + status: APPROVED @@ properties: @@ totalCount: @@ type: integer + required: + - period + - route + - status + - totalCount type: object#!/bin/bash # Inspect CountResponse block for example and required fields rg -n -C6 '^ CountResponse:|^\s+example:|^\s+required:|^\s+period:|^\s+route:|^\s+status:|^\s+totalCount:' components/transaction/api/openapi.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/openapi.yaml` around lines 3399 - 3425, The CountResponse example currently sets period as the string '{}' and omits required fields; update the CountResponse schema (symbol: CountResponse) so its example uses a real object for period (no quotes, e.g. {} or an actual Period object example), ensure example values match types for route (string), status (string) and totalCount (integer), and add a required: [period, route, status, totalCount] entry to the CountResponse properties block so those four fields are mandatory.
2176-2199:⚠️ Potential issue | 🟠 MajorAdd machine-readable formats for
route,start_date, andend_date.Line 2176 and Lines 2188-2199 describe UUID/RFC3339 constraints, but schemas omit
format, so tooling cannot enforce them.Suggested contract fix
- description: Transaction route UUID in: query name: route required: true schema: + format: uuid type: string @@ - description: Start date (RFC3339 format) in: query name: start_date required: true schema: + format: date-time type: string @@ - description: End date (RFC3339 format) in: query name: end_date required: true schema: + format: date-time type: string#!/bin/bash # Verify presence of format annotations for route/start_date/end_date in count endpoint rg -n -C2 'name: route|name: start_date|name: end_date|format:' components/transaction/api/openapi.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/openapi.yaml` around lines 2176 - 2199, The OpenAPI query parameter schemas for route, start_date, and end_date are missing machine-readable format annotations; update the parameter schema entries for name: route to include schema.format = "uuid" and for name: start_date and name: end_date to include schema.format = "date-time" (RFC3339), ensuring the existing type: string remains; locate the parameters by their parameter names (route, start_date, end_date) in the openapi.yaml and add the respective format fields to each schema.components/transaction/api/transaction_docs.go (2)
2751-2777:⚠️ Potential issue | 🟡 MinorAdd explicit UUID/date-time formats to count endpoint query parameters.
Line 2751 (
route) and Line 2766/Line 2773 (start_date,end_date) are missingformatdeclarations, despite UUID/RFC3339 semantics in descriptions.Proposed doc fix
{ "type": "string", "description": "Transaction route UUID", "name": "route", "in": "query", + "format": "uuid", "required": true }, @@ { "type": "string", "description": "Start date (RFC3339 format)", "name": "start_date", "in": "query", + "format": "date-time", "required": true }, { "type": "string", "description": "End date (RFC3339 format)", "name": "end_date", "in": "query", + "format": "date-time", "required": true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/transaction_docs.go` around lines 2751 - 2777, The OpenAPI docs for the transaction count endpoint are missing explicit formats: add "format": "uuid" to the query parameter named "route" and add "format": "date-time" to the query parameters named "start_date" and "end_date" in the transaction_docs.go parameter definitions so the UUID and RFC3339 semantics are machine-readable; locate the parameter objects for "route", "start_date", and "end_date" in the count endpoint docs and add the corresponding format fields.
4081-4091:⚠️ Potential issue | 🟠 MajorFix
CountResponseschema formats forrouteandtotalCount.Line 4081 should declare UUID format, and Line 4089 should declare 64-bit integer format to avoid lossy client type generation.
Proposed doc fix
"route": { "description": "Route UUID used as filter\nexample: 550e8400-e29b-41d4-a716-446655440010", - "type": "string" + "type": "string", + "format": "uuid" }, @@ "totalCount": { "description": "Total number of matching transactions\nexample: 773", - "type": "integer" + "type": "integer", + "format": "int64" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/transaction_docs.go` around lines 4081 - 4091, The CountResponse schema needs explicit formats: update the "route" property to include "format": "uuid" so clients generate UUID types, and update the "totalCount" property to include "format": "int64" so clients generate 64-bit integers; modify the CountResponse definition (properties "route" and "totalCount") to add these format fields alongside the existing "type" entries.components/ledger/api/docs.go (1)
6792-6818:⚠️ Potential issue | 🟡 MinorMissing OpenAPI
formatmetadata for UUID/date-time fields in the new count contract.At Line 6792–6818 and Line 9554–9557,
route,start_date,end_date, andCountResponse.routeare typed as plain strings withoutformat(uuid/date-time). This weakens client generation and validation for the new endpoint.Because this file is generated, please update the source OpenAPI/annotations and regenerate
components/ledger/api/docs.goinstead of editing this file directly.#!/bin/bash set -euo pipefail # Verify generated contract fields are still missing format hints sed -n '6792,6818p' components/ledger/api/docs.go echo "-----" sed -n '9552,9560p' components/ledger/api/docs.go # Locate source-of-truth files for this endpoint and model fd 'transaction_count.go|count-transactions.go|openapi.yaml|transaction_docs.go' components # Check source annotations/spec for explicit format directives rg -n -C2 'transactions/count|name:\s*route|name:\s*start_date|name:\s*end_date|CountResponse|format:\s*(uuid|date-time)' \ components/transaction/api/openapi.yaml \ components/transaction/internal/adapters/http/in/transaction_count.go \ components/transaction/internal/services/query/count-transactions.go \ components/transaction/api/transaction_docs.goAlso applies to: 9554-9557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/docs.go` around lines 6792 - 6818, The OpenAPI docs show route, start_date, end_date and CountResponse.route as plain strings without format metadata; update the source OpenAPI/annotations (not the generated components/ledger/api/docs.go) to add format: uuid for the route UUID fields and format: date-time for start_date and end_date (and CountResponse.route) in the transaction count endpoint spec or the related annotations in transaction_count.go / transaction_docs.go, then re-run the generator to regenerate components/ledger/api/docs.go so the generated schema includes the proper format attributes.components/transaction/api/transaction_swagger.yaml (2)
3956-3975:⚠️ Potential issue | 🟠 MajorAdd explicit formats for
route,start_date, andend_datequery params.The descriptions specify UUID/RFC3339, but the schema does not enforce that.
🔧 Proposed fix
- description: Transaction route UUID in: query name: route required: true + format: uuid type: string @@ - description: Start date (RFC3339 format) in: query name: start_date required: true + format: date-time type: string @@ - description: End date (RFC3339 format) in: query name: end_date required: true + format: date-time type: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/transaction_swagger.yaml` around lines 3956 - 3975, The query parameters 'route', 'start_date', and 'end_date' currently only declare type:string but lack format constraints; update their parameter objects (names: route, start_date, end_date) to include format: uuid for route and format: date-time for start_date and end_date (keeping type: string and required: true) so the OpenAPI schema enforces UUID and RFC3339 datetime formats; leave other params (e.g., status) unchanged.
346-361:⚠️ Potential issue | 🟠 MajorMake
CountResponsea strict contract (required fields + constraints).
period,route,status, andtotalCountare still optional, andtotalCountis unconstrained. Also,routeis documented as UUID but lacksformat: uuid.🔧 Proposed fix
CountResponse: @@ route: @@ + format: uuid type: string @@ totalCount: @@ + minimum: 0 type: integer + required: + - period + - route + - status + - totalCount type: object🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/transaction/api/transaction_swagger.yaml` around lines 346 - 361, Update the CountResponse schema to be a strict contract by adding a required array that includes period, route, status, and totalCount (so those fields are mandatory), change route to include format: uuid to match the documented UUID, and add constraints for totalCount such as type: integer and minimum: 0 (and optionally maximum if needed); ensure the schema entry for CountResponse (the object with properties period, route, status, totalCount) is updated accordingly so the validator enforces presence and the numeric constraint.components/ledger/api/swagger.json (2)
6768-6795:⚠️ Potential issue | 🟡 MinorAdd missing OpenAPI formats for typed query params.
Line 6769 (
route) is UUID but lacksformat: "uuid". Lines 6783 and 6790 (start_date,end_date) are RFC3339 date-times but lackformat: "date-time". This weakens contract validation and SDK typing.📌 Suggested diff
{ "type": "string", + "format": "uuid", "description": "Transaction route UUID", "name": "route", "in": "query", "required": true }, @@ { "type": "string", + "format": "date-time", "description": "Start date (RFC3339 format)", "name": "start_date", "in": "query", "required": true }, { "type": "string", + "format": "date-time", "description": "End date (RFC3339 format)", "name": "end_date", "in": "query", "required": true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 6768 - 6795, Add proper OpenAPI formats to the query parameter definitions to strengthen validation and SDK typing: update the parameter object with "name": "route" to include format: "uuid"; update the parameter objects with "name": "start_date" and "name": "end_date" to include format: "date-time"; leave "status" unchanged. Locate these parameter objects by their names ("route", "start_date", "end_date") in swagger.json and add the respective "format" fields to each JSON object.
9530-9541:⚠️ Potential issue | 🟡 MinorTighten
CountResponsefield constraints for API contract correctness.Line 9530 (
route) should declareformat: "uuid", and Line 9538 (totalCount) should enforceminimum: 0since counts cannot be negative.📌 Suggested diff
"route": { "description": "Route UUID used as filter\nexample: 550e8400-e29b-41d4-a716-446655440010", - "type": "string" + "type": "string", + "format": "uuid" }, @@ "totalCount": { "description": "Total number of matching transactions\nexample: 773", - "type": "integer" + "type": "integer", + "minimum": 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 9530 - 9541, The CountResponse schema's field constraints need tightening: update the "route" property (in the CountResponse definition) to include format: "uuid" alongside type: "string" to enforce UUIDs, and update the "totalCount" property to include minimum: 0 alongside type: "integer" to prevent negative counts; make these changes within the properties for CountResponse (referencing the "route" and "totalCount" property names) so the OpenAPI contract enforces the correct formats and value bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/transaction/api/transaction_swagger.json`:
- Around line 4063-4088: The OpenAPI schema references CountResponse -> Period
but Period is missing; add a new definition named "Period" in the definitions
section that describes the time range (e.g., include properties like startDate
and endDate with appropriate types/format: string with "date" or "date-time"
format, and mark required fields as needed). Ensure the new "Period"
definition's name exactly matches the reference "#/definitions/Period" so
CountResponse's allOf resolves, and update any examples or descriptions if
necessary to match existing conventions used in the file.
In `@components/transaction/api/transaction_swagger.yaml`:
- Around line 342-345: CountResponse.period references a missing schema named
Period; add a definitions entry named Period (or point CountResponse.period to
an existing time-range schema) to restore the OpenAPI contract. Locate the
CountResponse object and either replace the $ref '#/definitions/Period' with an
existing schema name or add a new definitions: Period object that defines the
expected fields (e.g., start/end timestamps and optional granularity/type) so
the $ref resolves during validation and codegen.
---
Duplicate comments:
In `@components/ledger/api/docs.go`:
- Around line 6792-6818: The OpenAPI docs show route, start_date, end_date and
CountResponse.route as plain strings without format metadata; update the source
OpenAPI/annotations (not the generated components/ledger/api/docs.go) to add
format: uuid for the route UUID fields and format: date-time for start_date and
end_date (and CountResponse.route) in the transaction count endpoint spec or the
related annotations in transaction_count.go / transaction_docs.go, then re-run
the generator to regenerate components/ledger/api/docs.go so the generated
schema includes the proper format attributes.
In `@components/ledger/api/swagger.json`:
- Around line 6768-6795: Add proper OpenAPI formats to the query parameter
definitions to strengthen validation and SDK typing: update the parameter object
with "name": "route" to include format: "uuid"; update the parameter objects
with "name": "start_date" and "name": "end_date" to include format: "date-time";
leave "status" unchanged. Locate these parameter objects by their names
("route", "start_date", "end_date") in swagger.json and add the respective
"format" fields to each JSON object.
- Around line 9530-9541: The CountResponse schema's field constraints need
tightening: update the "route" property (in the CountResponse definition) to
include format: "uuid" alongside type: "string" to enforce UUIDs, and update the
"totalCount" property to include minimum: 0 alongside type: "integer" to prevent
negative counts; make these changes within the properties for CountResponse
(referencing the "route" and "totalCount" property names) so the OpenAPI
contract enforces the correct formats and value bounds.
In `@components/ledger/api/swagger.yaml`:
- Around line 4614-4633: The parameters in the OpenAPI spec use plain string
types for UUID and RFC3339 datetime fields; update the parameter schemas for the
query params named "route" to include "format: uuid" and for "start_date" and
"end_date" to include "format: date-time" (leave "status" as-is), and apply the
same changes to the duplicate parameters around the other occurrence (params
named route, start_date, end_date in the block referenced near the 7154-7159
section) so generated clients/validators enforce UUID and RFC3339 formats.
In `@components/transaction/api/openapi.yaml`:
- Around line 3399-3425: The CountResponse example currently sets period as the
string '{}' and omits required fields; update the CountResponse schema (symbol:
CountResponse) so its example uses a real object for period (no quotes, e.g. {}
or an actual Period object example), ensure example values match types for route
(string), status (string) and totalCount (integer), and add a required: [period,
route, status, totalCount] entry to the CountResponse properties block so those
four fields are mandatory.
- Around line 2176-2199: The OpenAPI query parameter schemas for route,
start_date, and end_date are missing machine-readable format annotations; update
the parameter schema entries for name: route to include schema.format = "uuid"
and for name: start_date and name: end_date to include schema.format =
"date-time" (RFC3339), ensuring the existing type: string remains; locate the
parameters by their parameter names (route, start_date, end_date) in the
openapi.yaml and add the respective format fields to each schema.
In `@components/transaction/api/transaction_docs.go`:
- Around line 2751-2777: The OpenAPI docs for the transaction count endpoint are
missing explicit formats: add "format": "uuid" to the query parameter named
"route" and add "format": "date-time" to the query parameters named "start_date"
and "end_date" in the transaction_docs.go parameter definitions so the UUID and
RFC3339 semantics are machine-readable; locate the parameter objects for
"route", "start_date", and "end_date" in the count endpoint docs and add the
corresponding format fields.
- Around line 4081-4091: The CountResponse schema needs explicit formats: update
the "route" property to include "format": "uuid" so clients generate UUID types,
and update the "totalCount" property to include "format": "int64" so clients
generate 64-bit integers; modify the CountResponse definition (properties
"route" and "totalCount") to add these format fields alongside the existing
"type" entries.
In `@components/transaction/api/transaction_swagger.yaml`:
- Around line 3956-3975: The query parameters 'route', 'start_date', and
'end_date' currently only declare type:string but lack format constraints;
update their parameter objects (names: route, start_date, end_date) to include
format: uuid for route and format: date-time for start_date and end_date
(keeping type: string and required: true) so the OpenAPI schema enforces UUID
and RFC3339 datetime formats; leave other params (e.g., status) unchanged.
- Around line 346-361: Update the CountResponse schema to be a strict contract
by adding a required array that includes period, route, status, and totalCount
(so those fields are mandatory), change route to include format: uuid to match
the documented UUID, and add constraints for totalCount such as type: integer
and minimum: 0 (and optionally maximum if needed); ensure the schema entry for
CountResponse (the object with properties period, route, status, totalCount) is
updated accordingly so the validator enforces presence and the numeric
constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9b314d7-7ed0-47eb-8391-11bcf169d311
📒 Files selected for processing (8)
components/ledger/api/docs.gocomponents/ledger/api/swagger.jsoncomponents/ledger/api/swagger.yamlcomponents/transaction/api/openapi.yamlcomponents/transaction/api/transaction_docs.gocomponents/transaction/api/transaction_swagger.jsoncomponents/transaction/api/transaction_swagger.yamlcomponents/transaction/internal/adapters/postgres/transaction/transaction.postgresql.go
CountResponse.period referenced #/definitions/Period which was not defined, causing OpenAPI validation failures and broken code generation. X-Lerian-Ref: 0x1
|
This PR is very large (32 files, 1802 lines changed). Consider breaking it into smaller PRs for easier review. |
Resolves duplicate migration number conflict with 000026_add_route_description_to_operation. X-Lerian-Ref: 0x1
|
This PR is very large (32 files, 1802 lines changed). Consider breaking it into smaller PRs for easier review. |
All query parameters (route, status, start_date, end_date) are now optional. When no dates are provided, defaults to current day UTC. Introduces CountFilter struct for cleaner parameter passing and dynamic SQL query construction. X-Lerian-Ref: 0x1
Sync transaction and ledger swagger docs: remove required on query params, add Period definition with from/to fields, update descriptions with default behavior. X-Lerian-Ref: 0x1
|
This PR is very large (33 files, 1830 lines changed). Consider breaking it into smaller PRs for easier review. |
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Review — feat: account filters (segmentId/portfolioId) + transaction count endpoint
Solid architecture and clean layering throughout. The HTTP→query→repository separation is consistent with Midaz patterns, SQL parameterization via squirrel is correct (no injection risks), and the route registration order avoids the /transactions/:id vs /transactions/count conflict cleanly. OpenAPI was also updated. A few things to address before merge:
🔴 Major — Transaction index poorly aligned with optional-filter behavior
File: components/transaction/migrations/000027_add_idx_transaction_route_status.up.sql
The index column order is (organization_id, ledger_id, route, status, created_at). Since route and status are both optional, the planner can't leverage the created_at range (or status) when route is absent. The default call (no filters, just today's date range) won't benefit from this index on large ledgers.
Suggestion: revisit column ordering based on expected query mix. At minimum, consider an additional partial index on (organization_id, ledger_id, created_at) WHERE deleted_at IS NULL to cover the no-filter case, or reorder columns so created_at comes before the optional fields.
🔴 Major — Test assertions don't verify filter/date correctness
Files: transaction_count_test.go, count-transactions_test.go
Most mock calls use gomock.Any() for filter arguments, so they don't verify:
- that parsed route/status values are exactly propagated
- that the default "today UTC" date boundaries are correctly built
- that start/end dates sent to the repository match expectations
There's also no integration test for the CountByRoute SQL with different filter combinations. For a query-heavy endpoint, correctness depends heavily on predicate composition.
Suggestion: replace Any() with exact CountFilter assertions in handler/query tests; add repository integration tests covering: no filters, route-only, status-only, route+status, and date-boundary inclusiveness.
🟡 Minor — route query param missing format: uuid in OpenAPI schema
Files: openapi.yaml, generated swagger files
Runtime validation enforces UUID for route, but the schema describes it as a plain string. This causes contract drift for clients generated from the spec.
Suggestion: add format: uuid to the route query parameter.
🟡 Minor — has_segment_id missing from observability
File: components/onboarding/internal/adapters/http/in/observability.go
has_portfolio_id was updated for pointer semantics, but has_segment_id wasn't added — even though segment filtering is the primary new feature for Motor 1.
Suggestion: add attribute.Bool("app.request.query.has_segment_id", query.SegmentID != nil).
⚪ Nit — Inconsistent index rollback pattern
File: components/transaction/migrations/000027_add_idx_transaction_route_status.down.sql
Up uses CREATE INDEX CONCURRENTLY but down uses plain DROP INDEX IF EXISTS. Functionally fine, but inconsistent with prior migrations.
Suggestion: align to DROP INDEX CONCURRENTLY IF EXISTS if the migration tooling supports it.
Two blockers before merge: index strategy (can silently kill performance on large ledgers) and filter correctness coverage in tests. Everything else is straightforward to address. Good work overall — the feature design is clean.
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Summary
This PR adds a GET /v1/.../transactions/count endpoint for billing use cases and extends account list/lookup queries with segment_id and portfolio_id filters (promoting PortfolioID from string to *string). The layering is clean — handler → use case → repository — and the migration strategy for the transaction index is solid. However there are a few real issues that need addressing before merge.
✅ What's good
- Transaction migration (
000027) correctly usesCREATE INDEX CONCURRENTLYwith a partial index (WHERE deleted_at IS NULL). Column order in the composite index (org_id, ledger_id, route, status, created_at) is correct for the query pattern. PortfolioID: string → *stringis the right call — differentiates "not provided" from "empty string" without ambiguity.- UUID validation on
routein the count handler returns a 400 instead of a silent DB miss. - Default date range (start/end of today UTC) is sensible and cleanly implemented.
CountByRouteservice layer tests cover success, zero count, and repository error.- Mocks regenerated correctly to match updated repository interfaces.
-
Account indexes missing
CONCURRENTLY(000009_add_account_segment_portfolio_indexes.up.sql): Bothidx_account_segmentandidx_account_portfolioare created withoutCONCURRENTLY. This takes a full table lock onaccountduring migration — the transaction index correctly usesCONCURRENTLY, but these don't. Fix both. -
statusis not validated (CountTransactionsByRoutehandler):statusis passed raw to the DB. A request with?status=garbagereturns HTTP 200 withtotalCount: 0instead of a 400. Since this is a billing endpoint, silent wrong answers are worse than errors. Add a whitelist check against known status values. -
Tests not passing: The PR checklist explicitly has
- [ ] I have ensured that all tests pass.unchecked. This blocks merge. -
No integration test for
FindAllwithsegment_id: The existing integration test was updated to passnilforsegmentID, but there's no test asserting the SQL filter works. DB behavior of the new filter is untested at the integration layer. -
ListByIDs+segmentIDsemantics are unclear: Adding a segment filter to a function that fetches accounts by explicit IDs is an unusual contract. If the intent is intersection ("these IDs and this segment"), the function name and docstring should say so explicitly.
💡 Suggestions
uuid.MustParse(*headerParams.SegmentID)in the handler will panic if called with an invalid UUID outside the current validated flow. Preferuuid.Parse+ explicit error handling.- The count endpoint has no max date range validation. A 6-year range scan is a full table walk — worth a comment or configurable cap if this is intended for billing queries.
Verdict: REQUEST CHANGES — account migrations need CONCURRENTLY, status needs validation, tests must pass before merge.
Renumber transaction index migration from 000027 to 000030 to avoid conflict with develop. Split onboarding migration 000009 into two files (segment + portfolio) so each has a single statement compatible with CREATE INDEX CONCURRENTLY. Add CONCURRENTLY to all index up/down migrations. Replace uuid.MustParse with uuid.Parse for PortfolioID and SegmentID to prevent panics. Add format: uuid to route query param in swagger spec. X-Lerian-Ref: 0x1
|
This PR is very large (35 files, 1844 lines changed). Consider breaking it into smaller PRs for easier review. |
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Third Review — feat: account filters (segmentId/portfolioId) + transaction count endpoint
Good progress on this iteration. Most of the first-round blockers are resolved. The migration split, CONCURRENTLY additions, and UUID parsing improvements are clean. Below is a precise account of what's fixed, what's still open, and one newly found issue.
✅ Fixed since last review
-
CONCURRENTLYon account indexes — Migration split into two files (000009_add_account_segment_portfolio_indexes.up.sqlfor segment,000010_add_account_portfolio_index.up.sqlfor portfolio). Both correctly useCREATE INDEX CONCURRENTLY IF NOT EXISTSandDROP INDEX CONCURRENTLY IF EXISTS. This was the most critical migration risk; it's fully resolved. -
uuid.MustParse→uuid.Parse— BothPortfolioIDandSegmentIDincomponents/onboarding/internal/adapters/http/in/account.go(lines 148–166) now useuuid.Parsewith proper error handling. Panic risk eliminated. -
start_datebeforeend_datevalidation — Added intransaction_count.gousinglibCommons.IsInitialDateBeforeFinalDate. Inverted ranges now correctly return 400. -
Invalid date format returns 400 —
ErrInvalidDatetimeFormatis used for bothstart_dateandend_dateparse failures. Error constants are semantically correct (ErrInvalidQueryParameterfor bad UUID,ErrInvalidDatetimeFormatfor bad RFC3339). -
segmentIDpassed through toListByIDs—GetAllMetadataAccountsinget-all-metadata-accounts.go(line ~55) now passessegmentIDtoListByIDs, which applies the filter in the SQL query (lines 678–679 inaccount.postgresql.go). Metadata path no longer leaks cross-segment results. -
Service-layer test uses exact filter values —
count-transactions_test.gonow passes a concreteCountFilterstruct (notgomock.Any()) toCountByRoute, verifying route/status/date propagation at the service layer.
🔴 Blocker 1 — status still not validated
File: components/transaction/internal/adapters/http/in/transaction_count.go, line 55
status is accepted verbatim and forwarded to the repository with no whitelist check:
status := c.Query("status")
// ... no validation ...
filter := transaction.CountFilter{Status: status, ...}A request like GET /transactions/count?status=garbage returns HTTP 200 with totalCount: 0. For a billing endpoint, silent wrong answers are worse than an explicit error. The valid transaction statuses (e.g., APPROVED, PENDING, COMMITTED, VOIDED) are known — they should be validated at the handler level before touching the DB.
🔴 Blocker 2 — Index column order still doesn't cover the no-filter case
File: components/transaction/migrations/000030_add_idx_transaction_route_status.up.sql
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transaction_route_status
ON "transaction" (organization_id, ledger_id, route, status, created_at)
WHERE deleted_at IS NULL;The column order (org_id, ledger_id, route, status, created_at) requires route to be bound for the index to be useful. When route is omitted (the default call with only a date range), the planner can't use the created_at column — it will fall back to a sequential scan over all rows for that (org_id, ledger_id). On a large ledger this will be slow.
The suggestion from the previous review stands: add a separate partial index covering the no-filter case:
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transaction_created_at
ON "transaction" (organization_id, ledger_id, created_at)
WHERE deleted_at IS NULL;🟡 Minor 1 — Handler tests still use gomock.Any() for filter argument
File: components/transaction/internal/adapters/http/in/transaction_count_test.go
All setupMocks closures match CountByRoute with gomock.Any() for the filter:
transactionRepo.EXPECT().
CountByRoute(gomock.Any(), orgID, ledgerID, gomock.Any()). // filter not verified
Return(int64(773), nil)The service-layer test (count-transactions_test.go) now verifies filter values, but the handler test doesn't verify that:
- Parsed
routeandstatusarrive correctly in theCountFilter - The default date boundaries (start-of-day / end-of-day UTC) are built correctly and passed through
Consider replacing gomock.Any() with exact transaction.CountFilter assertions in at least the "success with all filters" and "success with no filters defaults to today" cases.
🟡 Minor 2 — route query param missing format: uuid in OpenAPI spec
File: components/transaction/api/openapi.yaml, lines ~2179–2183
- description: Transaction route UUID (optional filter)
in: query
name: route
schema:
type: string # ← missing format: uuidRuntime validation enforces UUID format, but the spec doesn't declare it. Add format: uuid to the route schema so generated clients and validators enforce the constraint.
🟡 Minor 3 — has_segment_id missing from query observability
File: components/onboarding/internal/adapters/http/in/observability.go
has_segment_id was added to the payload attributes (line 74), but not to the query attributes. Line 94 has has_portfolio_id for query params but no corresponding has_segment_id:
attribute.Bool("app.request.query.has_portfolio_id", query.PortfolioID != nil),
// ← has_segment_id missing hereGiven that segment filtering is the primary new query capability, this attribute is important for tracing and dashboarding.
⚪ Nit — No integration test for FindAll / ListByIDs with real segment_id
File: components/onboarding/internal/adapters/postgres/account/account.postgresql_integration_test.go
The integration tests were updated to pass nil for segmentID, but there's no test asserting the SQL filter actually works. A test verifying that FindAll with segmentID=X excludes accounts from segment Y would catch regressions in the SQL predicate.
⚪ Nit — Tests checklist still unchecked
The PR description still has - [ ] I have ensured that all tests pass. unchecked. If tests now pass, please check this box.
Summary
| Issue | Status |
|---|---|
| CONCURRENTLY on account indexes (000009, 000010) | ✅ Fixed |
| CONCURRENTLY on transaction index (000030) | ✅ Fixed |
uuid.MustParse → uuid.Parse |
✅ Fixed |
start_date before end_date validation |
✅ Fixed |
segmentID passed to ListByIDs in metadata path |
✅ Fixed |
status not validated in count handler |
🔴 Still open |
| Index column order for no-filter case | 🔴 Still open |
| Handler tests verify exact filter values | 🟡 Still open |
route missing format: uuid in OpenAPI |
🟡 Still open |
has_segment_id in query observability |
🟡 Still open |
Verdict: REQUEST CHANGES — two blockers remain: status validation and index coverage for the no-filter case. Both are straightforward to address.
Add whitelist validation for transaction status query param against known values (CREATED, APPROVED, PENDING, CANCELED, NOTED) returning 400 for invalid input. Add migration 000031 with index on (org_id, ledger_id, created_at) to cover the default no-filter query path. Replace gomock.Any() with exact CountFilter matcher in handler tests and add invalid status test case. Add format: uuid to route param in openapi.yaml. Add has_segment_id to query observability attributes. X-Lerian-Ref: 0x1
|
This PR is very large (37 files, 1949 lines changed). Consider breaking it into smaller PRs for easier review. |
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Both previously-blocking issues have been resolved in this update. Here is a full accounting.
✅ Blocker 1: status not validated — FIXED
transaction_count.go now validates status against a whitelist of known constants before it reaches the DB:
validStatuses := map[string]bool{
constant.CREATED: true,
constant.APPROVED: true,
constant.PENDING: true,
constant.CANCELED: true,
constant.NOTED: true,
}
if !validStatuses[status] {
validationErr := pkg.ValidateBusinessError(constant.ErrInvalidQueryParameter, "", "status")
return http.WithError(c, validationErr)
}?status=GARBAGE now returns HTTP 400. The test suite confirms it ("invalid status returns 400" case in transaction_count_test.go).
✅ Blocker 2: Index column order doesn't cover no-filter case — FIXED
The author split the original single index into two:
000030_add_idx_transaction_route_status.up.sql — filtered queries (route + status):
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transaction_route_status
ON "transaction" (organization_id, ledger_id, route, status, created_at)
WHERE deleted_at IS NULL;000031_add_idx_transaction_date_range.up.sql — date-range-only / no-filter queries:
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transaction_date_range
ON "transaction" (organization_id, ledger_id, created_at)
WHERE deleted_at IS NULL;This is exactly the fix that was requested. The planner can now use idx_transaction_date_range for the default (no route, no status) case and reach created_at directly. Both indexes include WHERE deleted_at IS NULL as a partial index, keeping them lean.
✅ Minor: Handler tests now verify CountFilter fields properly
transaction_count_test.go introduces a typed countFilterMatcher (a custom gomock.Matcher) that verifies route, status, and the date boundaries — replacing the original gomock.Any() approach. The no-filter case validates that the defaults are "start of today UTC" / "end of today UTC".
✅ Minor: route query param now has format: uuid in openapi.yaml
components/transaction/api/openapi.yaml now includes:
- description: Transaction route UUID (optional filter)
in: query
name: route
schema:
format: uuid
type: string✅ Minor: has_segment_id added to observability attributes
components/onboarding/internal/adapters/http/in/observability.go now includes:
attribute.Bool("app.request.query.has_segment_id", query.SegmentID != nil),Minor notes (non-blocking)
-
statusenum not declared in OpenAPI spec — thestatusquery parameter inopenapi.yamlandswagger.yamlis typed astype: stringwith noenumconstraint. Given that validation now exists in code, this is a documentation gap only (clients won't get static hints about valid values). Recommend addingenum: [CREATED, APPROVED, PENDING, CANCELED, NOTED]to the schema. Not a blocker. -
endOfDayuses nanosecond precision —time.Date(..., 23, 59, 59, 999999999, ...)is fine but worth being aware thatBETWEENsemantics in Postgres operate on timestamps; this is effectively inclusive of the full last second of the day. No correctness issue, just something to document in user-facing docs if precision matters. -
routefield inCountResponseis notformat: uuidin the Swagger/OpenAPI schemas (only the query param hasformat: uuid). Minor doc gap.
Summary
All 2 blockers from the previous review are resolved. All 3 minor items from the previous review are addressed. The migration strategy is correct — two purpose-specific partial indexes are better than one combined index that breaks the no-filter query path.
Approving.
3 participants
Midaz Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Obs: Please, always remember to target your PR to develop branch instead of main.
Additional Notes