fix(logger): summarise large binary query parameters - #12810
Conversation
stringifyParams called JSON.stringify on the whole parameter array, and a buffer serialises as an array of one number per byte. A large binary parameter therefore produced a log line several times the size of the data, which exhausts memory before anything is written β reported with a 200MB attachment crashing the process. A binary parameter over 1KB is now logged as its type and byte length instead. Everything else is rendered exactly as before, including the cases the old array-level stringify handled implicitly: an unrepresentable value still becomes null, and a parameter set that cannot be serialised at all still falls back to the raw value. Closes typeorm#10515
Code Review by Qodo
1. Binary detection is shallow
|
Rendering the array element by element changed two things it should not have. Array.prototype.map skips holes, so a sparse parameter array lost them β a hole logged as an empty slot rather than null β and serialising each element separately meant a custom toJSON received an empty key instead of the element's index. Substituting the oversized binary values first and then stringifying the array in one call keeps the summary while leaving every other rendering detail exactly as it was.
|
Pushed 646ffd6. Two of the three findings above were real, and both were mine. Rendering the array element by element was the mistake. The fix is simpler than what it replaces: substitute the oversized binary values first, then stringify the array in a single call. The summary still happens, and every other rendering detail is the array-level On the third finding, that the binary check is shallow: that is true, and deliberate for now. It matches the approach sketched on #10515, and going deeper means walking arbitrary nested structures on every logged query, which costs something on a hot path. If you would rather it recursed, say so and I will do it β but it seemed worth keeping this change to the case that actually crashes. |
Description of change
AbstractLogger.stringifyParamscallsJSON.stringifyon the whole parameter array. A buffer serialises as one number per byte, so a binary parameter produces a log line several times the size of the data itself β enough to exhaust memory before anything is written, as @alumni reported in #10515 with a 200MB attachment.Current behavior: a 200MB buffer parameter is serialised in full, and the process runs out of memory.
AdvancedConsoleLoggermakes it worse, since it then tries to syntax-highlight the result.New behavior: a binary parameter larger than 1KB is logged as its type and byte length β
["<Buffer(200000000 bytes)>"]β and everything else renders exactly as before.This follows the approach @alumni sketched on the issue, with the threshold as a named constant so it is easy to argue with. 1KB seemed like the point where the serialised form stops being readable anyway; happy to move it.
Preserving the existing output
Rendering the array element by element rather than in one call is where behaviour could drift, so I checked rather than assumed. Running the old and new implementations side by side over numbers, strings needing escapes,
null,undefined, booleans, nested objects, dates, empty arrays, functions, small buffers, circular references and aBigInt, every non-large-binary case produces a byte-identical string.Two of those are worth calling out, because they are what an element-wise rewrite usually gets wrong:
JSON.stringifyreturnsundefinedfor a value it cannot represent, while the array-level call renders it asnull. The element path restores that, so[undefined]and[() => 1]still log as[null].BigIntβ still falls back to returning the raw parameters, unchanged.The elements are joined with
,rather than,for the same reason: it keeps existing log lines identical.Scope
Only
stringifyParams. The second observation on the issue β thatSelectQueryBuilder.loadRawResultsalso doesJSON.stringify(parameters)when building a cache key β is left alone deliberately: it is a different code path with different constraints (the string is a cache identity, not a log line, so summarising a parameter there could collide two distinct queries). Glad to look at it separately if you want it fixed too.How I verified it
pnpm run compilepasses,eslintreports no errors,prettier --checkis clean, andtest:fastpasses apart from an unrelatedcli initfailure that reproduces on a cleanmasterhere.Seven unit tests are added in
test/unit/logger/stringify-params.test.ts, covering the large-buffer summary, a non-Bufferbinary view, a small buffer still being serialised in full, ordinary parameters being untouched,undefinedstill rendering asnull, and the circular-reference fallback. They need no database.Pull-Request Checklist
masterbranchtests/**.test.ts)docs/docs/**.md) β N/A, no user-facing API change