Added null check to matcher result. - #174
Conversation
|
Overview Fixes #173: an NPE in LambdaProxyMiddleware.execute when a request path matches a configured function but the HTTP method has no entry in methodToMatcherMap. map.get(method) returned null and .match(path) was called on it unconditionally. The PR hoists the lookup into a local and guards it, letting the existing result == null branch handle the miss. Two lines, correct, minimal, and it lands exactly on the reported defect. Correctness
The root cause this doesn't address Worth raising on the issue even if it's out of scope for this PR. There's a case-normalization asymmetry:
So a config entry written /v1/pets@GET registers under key GET, and every request for it looks up get and misses. Before this PR that was an NPE; after it, it's a clean 400 — but the route still silently never works. Normalizing at line 129 (endpoint[1].toLowerCase()) would make those configs function rather than fail politely. I'd suggest it as a follow-up; the PR is still correct without it. Other observations
Test coverage This is the one substantive gap. MockLambdaProxyMiddlewareTest has a single testConstructor case that asserts the happy path (get + /v1/pets/123), and MockLambdaProxyMiddleware doesn't share the fixed code — it has its own execute using a direct CONFIG.getFunctions().get(...) lookup, so it can't exercise this path at all. A regression test would need to hit the real LambdaProxyMiddleware. Even a narrow unit test asserting that an unmapped method yields Status ERR10086 instead of throwing would lock in the fix. Without one, nothing stops this from regressing. Security & performance
Verdict Approve. Correct, minimal, appropriately scoped to the reported NPE. Two follow-ups I'd open separately rather than block on: the toLowerCase() asymmetry in populateMethodToMatcherMap (which is likely the real reason users hit this), and a regression test against the real middleware. |
Added null check to prevent
.match(...)call when match result is null.