Fix OnEventf argument forwarding in the composite log - #779
Open
Robin1987China wants to merge 1 commit into
Open
Robin1987China wants to merge 1 commit into
Robin1987China wants to merge 1 commit into
Conversation
compositeLog.OnEventf passed the variadic arguments as a single
[]interface{} argument, so every wrapped log rendered the format verbs
against one slice value, producing output such as
"session [FIX.4.4 %!s(int=7)] seq %!d(MISSING)" instead of
"session FIX.4.4 seq 7".
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
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.
Wrapped logs receive malformed event messages from a composite log.
compositeLog.OnEventfforwardedits variadic arguments as a single
[]interface{}value instead of expanding them, so every formatverb in the format string was rendered against that one slice.
Before
log/compositewith two wrapped logs,l.OnEventf("session %s seq %d", "FIX.4.4", 7):Even a single argument is mangled (
%srenders the slice itself):There is no error — the garbage is written to the log and only shows up on inspection.
Fix
One line in
log/composite/composite_log.go:quickfix.Log.OnEventfisOnEventf(string, ...interface{}), soawas passed as one argument whosedynamic type is a slice.
Affected paths
Any call that goes through a composite log. 29
OnEventfcall sites pass arguments — inacceptor.go,session.go,in_session.go,session_state.go,initiator.go,logon_state.go,latent_state.go,not_session_time.go— and 13 of them pass two or more.OnIncoming/OnOutgoing/OnEventarenon-variadic and are not affected; this is the only occurrence in the repository.
Test
TestCompositeLogOnEventfExpandsFormatArgsinlog/composite/composite_log_test.go, with three subtests:single string arg—"session %s": shows even one argument was corrupted.mixed args—"session %s seq %d": the common case, and where%!d(MISSING)appears.slice arg—"tags %v count %d"with a[]int: guards against a "join the args into a string andforward that" fix, which would corrupt the output for slice arguments.
Each asserts both wrapped logs receive the expanded result.
It is a standalone
Test*function rather than a case in the existingCompositeLogTestSuite: thatsuite
SkipNow()s as a whole whenMONGODB_TEST_CXNis unset, so a test added there would silentlynever run without MongoDB. The new test has no external dependencies — no MongoDB, no SQL, no network,
no credentials.
Verification
go build ./...— cleango test -run TestCompositeLogOnEventfExpandsFormatArgs -v ./log/composite/— 3/3 PASSgo test -race ./log/composite/— okgofmt -l .andgofmt -s -l .— no outputmake vet— exit 0make test-ci— all packages pass exceptlog/mongo, which fails identically on the unmodifiedbaseline (see below);
log/compositepassesgolangci-lint(v1.64.6, repo.golangci.yml,GOTOOLCHAIN=go1.23.12) — 0 issues, same as baselineRepeating the test with the one line reverted turns all three subtests red again, so the fix is what
drives them.
What I could not verify
make test(it hardcodesMONGODB_TEST_CXN=mongodb://db:27017) was not run;I ran the CI job's command
make test-ciinstead.log/mongofails there both with and without mychange — without a connection string the suite skips, then panics on a nil dereference — which is
already tracked by Test: resolve panic when MONGODB_TEST_CXN is not set #755. Not caused by, and not fixed by, this PR.
1.23.12 (the version declared in
go.mod), not on 1.21.make lintdoes not work locally as written (the Makefile installs the binary into./binbut callsbare
golangci-lint); I did not touch it.make acceptwas not run — it needs ruby,make generateand an echo server. This change does nottouch parsing, protocol or generated code.
the call sites listed above.