feat(parser): add errorHandler option for inline parse error handling#5352
Conversation
dreamorosi
left a comment
There was a problem hiding this comment.
Hi @Zelys-DFKH, thank you for picking this up and for the well-structured PR - and apologies for the delay in getting to the review.
The direction matches what we discussed in #4191 and the implementation is easy to follow, however there are a few things we need to address before this can move forward - see the inline comments for the details and reasoning:
- an
asyncerrorHandlercurrently swallows parse failures and lets the handler run with the unparsed event (must fix) - in the Middy.js flavor, short-circuiting from
beforeskips every other middleware'safterphase - I'd like the recovery to go through theonErrorphase instead safeParseanderrorHandlershould be mutually exclusive at the type level- the callback should receive the original event alongside the error
- the Middy.js middleware's response type should reflect the error handler's return type
On top of that:
- the feature needs documentation in
docs/features/parser.md(thesafeParsesection is the natural home for it) plus code snippets underexamples/snippets/parser - missing test cases: envelope +
errorHandler, non-ParseErrorpropagation, and thesafeParseinteraction biome checkcurrently fails on formatting inparser.middy.test.ts
Happy to clarify or discuss any of these - thanks again for the contribution!
|
btw could you please leave a quick comment on #4191? We'd like to assign the issue to you for tracking, but GitHub only allows assigning users who have either commented on the issue or are collaborators — so right now we can't. Once you've commented, we'll assign it to you. Thanks! |
Constrain errorHandler to synchronous returns (type + runtime guard), move Middy.js recovery into onError so other middlewares' after/onError phases still run, make safeParse and errorHandler mutually exclusive at the type level, pass the original event to errorHandler alongside the error, and thread the handler's return type through MiddlewareObj. Also fixes two bugs found during self-review after implementing the above: errorHandler could be invoked for a ParseError unrelated to this parser's own parse failure (now scoped via a per-request WeakSet in the middleware and a narrower try/catch in the decorator), and the sync guard only checked for a native Promise, missing other thenables. Adds docs, example snippets, and the missing test cases called out in review.
|
Thanks for the thorough review, @dreamorosi, and sorry for the slow turnaround. All five points are addressed below, along with the docs, tests, and the biome fix.
One more thing came up while testing the onError change: since ParseError is public API, both flavors were treating any ParseError reaching them as this parser's own failure, including one a customer's handler might throw for unrelated reasons. Fixed that by scoping recovery to this parser instance's own before-phase failure (a WeakSet keyed on the request for the middleware, a narrower try/catch around just the parse() call for the decorator), with regression tests for both. Let me know if I've missed anything, or if you'd rather take a different approach on any of these. |
Bring in upstream changes to resolve the "behind" mergeable_state on PR aws-powertools#5352 before it's reviewed further.
|
There is a SonarQube issue (very simple fix), can you address it? |
Addresses SonarQube S6606: the ternary only ever branched on undefined, so ?? is equivalent and simpler.
dreamorosi
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough turnaround @Zelys-DFKH - all the points from the previous review have been addressed exactly as discussed: the sync-only errorHandler constraint, the onError-based recovery in the Middy.js flavor, the mutually exclusive types, the event in the callback signature, the threaded response type, plus the docs, snippets, and the extra test coverage. We're getting close to merging this.
There's one small inconsistency left, introduced by the SonarQube follow-up - see the inline comment. Once that's sorted I think we're good to go.
Restores the documented contract that only undefined triggers a rethrow. The earlier SonarQube-driven switch to ?? incorrectly treated null the same as undefined, but null can be a legitimate value a customer's errorHandler intentionally returns. Adds test coverage for both flavors.
Leaves a note at the exact spot a future SonarQube pass is likely to flag again, so the next person doesn't repeat the same regression.
|
dreamorosi
left a comment
There was a problem hiding this comment.
Thank you for addressing the review comments and implementing the feature!



Summary
Changes
Adds an optional
errorHandlercallback toParserOptions, letting callers intercept parse failures and return a custom response instead of throwing.The motivation comes from #4191: when
@parseris stacked with other decorators that expect a fully-typed event,safeParse: truedoesn't compose well -- the outer decorators seeParsedResultrather than the actual payload. An error handler keeps the event type clean while still providing a recovery path on parse failure. Thanks to @codyfrisch for walking through this use case in detail.The callback receives the
ParseError. If it returns a non-undefined value, that becomes the Lambda/Middy response and stops further processing. If it returnsundefined, the original error is rethrown -- so you can handle specific failure cases without silently absorbing unexpected ones. Non-ParseErrorexceptions (e.g., from schema transforms) always propagate as-is.Both the
@parserclass decorator andparser()Middy middleware support the new option.Issue number: closes #4191
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.