Merged
Conversation
…iders Reusable testcontainers-based toolkit for integration and E2E tests. Includes infrastructure providers (PostgreSQL, MongoDB, Redis, RabbitMQ, MinIO, MSSQL, MySQL, Oracle, SeaweedFS), chaos testing via Toxiproxy, and addons for e2ekit (app builder with secrets), metricskit, and queuekit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Full E2E test suite covering health checks, security middleware, template CRUD, report generation (HTML/CSV/XML/PDF/TXT), filters, datasource validation, query param validation, and error resilience. Uses itestkit with testcontainers for real infrastructure (Mongo, Postgres, RabbitMQ, Redis, MinIO). Adds make targets for E2E execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove redundant datasource detail fields, simplify schema-qualified table name handling, and clean up unused test assertions. Align init_helpers with new notification and metrics handler signatures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chromium 146 on Alpine 3.23 crashes with Illegal Instruction (SIGILL) on CPUs without certain newer instruction sets. Alpine 3.22 ships Chromium 142 which works correctly for headless PDF generation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughCentralizes datasource connection cleanup into bootstrap: services no longer call datasource Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant Service
participant Datasources
participant Cleanup
rect rgba(100, 200, 150, 0.5)
Note over Bootstrap,Datasources: Initialization
Bootstrap->>Datasources: externalDataSources = ExternalDatasourceConnectionsLazy(logger)
Bootstrap->>Cleanup: register cleanup closure
end
rect rgba(200, 150, 100, 0.5)
Note over Service,Datasources: Runtime data access
Service->>Datasources: getDataSourceDetailsByID(id)
Datasources-->>Service: return schema/details
Note over Service,Datasources: (no CloseConnection call)
end
rect rgba(150, 150, 250, 0.5)
Note over Cleanup,Datasources: Shutdown cleanup
Cleanup->>Datasources: for each ds in GetAll()
Cleanup->>Datasources: if ds.Type == postgres -> ds.PostgresRepository.CloseConnection()
Cleanup->>Datasources: if ds.Type == mongodb -> ds.MongoDBRepository.CloseConnection(ctx)
Datasources-->>Cleanup: report close result (logged)
end
|
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
Contributor
|
This PR is very large (113 files, 20827 lines changed). Consider breaking it into smaller PRs for easier review. |
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 88.2% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/reporter/components/manager/internal/adapters/http/in |
89.2% |
github.com/LerianStudio/reporter/components/manager/internal/services |
89.2% |
Generated by Go PR Analysis workflow
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 90.7% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/reporter/components/worker/internal/services |
92.6% |
Generated by Go PR Analysis workflow
🔒 Security Scan Results —
|
🔒 Security Scan Results —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 60
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/manager/internal/services/get-data-source-information.go (1)
45-64: 🧹 Nitpick | 🔵 TrivialUse the shared datasource type constants in this switch.
This block now sits alongside other datasource-type switches such as
components/manager/internal/bootstrap/config.go, but it still hard-codes"postgresql"and"mongodb". Reusingpkg.PostgreSQLTypeandpkg.MongoDBTypekeeps the listing logic aligned with the rest of the datasource lifecycle code.Suggested change
- switch dataSource.DatabaseType { - case "postgresql": + switch dataSource.DatabaseType { + case pkg.PostgreSQLType: if dataSource.DatabaseConfig == nil { uc.Logger.Log(ctx, log.LevelError, "PostgreSQL datasource has nil DatabaseConfig", log.String("key", key)) continue } @@ - case "mongodb": + case pkg.MongoDBType: dataSourceInformation = &model.DataSourceInformation{ Id: key, ExternalName: dataSource.MongoDBName, Type: dataSource.DatabaseType, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/manager/internal/services/get-data-source-information.go` around lines 45 - 64, Replace the hard-coded datasource type strings in the switch over dataSource.DatabaseType with the shared constants to keep behavior consistent: use pkg.PostgreSQLType instead of "postgresql" and pkg.MongoDBType instead of "mongodb" in the switch cases for the function handling dataSource.DatabaseType (the switch that creates model.DataSourceInformation), and add the corresponding import for the pkg package if not already present; ensure the case labels match the existing constants' names exactly and run tests/build to verify no import name collisions.
🤖 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/manager/internal/bootstrap/config.go`:
- Around line 447-470: The cleanup loop currently creates a plain background ctx
and passes it to MongoDBRepository.CloseConnection even though that method
ignores it; instead create a single timeout context for the entire external
datasource cleanup (e.g. ctx, cancel :=
context.WithTimeout(context.Background(), 10*time.Second); defer cancel())
before iterating externalDataSources.GetAll(), use that ctx for
MongoDBRepository.CloseConnection(ctx) calls (and any future context-accepting
CloseConnection variants), and remove/replace the existing background ctx usage
in this block so all datasource closes are bounded by the single timeout.
In `@components/worker/Dockerfile`:
- Line 24: Add an inline comment above the FROM alpine:3.22 line in the
Dockerfile explaining that the image is pinned to Alpine 3.22 as a pragmatic
workaround for Chromium 146 triggering SIGILL on some architectures, that Alpine
3.22 includes Chromium 142 which avoids the issue, and note the support horizon
(Alpine 3.22 supported until May 2027) plus a reminder to revisit/remove the pin
once Chromium/Alpine versions are upgraded; include this comment near the
existing FROM alpine:3.22 entry so future maintainers understand why the version
is pinned and when to reassess.
In `@mk/tests.mk`:
- Around line 225-247: The test-e2e Make target currently runs "go test -v
-tags=e2e -timeout 30m -count=1 ./tests/e2e/..." without the -race flag or
gotestsum wrapper; update the test-e2e target (the rule that prints "Running E2E
tests" and runs the go test command) to either add a comment explaining why
-race is intentionally omitted for long-running E2E tests, or align it with
other targets by adding the -race flag and wrapping the invocation with
GOTESTSUM (same pattern used by test-unit/test-integration/test-chaos) so
results and exit codes are handled consistently.
In `@pkg/datasource-config_test.go`:
- Around line 1243-1253: Update the misleading "Eager connect fails" comments
and assertion messages in the lazy-mode test so they reflect that connections
are "not attempted in lazy mode" rather than failing; specifically change the
comment and the assert.False message for pgDS and mongoDS in this test (the
variables pgDS, mongoDS and the result map) to indicate "not attempted in lazy
mode" or similar wording consistent with the lazy-mode contract already
validated in TestInitPostgresDataSource_LazyMode.
In `@pkg/itestkit/addons/e2ekit/builder.go`:
- Around line 108-116: Add a short doc comment above the Builder methods
explaining that WithNetworks replaces the networks slice (it makes a defensive
copy and sets networks) while WithExtraHosts appends to the existing extraHosts
slice (preserving defaults such as "host.docker.internal:host-gateway"); mention
the rationale for the asymmetry so callers know append-vs-replace semantics when
using WithNetworks and WithExtraHosts.
- Around line 441-468: The function dumpRecentLogs currently reads from the
start of the log stream (using c.Logs() and io.ReadFull) but claims to show the
"last ~N bytes"; update dumpRecentLogs to actually tail the logs by either using
the logs API's Tail option when calling c.Logs() (if supported by the
testcontainers implementation) or by reading the entire stream and retaining
only the last maxBytes via a small ring buffer/bytes.Buffer rotation before
logging; reference the dumpRecentLogs function, the c.Logs() call, the maxBytes
parameter, and the current io.ReadFull/read usage to locate and replace the
logic (or alternatively, if opting for the simpler change, adjust the log
message to accurately say "first ~N bytes" instead of "last ~N bytes").
In `@pkg/itestkit/addons/e2ekit/helpers.go`:
- Line 35: The call to runtime.Caller(0) currently discards the boolean ok and
can yield an empty file path; update the runtime.Caller(0) invocation in
helpers.go to capture and check the ok value (e.g., ok, file :=
runtime.Caller(0) or _, file, _, ok := runtime.Caller(0)) and handle the false
case explicitly: return an error or log/fatal with a clear message so the
subsequent project-root lookup/loop does not silently fail; ensure any callers
of the helper adjust to the returned error if you choose to return one.
In `@pkg/itestkit/addons/metricskit/reporter.go`:
- Around line 61-70: The error breakdown loop iterates the map returned by
s.GetErrorCounts(), producing non-deterministic ordering; fix it by extracting
the map keys into a slice (from errorCounts), sort the keys (e.g., sort.Strings
for string categories), then iterate the sorted keys to append lines; update the
loop that currently ranges over errorCounts (and that uses string(category)) to
use the sorted key slice instead so the ERROR BREAKDOWN output is stable and
reproducible.
In `@pkg/itestkit/addons/queuekit/amqp.go`:
- Around line 327-339: The read of p.channel happens while holding p.mu but
PublishWithContext is called after releasing the lock, allowing Close() to race
and close/nil the channel; fix by reading both the channel and the closed state
under the mutex (e.g., lock p.mu, ch := p.channel, closed := p.closed), if
closed return a suitable error (or amqp.ErrClosed) without calling
PublishWithContext, then unlock and call ch.PublishWithContext only when
closed==false; reference p.mu, p.channel, p.closed, PublishWithContext and Close
in your change.
In `@pkg/itestkit/addons/queuekit/matcher_test.go`:
- Around line 51-79: Add a test in TestMatchHeader that verifies MatchHeader
returns false when the Message.Headers map is nil: create a case (e.g., name
"nil headers") where msg := Message{Headers: nil} (or omit Headers) and call
matcher := MatchHeader("x-type","notification") expecting false; update the
tests slice or add a separate t.Run using the same assertion pattern so the
behavior for nil Headers is explicitly covered.
In `@pkg/itestkit/addons/queuekit/matcher.go`:
- Around line 247-274: Extend compareValues to handle additional numeric
expected types (uint, uint64, int32, uint32, float32, etc.) by converting actual
float64 values into the appropriate numeric types for comparison and vice versa
where needed; update the type switch in compareValues to include cases for
uint/uint64/uint32/int32/float32 and perform safe conversions (e.g., compare
float64 actual to float32(expected) or to float64(uint(expected)) as
appropriate) so numeric expectations that arrive with those types match
JSON-decoded float64 actuals while preserving existing nil handling and default
equality fallback.
In `@pkg/itestkit/chaos_toxiproxy.go`:
- Around line 98-104: The CreateProxy method in toxiproxyChaos currently
increments t.nextPort before checking bounds, which can cause multiple
unnecessary increments under concurrency; change the allocation to a
compare-and-swap loop that atomically reads t.nextPort, verifies it is <=
proxyPortRangeEnd (or < max), and only increments via CompareAndSwap when safe
(so use t.nextPort.Load(), check against proxyPortRangeEnd/maxProxies, and then
t.nextPort.CompareAndSwap to claim the port), then set internalPort from the
claimed value and proceed with the rest of CreateProxy.
In `@pkg/itestkit/chaos.go`:
- Around line 8-27: The public API types ChaosConfig, ProxyRef and
ChaosInterface lack proper English GoDoc; update the file to add concise English
GoDoc comments above the ChaosConfig, ProxyRef type declarations and each
exported field (Enabled, Image, Name, ListenAddr, Upstream) and above the
ChaosInterface declaration and each method signature (CreateProxy, AddLatency,
AddTimeout, AddBandwidth, CutConnection, RemoveToxic, RemoveAllToxics, Close) so
generated docs are consistent and English-only; keep comments short, describe
purpose and semantics (e.g., what ListenAddr and Upstream represent, units for
durations/rates, and error behavior) and remove the Portuguese inline comments.
In `@pkg/itestkit/container_generic.go`:
- Around line 106-116: Wrap returned errors from
testcontainers.GenericContainer, cn.Host, and cn.MappedPort with contextual
messages to match the customizer error wrapping already used (e.g., include
which container and operation failed); locate the calls to GenericContainer(ctx,
gr), cn.Host(ctx), and cn.MappedPort(ctx, nat.Port(...)) and change their
returns to use fmt.Errorf (or errors.Wrap) to add context like "failed to create
generic container", "failed to get container host", and "failed to get mapped
port" while returning the wrapped error instead of the raw err.
In `@pkg/itestkit/customizer_options.go`:
- Around line 36-54: CCopyFile and CCopyDir are identical; consolidate to one
function (e.g., CCopy) or implement a shared helper to avoid duplication:
replace CCopyFile and CCopyDir with a single exported CCopy(hostPathOrDir,
containerPath string, mode int64) Customizer that constructs the same
testcontainers.ContainerFile and calls testcontainers.WithFiles, or have both
thin wrappers call a new unexported buildContainerFile(host, container, mode)
helper that returns the ContainerFile and passes it to testcontainers.WithFiles;
update or add a brief comment explaining that testcontainers.WithFiles accepts
both files and directories if you choose to keep both names.
In `@pkg/itestkit/hostport.go`:
- Around line 35-54: The external docker exec.Command calls in HostGatewayIP()
can hang; replace them with context-aware calls using context.WithTimeout and
exec.CommandContext for both the "docker info ..." and "docker network inspect
..." invocations, using a short reasonable timeout (e.g. a few seconds), ensure
you call cancel() to release resources, and treat context deadline errors like
other exec errors so HostGatewayIP still falls back gracefully; update
references to the existing hostGatewayIP logic and preserve the current behavior
on success/failure.
In `@pkg/itestkit/infra.go`:
- Around line 35-40: The duplicate-detection currently builds a string key with
key := kind + ":" + name which can collide for different (kind,name) pairs;
replace this with a structured key (e.g., define a small pair struct type or use
a nested map) and update the map declaration, lookups, and inserts to use that
structured key instead of the concatenated string; specifically change the seen
map to something like map[pair]struct{} (or map[string]map[string]struct{}) and
replace the key creation, the exists check (if _, exists := seen[key]; ...) and
the insert (seen[key] = struct{}{}) to use the new pair variable (or
seen[kind][name] = struct{}{}), ensuring the duplicate error remains unchanged.
In `@pkg/itestkit/infra/minio/minio_options.go`:
- Around line 32-40: The WithMinioFixedPort option currently binds MinIO to all
interfaces by setting HostIP to "0.0.0.0"; change the HostIP in the
nat.PortBinding inside WithMinioFixedPort (which mutates
minioOptions.hostConfigModifiers and the container.HostConfig.PortBindings for
nat.Port("9000/tcp")) to "127.0.0.1" to restrict access to loopback, and apply
the same change in the analogous function in seaweedfs_options.go so test
services default to local-only exposure.
In `@pkg/itestkit/infra/mssql/mssql_test.go`:
- Around line 22-27: The parent test creates ctx, cancel :=
context.WithTimeout(...) and defers cancel(), but subtests like the one started
with t.Run("basic mssql with query verification", ...) and the "mssql with
custom database" subtest call t.Parallel() and can outlive the parent, so move
context creation into each parallel subtest: inside each t.Run body (before
calling t.Parallel()) call ctx, cancel :=
context.WithTimeout(context.Background(), 5*time.Minute) and defer cancel(), and
update code in the subtests to use that local ctx instead of the parent's ctx;
remove reliance on the parent ctx cancel for those parallel subtests.
- Around line 251-264: The "double terminate" subtest currently reuses the
parent's ctx causing a shared-context race; inside the "double terminate should
not error" subtest create a fresh context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), 5*time.Minute) and defer cancel()) and
use that when calling infra.Terminate so the subtest has its own cancellable
timeout; locate the subtest by the string "double terminate should not error"
and the use of mssql.NewMSSQLInfra / infra.Terminate to apply the change.
In `@pkg/itestkit/infra/mssql/mssql.go`:
- Around line 180-189: The code ignores the error from strconv.Atoi when parsing
portStr (in the endpoint.ProxyListen handling block), causing an invalid port to
silently become 0; update the parsing in the same conditional (where
net.SplitHostPort is used) to check the error returned by strconv.Atoi(portStr)
and if non-nil return a descriptive error (e.g., fmt.Errorf("invalid proxy port
%q in %s: %w", portStr, endpoint.ProxyListen, err)) instead of proceeding with
portNum==0, keeping the same return types.
- Around line 132-135: Current DSN builds use fmt.Sprintf with m.cfg.Password
and don't percent-encode special characters; replace the fmt.Sprintf-based
construction of dsn so credentials are encoded via url.UserPassword (e.g.,
create a url.URL with Scheme "sqlserver", User url.UserPassword("sa",
m.cfg.Password), Host finalAddr and set the database as a query parameter when
m.cfg.Database != ""), then use url.URL.String() as the DSN; apply the same
replacement for the other DSN construction that also uses m.cfg.Password
elsewhere in this file.
In `@pkg/itestkit/infra/mysql/mysql_test.go`:
- Around line 22-27: The parent context created by ctx, cancel :=
context.WithTimeout(...) is shared with parallel subtests and may be cancelled
when the parent returns; update the "mysql with custom config" subtest to create
its own derived context and cancel function inside the t.Run closure (similar to
the existing "basic mysql with query verification" pattern) so each parallel
subtest uses its own context and calls its own cancel; locate the t.Run for
"mysql with custom config" and replace usage of the outer ctx/cancel with a new
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) (and
defer cancel) inside that subtest before calling t.Parallel.
In `@pkg/itestkit/infra/mysql/mysql.go`:
- Around line 128-136: The proxy is being created with the host-mapped upstream
address (upstream) which differs from MongoDB/Postgres implementations that use
the container network alias when available; change the CreateProxy call in the
block that checks m.cfg.EnableProxy to pass the MySQL container's network alias
(when present) instead of upstream so the proxy connects over the shared Docker
network. Locate the block using env.Chaos.CreateProxy(ctx, m.cfg.ProxyName,
upstream) and replace the upstream argument with the network alias value (the
same alias the MySQL container is reachable by on the test network), ensuring
finalAddr and proxyListen are still set from ref.ListenAddr.
- Around line 180-209: Replace the brittle strings.Split parsing and the ignored
strconv.Atoi error: for both endpoint.ProxyListen and endpoint.Upstream use
net.SplitHostPort to reliably get host and port (handles IPv6 like
"[::1]:3306"), then convert the returned port string with strconv.Atoi and
check/return any error; keep the existing m.networkAlias branch unchanged and
continue to call itestkit.NormalizeHost on the host when returning the upstream
address. Ensure error messages include the raw input and the parsing error
(e.g., fmt.Errorf("invalid proxy address %q: %w", endpoint.ProxyListen, err) or
similar) so neither parse failure nor Atoi failure is ignored.
In `@pkg/itestkit/infra/oracle/oracle_test.go`:
- Around line 331-359: The test currently only pings the DB so it never verifies
that WithOracleEnv actually applied any configuration; change the test to set a
clearly observable env (e.g. oracle.WithOracleEnv("TEST_FLAG","1")) and then
assert the container has that env set by querying the running container (use the
oracle infra helper such as infra.ContainerExec(ctx, "printenv TEST_FLAG") or
the infra method that inspects container env) and assert the output equals "1";
keep the existing NewOracleInfra/OracleConfig/WithOracleEnv usage but add the
container inspection call and an assertion instead of relying solely on
db.PingContext.
- Around line 22-26: The top-level timeout context (ctx, cancel :=
context.WithTimeout(...); defer cancel()) must be removed and instead create a
fresh timeout context inside each parallel subtest; for both t.Run blocks (e.g.,
the "basic oracle with query verification" subtest and the other parallel
subtest) call ctx, cancel := context.WithTimeout(context.Background(),
10*time.Minute) at the start of the subtest and defer cancel() there so Build(),
PingContext(), Terminate(), and QueryRowContext() run with a live context for
the duration of the subtest.
In `@pkg/itestkit/infra/oracle/oracle.go`:
- Around line 210-214: The strconv.Atoi call currently ignores its error
(portNum, _ := strconv.Atoi(portStr)); update this to check and handle the
error: call strconv.Atoi(portStr) into (portNum, err), and if err != nil return
an informative error (e.g., wrap err with context about portStr) instead of
silently returning portNum=0; ensure the function returns hostStr and the error
(consistent with the other error-checked parse earlier), referencing the
variables portStr, portNum and hostStr and the strconv.Atoi call to locate the
change.
In `@pkg/itestkit/infra/postgres/postgres_options.go`:
- Around line 37-47: The closure in WithPGInitFile mutates the captured
parameter containerFileName causing shared mutable state; fix it by creating a
new local variable (e.g., fname := containerFileName) inside the outer function
before returning the closure, use that local fname in the closure instead of
reassigning containerFileName, and then append the testcontainers.ContainerFile
to o.runOpts (ContainerFilePath: "/docker-entrypoint-initdb.d/"+fname) so the
captured value is immutable across goroutines; keep all references to
postgresOptions and runOpts unchanged otherwise.
In `@pkg/itestkit/infra/postgres/postgres_test.go`:
- Around line 19-31: In waitForDB, remove the redundant final call to
db.PingContext(ctx) by storing the last ping error inside the retry loop (e.g.,
lastErr) each time PingContext fails, and after the loop return that lastErr (or
wrap it with context, e.g., fmt.Errorf("ping failed after retries: %w",
lastErr)); keep the existing ctx.Done() handling and ensure lastErr is non-nil
when returned.
In `@pkg/itestkit/infra/postgres/postgres.go`:
- Around line 189-194: The parsing of ProxyListen's port silently ignores the
strconv.Atoi error; update the code (the branch handling ProxyListen where
portStr is converted) to check the error from strconv.Atoi(portStr) and return a
non-nil error (or wrap it with context including ProxyListen/portStr) instead of
returning portNum==0; ensure the function that currently returns hostStr,
portNum, nil returns the parsing error when strconv.Atoi fails so callers get a
clear failure instead of a zero port.
In `@pkg/itestkit/infra/rabbitmq/rabbitmq_test.go`:
- Around line 354-360: When ExchangeDeclarePassive in the test loop (calling
ch.ExchangeDeclarePassive) fails the channel is closed and the code correctly
recreates it with ch, _ = conn.Channel(); however the channel creation error is
ignored—change the recovery to capture and handle the returned error from
conn.Channel(), e.g. replace ch, _ = conn.Channel() with ch, err =
conn.Channel() and fail the test or t.Fatalf/assert with a clear message if err
!= nil; also include the original ExchangeDeclarePassive error in the test
failure message produced by t.Errorf/t.Fatalf so both the declare error and any
channel-recreation error are visible.
In `@pkg/itestkit/infra/rabbitmq/rabbitmq.go`:
- Around line 174-184: The proxy branch currently discards the error from
strconv.Atoi(portStr) which can hide invalid proxy ports; update the ProxyListen
handling (the block that checks endpoint.ProxyListen) to check and return the
strconv.Atoi error instead of ignoring it—i.e., parse portStr into portNum and
if strconv.Atoi returns an error, wrap and return it (similar to the later code
path that handles Atoi errors) so invalid proxy addresses produce a clear error
rather than silently using zero.
In `@pkg/itestkit/infra/redis/redis_test.go`:
- Around line 307-332: The test currently only pings the container so it never
verifies the injected RedisOption took effect; update the test after creating
client to query the Redis configuration (e.g., run CONFIG GET maxmemory via
client.ConfigGet or client.Do(ctx, "CONFIG", "GET", "maxmemory") using the
goredis client) and assert the returned value matches the injected "--maxmemory
50mb" (or convert to bytes), or alternatively replace the option with one that
has an observable runtime effect and assert that effect; reference
redis.NewRedisInfra, redis.WithRedisEnv, infra.Addr(), and the goredis client
when making the change.
- Around line 21-25: The current parent-level ctx, cancel created with
context.WithTimeout is canceled by defer cancel() before parallel subtests run
(because t.Run + t.Parallel causes the parent to return), so move the context
creation inside each parallel subtest (create ctx, cancel with
context.WithTimeout at the top of each t.Run subtest and defer cancel() or call
t.Cleanup(cancel) within that subtest) or alternatively replace the parent defer
cancel() with t.Cleanup(cancel) so cancellation is delayed until all subtests
complete; update usages of ctx in Build/SET/GET/PING to use the per-subtest ctx
and keep references to the symbols context.WithTimeout, cancel, t.Run,
t.Parallel, and t.Cleanup to locate the changes.
In `@pkg/itestkit/infra/seaweedfs/seaweedfs.go`:
- Around line 87-172: The created Docker network (nw / s.network / networkName)
is not removed on container startup failures, causing orphaned networks; update
the error paths after each testcontainers.GenericContainer call (for master,
volume, and filer) so that before returning on error you remove the network
(call the network's Remove/Close method on nw or s.network with ctx), and when
you already terminate created containers (master/volume) also ensure you remove
the network afterward (handling/remove any returned error). Ensure s.network is
set only after successful creation or that you call Remove on nw directly in the
failure branches.
In `@pkg/itestkit/README.md`:
- Around line 499-503: The examples call pgInfra.DSN() and rabbit.AMQPURL()
directly inside WithEnvVar but those functions return (string, error); update
the examples to first call DSN() and AMQPURL(), check or handle the returned
error (e.g., assign to variables dsn, amqp and ignore or handle the error), then
pass the extracted string into WithEnvVar; locate usages around the example
block and the second occurrence (lines referenced) and replace direct calls with
temporary variables before calling WithEnvVar.
In `@pkg/itestkit/suite.go`:
- Around line 125-139: Suite.Terminate currently discards all errors and always
returns nil; change it to capture and return diagnostics by collecting errors
from s.infra[i].Terminate(ctx), s.chaos.Close(ctx), and s.network.Remove(ctx)
(or at least return the first non-nil error). Update the loop over s.infra, the
s.chaos.Close call, and the s.network.Remove call to check returned errors and
propagate them (or aggregate them into a single error) instead of ignoring them,
so callers receive termination failure information.
In `@pkg/pdf/pool.go`:
- Line 105: The chromedp.Flag("no-sandbox", true) line disables Chrome's sandbox
and must be explicitly documented: add an inline comment above this Flag call
explaining it's required for some containerized environments (sandbox/container
security model conflicts) and note the increased attack surface; update
repository/docs or a SECURITY.md entry with the risk assessment or security
review reference; and make the behavior conditional by reading an environment
variable (e.g., CHROME_NO_SANDBOX) where the code that constructs the Chrome
options (the place that calls chromedp.Flag("no-sandbox", true) in pool.go) only
sets the flag when that env var is true or when running in known container
deployments so different environments can opt into or out of no-sandbox.
In `@tests/e2e/infra_datasources_test.go`:
- Around line 167-171: The test currently silently skips the assertion when
json.Unmarshal(resp.Body(), &body) fails, which masks failures; change the block
so that if json.Unmarshal returns an error you fail the test (e.g., t.Fatalf or
t.Errorf+t.FailNow) and include the raw resp.Body() and the unmarshal error in
the message, otherwise call shared.AssertErrorCode(t, body, "TPL-0031"); ensure
you reference the same variables/functions (json.Unmarshal, resp.Body(), body,
shared.AssertErrorCode) so the assertion never gets skipped silently.
In `@tests/e2e/infra_health_test.go`:
- Around line 156-160: Replace the fixed-size buffer read in the health test
(the resp.Body.Read(buf[:]) usage) with io.ReadAll to handle variable-length
responses: call io.ReadAll(resp.Body), check and fail the test on any error, and
assert that the returned byte slice length is > 0; also ensure io is imported
and resp.Body is closed (defer resp.Body.Close()) if not already.
In `@tests/e2e/infra_security_test.go`:
- Around line 156-185: Replace the unconditional t.Skip in
TestSec_RateLimitGlobal with a conditional skip that reads the
RATE_LIMIT_ENABLED environment flag (e.g., via os.Getenv or strconv.ParseBool)
and only calls t.Skip when rate limiting is disabled; keep the rest of the test
(the loop using rateLimitGlobal and requests to
env.ManagerBaseURL+"/v1/templates") unchanged so the test runs in environments
where RATE_LIMIT_ENABLED is true.
In `@tests/e2e/README.md`:
- Around line 78-82: Update the README prerequisite that currently reads "Go
1.25+" to match the project's go.mod by changing that string to "Go 1.26+" so
the prerequisites reflect the required Go version.
In `@tests/e2e/report_filters_test.go`:
- Around line 274-325: The current in/nin tests only assert absence of
disallowed statuses which can pass on empty results; after you call
apiClient.DownloadReport and set content := string(data) in both the inclusion
test (where filters := shared.MakeFilters(..., shared.FilterIn(...))) and
TestFilter_NinExclusion, add a positive assertion that the report is non-empty
and contains a known allowed organization (e.g., assert.Contains(t, content,
"AllowedOrgName") or require.NotEmpty(t, content) and assert.Contains for a
fixture org), or otherwise verify at least one expected row is present using the
reportID/download flow to ensure the filter actually returned data. Ensure the
checks reference the existing variables reportID, apiClient.DownloadReport, and
content.
- Around line 89-263: The tests TestFilter_GtDate, TestFilter_GteDate,
TestFilter_LtDate, TestFilter_LteDate, TestFilter_BetweenDates and
TestFilter_BetweenDateRange currently only assert NotEmpty or HTML output, which
doesn't verify the date predicates; update each test (the functions named above
that call shared.MakeFilters with shared.FilterGt/Gte/Lt/Lte/FilterBetween and
use apiClient.CreateReport + apiClient.DownloadReport) to assert specific
fixture rows are included or excluded: after downloading report data convert
bytes to string and assert that known organization identifiers (IDs, names or
created_at dates from the test fixtures) expected to match the predicate are
present and those that should be filtered out are absent. Use the same report
creation flow (shared.AssertReportCompleted, apiClient.DownloadReport) but
replace the generic NotEmpty/HTML checks with assert.Contains/assert.NotContains
on the report content for the exact fixture rows relevant to each date
predicate.
- Around line 31-85: The tests TestFilter_EqSingleValue and
TestFilter_EqMultipleValues only assert presence of expected orgs and will pass
if filters are ignored; update each test (the CreateReport + DownloadReport flow
using shared.MakeFilters and apiClient.CreateReport) to also assert exclusion or
exact row count: after downloading the HTML (where shared.AssertHTMLContent or
content := string(data) is used) add an assertion that a known non-matching
organization (e.g., a different fixture org like "Gamma LLC" or any org that
should be excluded) is NOT present, or parse/count the rendered table rows and
assert the count equals the expected number for the filter; this ensures eq
semantics and multi-value OR behavior are actually validated.
- Around line 28-49: The test currently uses a single ctx with
shared.DefaultPollTimeout for all steps; split that into separate contexts so
polling and download get extra time. Keep using a short context (e.g.,
context.WithTimeout(context.Background(), shared.DefaultPollTimeout)) for
template creation and apiClient.CreateReport, then create a new pollCtx
(context.WithTimeout(context.Background(),
shared.DefaultPollTimeout+shared.PDFExtraTimeout)) and pass pollCtx to
shared.AssertReportCompleted and to apiClient.DownloadReport (or create a
downloadCtx with the same extended timeout) so that createTemplateForFormat,
apiClient.CreateReport, shared.AssertReportCompleted, and
apiClient.DownloadReport each use appropriate contexts rather than a single
shared one.
In `@tests/e2e/report_management_test.go`:
- Around line 64-190: Several nearly identical tests (TestReport_CreateHTML,
TestReport_CreateCSV, TestReport_CreateXML, TestReport_CreatePDF,
TestReport_CreateTXT) repeat the same logic; convert them into a single
table-driven test (e.g., TestReport_CreateFormats) that defines a cases slice
with name, fixture and format, iterates cases with tc := tc and t.Run(tc.name,
func(t *testing.T){ t.Parallel(); ... }), and inside reuse createTestTemplate,
shared.CreateReportRequest, apiClient.CreateReport and the same assertions
(shared.AssertHTTPStatus, shared.AssertValidUUID, shared.AssertJSONField) to
remove duplication and make future formats easy to add.
In `@tests/e2e/resilience_error_retry_test.go`:
- Around line 53-83: The test names TestErr_DLQNonRetryable and
TestErr_DLQMaxRetries are misleading because both only assert template creation
is rejected with HTTP 400; rename the tests (or update their descriptions) to
reflect the actual behavior (for example
TestErr_InvalidDatabaseRejectedAtCreation and
TestErr_InvalidTableRejectedAtCreation) or add a brief comment inside each test
clarifying they are placeholders until DLQ/chaos infrastructure is available;
update any test registration references or comments that mention DLQ to match
the new names to keep intent clear.
- Around line 89-106: The test TestErr_CircuitBreakerOpens does not exercise
circuit-breaker behavior because it only triggers 400 validation errors; either
rename the function (e.g., TestErr_TemplateValidationConsistency) and update any
references to reflect that it validates consistent template rejection, or modify
the test to actually exercise circuit-breaker semantics (simulate repeated
server-side failures and assert fast-fail behavior after N attempts).
Specifically, change the test name TestErr_CircuitBreakerOpens and/or add a
clear comment above the loop explaining this is only validating API rejection
(referencing tplBytes, attemptCount, and the CreateTemplateRaw calls), or
replace the loop to induce server errors and assert the circuit-breaker response
sequence.
In `@tests/e2e/shared/assertions.go`:
- Around line 199-205: The AssertValidUUID function currently uses a regex that
matches any UUID version; update it to validate UUID v4 specifically by changing
the pattern in assert.Regexp to enforce the third group starts with "4" and the
fourth group starts with one of [89ab] (e.g.
`^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$`), and
update the assertion message to reflect "should be valid UUID v4" (or
alternatively, if you prefer to accept all versions, change the function comment
to "valid UUID format" instead); refer to AssertValidUUID to locate where to
apply this change.
- Around line 262-273: Replace the fragile string equality check for EOF in the
token parsing loop: when reading tokens from decoder.Token() detect EOF using
errors.Is(err, io.EOF) instead of err.Error() == "EOF"; add the "errors" and
"io" imports, and keep the existing t.Fatalf for non-EOF errors so the loop
returns on EOF and fails on other parsing errors.
In `@tests/e2e/shared/client.go`:
- Around line 524-537: The WorkerHealth function currently allocates a new
resty.Client on every call; change it to reuse a client by either (A) adding a
parameter like WorkerHealth(ctx context.Context, client *resty.Client,
workerBaseURL string) and use the provided client (calling SetBaseURL/SetTimeout
once at setup), or (B) implement a package-level cached client (e.g., var client
*resty.Client with sync.Once initialization) that configures BaseURL and Timeout
once and is reused on subsequent calls; update all call sites/tests that invoke
WorkerHealth to pass or initialize the reusable client accordingly and ensure
context is still passed to the request.
- Around line 311-326: CreateReportWithIdempotency currently omits parsing error
responses; mirror CreateReport by declaring an errResult (map[string]any) and
calling SetError(&errResult) on the request built in
CreateReportWithIdempotency, then after the Post call return resp.StatusCode()
and errResult when the response is an error (4xx/5xx) so callers receive the
parsed error body instead of nil; update references inside
CreateReportWithIdempotency to use the same error-handling pattern as
CreateReport.
- Around line 386-392: The code creates a new http.Header named headers and
copies values from resp.Header(), which is redundant because resp.Header()
already returns an http.Header; instead remove the headers map and the loop and
return resp.Header() directly where the function currently returns headers
(locate the return that uses headers in the function handling resp.StatusCode(),
resp.Body(), headers, nil). Ensure no other callers expect a distinct copy—if a
defensive copy is required, use resp.Header().Clone() instead.
In `@tests/e2e/shared/infra.go`:
- Line 103: Remove the sensitive S3 AccessKeyID from the log in
tests/e2e/shared/infra.go: update the log.Printf call (the one referencing
appEnv.S3Endpoint, appEnv.S3Bucket, appEnv.S3AccessKeyID) to either omit
appEnv.S3AccessKeyID entirely or replace it with a masked value (e.g.,
"<redacted>" or show only last 4 chars) so credentials are not emitted; keep
logging of non-sensitive fields (appEnv.S3Endpoint, appEnv.S3Bucket) and ensure
any future use of log.Printf here does not reference appEnv.S3AccessKeyID.
In `@tests/e2e/template_crud_test.go`:
- Around line 322-386: Update the three tests TestTemplate_CreateInvalidField,
TestTemplate_CreateInvalidDatabase, and TestTemplate_CreateInvalidTable to
include brief inline comments above the assertions that accept multiple error
codes explaining which domain scenarios map to each accepted code (e.g., why
invalid field may yield "TPL-0014" vs "TPL-0008", invalid database may yield
"TPL-0031" vs "TPL-0038", invalid table may yield "TPL-0030" vs "TPL-0037");
keep the existing assert logic unchanged but add these explanatory comments near
the code variables "code" and the assert.True checks so future reviewers know
which condition produces which code and won't mistake the permissiveness for a
test bug.
- Around line 912-929: TestTemplate_UpdateNotFound documents that UpdateTemplate
currently yields 500 because TemplateRepo.FindByID returns raw
mongo.ErrNoDocuments; create an issue to track fixing the service error handling
so UpdateTemplate returns 404: update the code path that calls
TemplateRepo.FindByID (the uploadTemplateFileToStorage / UpdateTemplate flow) to
detect mongo.ErrNoDocuments and wrap/translate it into a domain NotFound error
(or a custom ErrTemplateNotFound) that the HTTP error mapper uses to return
http.StatusNotFound, and reference TestTemplate_UpdateNotFound, UpdateTemplate,
TemplateRepo.FindByID, and mongo.ErrNoDocuments in the issue for context.
---
Outside diff comments:
In `@components/manager/internal/services/get-data-source-information.go`:
- Around line 45-64: Replace the hard-coded datasource type strings in the
switch over dataSource.DatabaseType with the shared constants to keep behavior
consistent: use pkg.PostgreSQLType instead of "postgresql" and pkg.MongoDBType
instead of "mongodb" in the switch cases for the function handling
dataSource.DatabaseType (the switch that creates model.DataSourceInformation),
and add the corresponding import for the pkg package if not already present;
ensure the case labels match the existing constants' names exactly and run
tests/build to verify no import name collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4275e244-b5aa-4347-bee9-9659944cedcd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (112)
Makefilecomponents/manager/internal/adapters/http/in/data-source_test.gocomponents/manager/internal/adapters/http/in/middlewares.gocomponents/manager/internal/bootstrap/config.gocomponents/manager/internal/bootstrap/init_helpers.gocomponents/manager/internal/services/create-template_test.gocomponents/manager/internal/services/get-data-source-details-by-id.gocomponents/manager/internal/services/get-data-source-details-by-id_test.gocomponents/manager/internal/services/get-data-source-information.gocomponents/manager/internal/services/mapped-fields-validation.gocomponents/manager/internal/services/mapped-fields-validation_test.gocomponents/manager/internal/services/update-template-by-id_test.gocomponents/worker/Dockerfilego.modmk/tests.mkpkg/datasource-config_test.gopkg/itestkit/CONTRIBUTING.mdpkg/itestkit/README.mdpkg/itestkit/addons/e2ekit/build_secrets.gopkg/itestkit/addons/e2ekit/builder.gopkg/itestkit/addons/e2ekit/helpers.gopkg/itestkit/addons/metricskit/assertions.gopkg/itestkit/addons/metricskit/error_classifier.gopkg/itestkit/addons/metricskit/interfaces.gopkg/itestkit/addons/metricskit/metrics.gopkg/itestkit/addons/metricskit/reporter.gopkg/itestkit/addons/queuekit/amqp.gopkg/itestkit/addons/queuekit/assertions.gopkg/itestkit/addons/queuekit/consumer.gopkg/itestkit/addons/queuekit/consumer_test.gopkg/itestkit/addons/queuekit/matcher.gopkg/itestkit/addons/queuekit/matcher_test.gopkg/itestkit/addons/queuekit/queuekit.gopkg/itestkit/chaos.gopkg/itestkit/chaos_toxiproxy.gopkg/itestkit/container_generic.gopkg/itestkit/customizer.gopkg/itestkit/customizer_options.gopkg/itestkit/hostport.gopkg/itestkit/infra.gopkg/itestkit/infra/minio/minio.gopkg/itestkit/infra/minio/minio_options.gopkg/itestkit/infra/mongodb/mongodb.gopkg/itestkit/infra/mongodb/mongodb_options.gopkg/itestkit/infra/mongodb/mongodb_test.gopkg/itestkit/infra/mssql/mssql.gopkg/itestkit/infra/mssql/mssql_options.gopkg/itestkit/infra/mssql/mssql_test.gopkg/itestkit/infra/mysql/mysql.gopkg/itestkit/infra/mysql/mysql_options.gopkg/itestkit/infra/mysql/mysql_test.gopkg/itestkit/infra/oracle/oracle.gopkg/itestkit/infra/oracle/oracle_options.gopkg/itestkit/infra/oracle/oracle_test.gopkg/itestkit/infra/postgres/postgres.gopkg/itestkit/infra/postgres/postgres_options.gopkg/itestkit/infra/postgres/postgres_test.gopkg/itestkit/infra/rabbitmq/rabbit_options.gopkg/itestkit/infra/rabbitmq/rabbitmq.gopkg/itestkit/infra/rabbitmq/rabbitmq_test.gopkg/itestkit/infra/rabbitmq/testdata/definitions.jsonpkg/itestkit/infra/redis/redis.gopkg/itestkit/infra/redis/redis_options.gopkg/itestkit/infra/redis/redis_test.gopkg/itestkit/infra/seaweedfs/seaweedfs.gopkg/itestkit/infra/seaweedfs/seaweedfs_options.gopkg/itestkit/suite.gopkg/pdf/pool.gopkg/safe-datasources.gotests/e2e/README.mdtests/e2e/infra_datasources_test.gotests/e2e/infra_health_test.gotests/e2e/infra_security_test.gotests/e2e/main_test.gotests/e2e/report_filters_test.gotests/e2e/report_generation_test.gotests/e2e/report_management_test.gotests/e2e/resilience_error_retry_test.gotests/e2e/shared/apps.gotests/e2e/shared/assertions.gotests/e2e/shared/client.gotests/e2e/shared/constants.gotests/e2e/shared/helpers.gotests/e2e/shared/infra.gotests/e2e/template_crud_test.gotests/e2e/template_report_validation_test.gotests/e2e/testdata/init_mongo.jstests/e2e/testdata/init_postgres.sqltests/e2e/testdata/templates/ACCS005.tpltests/e2e/testdata/templates/account-status_txt.tpltests/e2e/testdata/templates/account_pdf.tpltests/e2e/testdata/templates/balance-list_csv.tpltests/e2e/testdata/templates/cadoc-4111.tpltests/e2e/testdata/templates/csv-content-as-html.tpltests/e2e/testdata/templates/empty.tpltests/e2e/testdata/templates/engine-features-showcase_html.tpltests/e2e/testdata/templates/event-handler-injection_html.tpltests/e2e/testdata/templates/invalid-database_html.tpltests/e2e/testdata/templates/invalid-field_html.tpltests/e2e/testdata/templates/invalid-table_html.tpltests/e2e/testdata/templates/list-accounts_html.tpltests/e2e/testdata/templates/multi-source_html.tpltests/e2e/testdata/templates/not-tpl.txttests/e2e/testdata/templates/schema-qualified_html.tpltests/e2e/testdata/templates/script-injection_html.tpltests/e2e/testdata/templates/valid_csv.tpltests/e2e/testdata/templates/valid_html.tpltests/e2e/testdata/templates/valid_pdf.tpltests/e2e/testdata/templates/valid_txt.tpltests/e2e/testdata/templates/valid_xml.tpltests/e2e/validation_query_params_test.gotests/integration/notification-integration_test.go
💤 Files with no reviewable changes (7)
- components/manager/internal/adapters/http/in/data-source_test.go
- components/manager/internal/services/create-template_test.go
- components/manager/internal/services/mapped-fields-validation.go
- components/manager/internal/services/get-data-source-details-by-id.go
- components/manager/internal/services/update-template-by-id_test.go
- components/manager/internal/services/mapped-fields-validation_test.go
- components/manager/internal/services/get-data-source-details-by-id_test.go
- fix(mysql): use network alias for proxy upstream and net.SplitHostPort for IPv6 compatibility - fix(seaweedfs): clean up Docker network on container startup failure - fix(queuekit): hold mutex through PublishWithContext to prevent race - fix(assertions): use errors.Is(err, io.EOF) instead of string comparison - fix(e2e): require JSON unmarshal success in datasource test - fix(client): add SetError handling to CreateReportWithIdempotency - fix(infra): remove S3 access key from log output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
Contributor
|
This PR is very large (113 files, 20850 lines changed). Consider breaking it into smaller PRs for easier review. |
Fixes CRITICAL authorization bypass vulnerability where gRPC-Go accepted requests with missing leading slash in :path pseudo-header, allowing authz policy bypass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
Contributor
|
This PR is very large (113 files, 20856 lines changed). Consider breaking it into smaller PRs for easier review. |
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (5)
pkg/itestkit/infra/seaweedfs/seaweedfs.go (1)
78-118:⚠️ Potential issue | 🟠 MajorRollback partially started resources on every
Startfailure.Line 118, Line 182, Line 187, Line 202, and Line 212 can still return with the network and/or started containers alive.
Startshould not depend on the caller remembering to tear down a partially initialized instance.Suggested fix
-func (s *SeaweedFSInfra) Start(ctx context.Context, env *itestkit.Env) error { +func (s *SeaweedFSInfra) Start(ctx context.Context, env *itestkit.Env) (err error) { opts := defaultSeaweedFSOptions() ... nw, err := network.New(ctx, network.WithDriver("bridge")) if err != nil { return fmt.Errorf("create network: %w", err) } s.network = nw + defer func() { + if err == nil { + return + } + if s.filer != nil { + _ = s.filer.Terminate(ctx) + s.filer = nil + } + if s.volume != nil { + _ = s.volume.Terminate(ctx) + s.volume = nil + } + if s.master != nil { + _ = s.master.Terminate(ctx) + s.master = nil + } + if s.network != nil { + _ = s.network.Remove(ctx) + s.network = nil + } + }() networkName := nw.Name ... return nil }Also applies to: 180-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/itestkit/infra/seaweedfs/seaweedfs.go` around lines 78 - 118, The Start method (SeaweedFSInfra.Start) can leave the network and containers (master, volume, filer) running on error; update Start to track created resources and always clean them up on any failure: after creating the network (network.New) and each container (testcontainers.GenericContainer returning master/volume/filer), record the resource in a local slice or variables and on any subsequent error call Terminate/Remove for started containers and remove the network (s.network) before returning the error; ensure any early returns (after master, after volume, after filer, and before final success) perform this rollback so no partial resources remain.pkg/itestkit/infra/mysql/mysql.go (2)
187-196:⚠️ Potential issue | 🟡 MinorCheck the proxy port parse error.
Line 194 still discards
strconv.Atoi’s error. A malformedProxyListensilently turns into port0here instead of failing fast, even though the upstream branch below already treats bad ports as errors.Atoiexplicitly returns(int, error). (pkg.go.dev)🔧 Proposed fix
- portNum, _ := strconv.Atoi(proxyPort) + portNum, parseErr := strconv.Atoi(proxyPort) + if parseErr != nil { + return "", 0, fmt.Errorf("invalid proxy port %q: %w", proxyPort, parseErr) + } return proxyHost, portNum, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/itestkit/infra/mysql/mysql.go` around lines 187 - 196, The code handling endpoint.ProxyListen currently ignores strconv.Atoi's error, which can yield a silent port 0; update the proxy branch (where net.SplitHostPort(endpoint.ProxyListen) is used) to capture the (portNum, err := strconv.Atoi(proxyPort)) return and if err != nil return "", 0, fmt.Errorf("invalid proxy port %q from %q: %w", proxyPort, endpoint.ProxyListen, err) so the function fails fast on malformed ports instead of returning port 0.
232-237:⚠️ Potential issue | 🟠 MajorUse
m.cfgandnet.JoinHostPortfor consistent stub endpoint creation.
NewMySQLInfra(cfg)normalizes empty username/password/database fields and stores them inm.cfg, but line 237 still uses the originalcfg. CallingNewMySQLInfraStub(MySQLConfig{}, "localhost", 3306)produces an endpoint with an empty-credential DSN whilem.cfgreports the normalized defaults everywhere else, creating an inconsistent state. Additionally, line 234's manual string concatenation breaks IPv6 addresses (e.g.,::1:3306instead of[::1]:3306); usenet.JoinHostPortinstead.🧩 Proposed fix
func NewMySQLInfraStub(cfg MySQLConfig, host string, port int) *MySQLInfra { m := NewMySQLInfra(cfg) - upstream := fmt.Sprintf("%s:%d", host, port) + upstream := net.JoinHostPort(host, strconv.Itoa(port)) m.endpoint = &MySQLEndpoint{ Upstream: upstream, - DSN: fmt.Sprintf("%s:%s@tcp(%s)/%s?parseTime=true", cfg.Username, cfg.Password, upstream, cfg.Database), + DSN: fmt.Sprintf("%s:%s@tcp(%s)/%s?parseTime=true", m.cfg.Username, m.cfg.Password, upstream, m.cfg.Database), } // Store the raw host for HostPort() to return without normalization m.stubHost = host🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/itestkit/infra/mysql/mysql.go` around lines 232 - 237, NewMySQLInfraStub creates the endpoint using the original cfg and a manual host:port concat which leads to inconsistent credentials and broken IPv6 formatting; update NewMySQLInfraStub to build upstream with net.JoinHostPort(host, strconv.Itoa(port)) and construct the DSN using m.cfg (e.g., m.cfg.Username, m.cfg.Password, m.cfg.Database) so the created MySQLEndpoint (endpoint.Upstream and endpoint.DSN) matches the normalized values stored on the MySQLInfra instance.tests/e2e/shared/assertions.go (1)
201-206:⚠️ Potential issue | 🟡 Minor
AssertValidUUIDstill doesn't match the v4 contract.The doc/comment says v4, but the regex on Line 206 accepts any UUID version. Either tighten the pattern to v4 or update the helper text to say it validates a generic UUID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/shared/assertions.go` around lines 201 - 206, The AssertValidUUID helper currently allows any UUID version; update it to enforce UUIDv4 by tightening the regex in AssertValidUUID to require a '4' as the first hex of the 3rd group and [89ab] (case-insensitive or lowercase) as the first hex of the 4th group, and update the function comment and assertion message to state "valid UUID v4"; alternatively, if you intend to accept any version, change the comment and assertion message to "valid UUID" to match the existing regex (modify either the regex in AssertValidUUID or the text consistently).pkg/itestkit/addons/queuekit/amqp.go (1)
149-157:⚠️ Potential issue | 🔴 CriticalGuard
Consumeagainst concurrentClose().
connect()returns before this lock is taken. A concurrentClose()can nilc.channelin between, so Line 157 ends up dereferencing a nil channel pointer instead of returning a clean error. This is the same race pattern that was already fixed on the publisher side.🔒 Minimal guard
c.mu.Lock() - ch := c.channel + if c.closed || c.channel == nil { + c.mu.Unlock() + return nil, fmt.Errorf("consumer is closed") + } + ch := c.channel c.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/itestkit/addons/queuekit/amqp.go` around lines 149 - 157, The call to ch.Consume can dereference c.channel after a concurrent Close() nils it because connect() returns before the mu lock; fix by protecting access to c.channel with c.mu: inside the critical section grab a local variable (e.g. ch := c.channel), if ch == nil return a clear error, then release the lock and call ch.Consume using that local. Update the code paths around connect(), Close(), and the Consume invocation to use this pattern (reference symbols: connect(), Close(), c.mu, c.channel, ch.Consume) so the nil-pointer race is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/itestkit/addons/queuekit/amqp.go`:
- Around line 157-165: The consumer currently calls ch.Consume with an empty
consumer tag and AutoAck=false while converting amqp.Delivery into a Message
without exposing Ack/Nack, and it never calls ch.Cancel on context cancellation;
fix by generating and using a persistent consumer tag when calling ch.Consume
(store it on the consumer struct), and on ctx.Done() call ch.Cancel(consumerTag,
false) to gracefully cancel the server consumer; also change the Message
conversion code to retain the original amqp.Delivery (or add Ack/Nack methods on
the Message type that delegate to the underlying amqp.Delivery) so callers can
manually Ack/Nack when AutoAck is false, or alternatively set AutoAck=true if
you intend to avoid manual acknowledgements. Ensure Close() still closes the
channel but do not rely on it for context-based cancellation.
- Around line 29-30: The current Exclusive bool on the AMQP options struct is
being used for both queue-level and consumer-level exclusivity; add two distinct
fields (e.g., QueueExclusive and ConsumerExclusive) to the struct that currently
defines Exclusive, update any builder/option helpers (e.g., WithExclusive) into
two separate helpers (e.g., WithQueueExclusive and WithConsumerExclusive or
adjust WithExclusive to accept a target), and change the calls that perform
QueueDeclare and Consume to pass the proper flag (use QueueExclusive for
QueueDeclare and ConsumerExclusive for Consume); also search and update all
usages referenced around the impacted areas (the struct definition where
Exclusive exists plus the logic in QueueDeclare/Consume and the option
application sites noted in the review) to ensure no callers are left using the
old combined Exclusive field.
In `@pkg/itestkit/infra/mysql/mysql.go`:
- Around line 15-22: MySQLConfig exposes RootPassword but it is never used by
Start/NewMySQLInfra so it is effectively a no-op; either remove the field or
thread it into the container startup path. Update NewMySQLInfra/Start to stop
overriding cfg.RootPassword with a hardcoded default and instead pass
cfg.RootPassword into whatever environment, args or initialization routine
creates the MySQL container (the code paths referenced around NewMySQLInfra and
Start), or remove RootPassword from MySQLConfig and callers that set it; ensure
all places that currently default/set the root password (the blocks shown near
lines 62-64 and 89-94 in the diff) honor cfg.RootPassword or are deleted to keep
the config surface consistent.
- Around line 107-143: After successfully creating the container with
tcmysql.Run (variable c) you must ensure the container is terminated if any
subsequent steps fail (c.Host, c.MappedPort, env.Chaos.CreateProxy,
etc.)—current returns after line 107 can leak a running container. Fix by
setting m.container = c then adding cleanup logic: either use a deferred cleanup
tied to a success flag (set success = true only at the end) or call
c.Terminate(ctx)/m.container.Terminate(ctx) before each error return in the
subsequent steps (c.Host, c.MappedPort, and env.Chaos.CreateProxy paths),
ensuring the container is always stopped on partial Start failures.
- Around line 124-149: The Upstream field is built by concatenating host and
port with fmt.Sprintf which breaks for IPv6 literals (e.g., "::1:3306"); update
the construction of upstream so it uses net.JoinHostPort(host, port.Port()) to
ensure IPv6 addresses are properly bracketed, and ensure the DSN in
MySQLEndpoint (fields Upstream, ProxyListen, DSN) uses the corrected
upstream/finalAddr so HostPort() and net.SplitHostPort calls succeed.
In `@pkg/itestkit/infra/seaweedfs/seaweedfs.go`:
- Around line 18-20: Replace the floating "latest" default for SeaweedFS with a
concrete, tested image tag by updating the defaultImage constant to a pinned tag
(e.g., a specific version) so tests are reproducible; keep the existing override
via cfg.Image intact and update any accompanying comment to note the pinned
default and rationale. Ensure the change targets the defaultImage constant in
pkg/itestkit/infra/seaweedfs/seaweedfs.go and does not alter the logic that uses
cfg.Image to override the default.
- Around line 190-210: The upstream/proxy address formatting uses
fmt.Sprintf("%s:%s") which breaks on IPv6 literals; replace those constructions
by using net.JoinHostPort when building upstream and proxyUpstream so IPv6
addresses are bracketed correctly (update the variables upstream and
proxyUpstream), keep finalAddr and proxyListen assignment logic the same, and
continue parsing with net.SplitHostPort as before; specifically modify the code
paths around upstream, proxyUpstream, and where
s.cfg.EnableProxy/env.Chaos.CreateProxy/ref.ListenAddr are used to construct
finalAddr so JoinHostPort is used instead of fmt.Sprintf.
In `@tests/e2e/infra_datasources_test.go`:
- Around line 226-227: The test currently only compares len(details1.Tables) and
len(details2.Tables) which can miss differences in table names/fields; modify
the assertion to compare the actual schema payloads by normalizing order first:
sort details1.Tables and details2.Tables (and each table's Columns/Fields) by a
stable key (e.g., table.Name and field.Name) using sort.Slice/SliceStable, then
assert.Equal on the normalized details (details1 vs details2) so the test
validates cached schema contents, not just counts. Reference the
variables/details structs used in the test (details1, details2, Tables) when
implementing the sort-and-compare.
- Around line 101-103: The test is incorrectly skipping on a 500 response for
plugin_crm which masks a regression (PluginCRMMongo is always seeded in
tests/e2e/shared/infra.go); replace the unconditional t.Skip triggered by
checking status == http.StatusInternalServerError so the test fails on 500
(remove or change the t.Skip call in tests/e2e/infra_datasources_test.go), and
if this datasource truly must be optional make skipping conditional on an
explicit environment flag (e.g., SKIP_OPTIONAL_PLUGIN_CRM) rather than the
response status; ensure you reference the status variable and t.Skip in the same
function when implementing the change.
In `@tests/e2e/shared/assertions.go`:
- Around line 259-269: AssertXMLContent currently treats empty byte slices as
valid because xml.Decoder.Token returns io.EOF immediately; before creating the
decoder in AssertXMLContent, trim whitespace from data (e.g.,
bytes.TrimSpace(data)) and if the result is zero-length call t.Fatalf (or
t.Errorf) with a clear message rejecting empty or whitespace-only XML payloads,
then proceed to create decoder := xml.NewDecoder(bytes.NewReader(data)) and the
existing Token loop.
In `@tests/e2e/shared/client.go`:
- Around line 175-181: The typed client methods are directly concatenating id
into request paths (e.g., ManagerClient.GetTemplate) which breaks for IDs with
reserved URL characters; update each typed helper that builds a path by
appending id to call url.PathEscape(id) before concatenation (and add the
net/url import), leaving the existing Raw methods unchanged so negative tests
still work; ensure you update all similar methods referenced in the review (the
GetTemplate method and the other typed helpers that append id into the path) to
use url.PathEscape instead of raw string concatenation.
In `@tests/e2e/shared/infra.go`:
- Around line 373-378: The test only seeds the organization-specific collection
(orgCollection / collection using CollectionHolders + "_" + PluginCRMMidazOrgID)
but never seeds the base CollectionHolders which other tests ASSERT against;
update the seeding logic in tests/e2e/shared/infra.go so it inserts the same
documents (or appropriate fixtures) into both db.Collection(CollectionHolders)
and db.Collection(CollectionHolders + "_" + PluginCRMMidazOrgID) — ensure you
reference CollectionHolders and PluginCRMMidazOrgID when creating the two
collection handles and perform identical seed operations for each (also apply
the same fix to the analogous block around lines 428-430).
---
Duplicate comments:
In `@pkg/itestkit/addons/queuekit/amqp.go`:
- Around line 149-157: The call to ch.Consume can dereference c.channel after a
concurrent Close() nils it because connect() returns before the mu lock; fix by
protecting access to c.channel with c.mu: inside the critical section grab a
local variable (e.g. ch := c.channel), if ch == nil return a clear error, then
release the lock and call ch.Consume using that local. Update the code paths
around connect(), Close(), and the Consume invocation to use this pattern
(reference symbols: connect(), Close(), c.mu, c.channel, ch.Consume) so the
nil-pointer race is avoided.
In `@pkg/itestkit/infra/mysql/mysql.go`:
- Around line 187-196: The code handling endpoint.ProxyListen currently ignores
strconv.Atoi's error, which can yield a silent port 0; update the proxy branch
(where net.SplitHostPort(endpoint.ProxyListen) is used) to capture the (portNum,
err := strconv.Atoi(proxyPort)) return and if err != nil return "", 0,
fmt.Errorf("invalid proxy port %q from %q: %w", proxyPort, endpoint.ProxyListen,
err) so the function fails fast on malformed ports instead of returning port 0.
- Around line 232-237: NewMySQLInfraStub creates the endpoint using the original
cfg and a manual host:port concat which leads to inconsistent credentials and
broken IPv6 formatting; update NewMySQLInfraStub to build upstream with
net.JoinHostPort(host, strconv.Itoa(port)) and construct the DSN using m.cfg
(e.g., m.cfg.Username, m.cfg.Password, m.cfg.Database) so the created
MySQLEndpoint (endpoint.Upstream and endpoint.DSN) matches the normalized values
stored on the MySQLInfra instance.
In `@pkg/itestkit/infra/seaweedfs/seaweedfs.go`:
- Around line 78-118: The Start method (SeaweedFSInfra.Start) can leave the
network and containers (master, volume, filer) running on error; update Start to
track created resources and always clean them up on any failure: after creating
the network (network.New) and each container (testcontainers.GenericContainer
returning master/volume/filer), record the resource in a local slice or
variables and on any subsequent error call Terminate/Remove for started
containers and remove the network (s.network) before returning the error; ensure
any early returns (after master, after volume, after filer, and before final
success) perform this rollback so no partial resources remain.
In `@tests/e2e/shared/assertions.go`:
- Around line 201-206: The AssertValidUUID helper currently allows any UUID
version; update it to enforce UUIDv4 by tightening the regex in AssertValidUUID
to require a '4' as the first hex of the 3rd group and [89ab] (case-insensitive
or lowercase) as the first hex of the 4th group, and update the function comment
and assertion message to state "valid UUID v4"; alternatively, if you intend to
accept any version, change the comment and assertion message to "valid UUID" to
match the existing regex (modify either the regex in AssertValidUUID or the text
consistently).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a46ca17-22f4-475f-81c6-ecce22174d8c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modpkg/itestkit/addons/queuekit/amqp.gopkg/itestkit/infra/mysql/mysql.gopkg/itestkit/infra/seaweedfs/seaweedfs.gotests/e2e/infra_datasources_test.gotests/e2e/shared/assertions.gotests/e2e/shared/client.gotests/e2e/shared/infra.go
gandalf-at-lerian
approved these changes
Mar 19, 2026
Contributor
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Suite de e2e bem estruturada, cobertura abrangente, itestkit com bom design. Fixes de produção (gRPC CVE, Alpine, pool cleanup, RabbitMQ monitor) parecem corretos e bem justificados. Pontos de atenção levantados no DM (chaos tests como follow-up, context sem timeout em alguns testes, nomes de TC vs comportamento real de DLQ). Nenhum blocker. ✅
arthurkz
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Additional Notes
Obs: Please, always remember to target your PR to develop branch instead of main.