chore: fix test breakage in error-reporting - #8966
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Hapi request extractor to support both hapi.Request and plain object types, adding robust type and null checks for headers, URLs, and status codes. It also includes a new unit test to verify correct handling of array headers. The reviewer suggested replacing the check req.info?.toString() === '[object Object]' with a more robust object check to prevent false positives from single-element arrays of objects.
I am having trouble creating individual review comments. Click here to see my feedback.
handwritten/error-reporting/src/request-extractors/hapi.ts (57-67)
Avoid using req.info?.toString() === '[object Object]' to verify if a variable is a plain object, because single-element arrays of objects (e.g., [{}]) will also evaluate to "[object Object]". Use typeof req.info === 'object' && req.info !== null && !Array.isArray(req.info) instead.
function extractRemoteAddressFromRequest(
req: any,
) {
if (
req.headers &&
typeof req.headers === 'object' &&
'x-forwarded-for' in req.headers
) {
return req.headers['x-forwarded-for'];
}
if (
req.info &&
typeof req.info === 'object' &&
!Array.isArray(req.info)
) {
return req.info.remoteAddress;
}References
- Do not use
value?.toString() === '[object Object]'to verify if a variable is a plain object, because single-element arrays of objects (e.g.,[{}]) will also evaluate to"[object Object]". Usetypeof value === 'object' && value !== null && !Array.isArray(value)or a similar robust check instead.
|
Note, in response to the Gemini feedback, the lines it's reporting on are not modified by the PR. My preference here is to maintain the minimum possible diff to resolve the test failure. |
shivanee-p
left a comment
There was a problem hiding this comment.
it appears to require a running server to inject dependencies <-- that's wild for a mock 👀
Ty for updating the unit tests to reflect this
* chore: fix test breakage in error-reporting * rollback to minimum set of changes required * add test whitespace for readability
Why I am addressing this
I am trying to fix all of the current outstanding test breakages blocking the merge of our new Bun CI (#8904). To verify this PR, we are running ALL unit tests in the monorepo. This has revealed a handful of existing breakages, including this one.
The problem
In
@hapi/hapi(and modern@types/node/ HTTP header type definitions),req.headerselements are typed asunknown(e.g. Record<string, unknown>). Passing header values likereq.headers['user-agent'],req.headers.referrer, orextractRemoteAddressFromRequest(req)directly intogetSingleHeader(val: string | string[] | undefined)causedTS2345: Argument of type 'unknown' is not assignable to parameter of type 'string | string[] | undefined'.The fix
Update the internal implementation to correctly reflect the real types and add a new test to verify functionality.
I don't love the use of
ascasting in the tests, but mocking a type-complianthapi.Requestobject is surprisingly difficult (it appears to require a running server to inject dependencies). I spoke with Gemini about this and it recommended casting a bare JS object (like the existing tests). An alternative would be to update the implementation to have a less strict type, but I would prefer to keep the minimum diff.