Fix: be more defensive regarding request traces - #1973
Conversation
As they NPE in UTs but would also NPE in calls made by some non-maven callers that usually do not set trace at all.
gnodet
left a comment
There was a problem hiding this comment.
Verdict: ✅ Approve — clean, correct fix for a real NPE.
What this fixes: Stream.findFirst() throws NullPointerException when the first mapped element is null (it internally calls Optional.of(value) which rejects null). When a non-Maven caller doesn't set a trace on its requests, getTrace() returns null and the stream crashes before orElse(null) is ever reached.
What I checked:
isReentrant(RequestTrace)is already null-safe — the for-loop conditiont != nullexits immediately, returningfalse(i.e., "not re-entrant" → validation runs). So the downstream behavior for null traces was always correct; only the stream extraction was broken.- The
Objectsimport was already present — no structural changes needed. - Both
resolveArtifacts()andresolveMetadata()had the same pattern and both are fixed symmetrically. - Single-request overloads (
resolveArtifact,collectDependencies, etc.) callisReentrant(request.getTrace())directly — no stream, no issue.
Minor observation (no action needed): The comment "All requests in a batch share the same trace context, so checking any one is sufficient" is slightly imprecise now that the code specifically seeks the first non-null trace, but this is purely cosmetic.
CI: ✅ 18/18 jobs green.
Reviewed with AI assistance on behalf of @gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
As they NPE in UTs but would also NPE in calls made by some non-maven callers that usually do not set trace at all.