Fix Handle null context in Baggage.fromContext() and Baggage.fromContextOrNull() - #8667
Fix Handle null context in Baggage.fromContext() and Baggage.fromContextOrNull()#8667NithinU2802 wants to merge 2 commits into
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-07-28 16:27 UTC Review the latest changes. Status above doesn't look right?
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8667 +/- ##
============================================
- Coverage 91.46% 91.46% -0.01%
- Complexity 10456 10458 +2
============================================
Files 1021 1021
Lines 27647 27653 +6
Branches 3242 3242
============================================
+ Hits 25288 25293 +5
Misses 1616 1616
- Partials 743 744 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @Nullable | ||
| static Baggage fromContextOrNull(Context context) { | ||
| if (context == null) { | ||
| ApiUsageLogger.logNullParam(Context.class, "fromContextOrNull", "context"); |
There was a problem hiding this comment.
Small inconsistency here: this passes Context.class while fromContext above passes Baggage.class. Span.fromContextOrNull uses the declaring class too, and since the message is built from it, users would see a warning naming Context.fromContextOrNull, which isn't a real method on Context.
| ApiUsageLogger.logNullParam(Context.class, "fromContextOrNull", "context"); | |
| ApiUsageLogger.logNullParam(Baggage.class, "fromContextOrNull", "context"); |
There was a problem hiding this comment.
yeah, that's correct had updated from Context to Baggage.
| } | ||
|
|
||
| @Test | ||
| void fromContext_null() { |
There was a problem hiding this comment.
The issue lists the ApiUsageLogger output as part of the expected behavior, but the test only asserts return values. Adding a LogCapturer assertion on the api usage logger would cover it, and would have caught the Context.class / Baggage.class mismatch above. Minor: might read better split into fromContext_null and fromContextOrNull_null.
There was a problem hiding this comment.
I have split tests into fromContext_null and fromContextOrNull_null also added LogCapturer to verify the ApiUsageLogger output✌️
There was a problem hiding this comment.
LGTM, both points addressed. Needs a maintainer's sign-off though, I'm just a fellow contributor, not a code owner
@jack-berg can do the rest
…l and fromContextOrNull_null tests, and add LogCapturer assertions
2 participants
In Baggage, to add null checks to Baggage.fromContext(context) and Baggage.fromContextOrNull(context) with log using ApiUsageLogger and for Baggage.fromContext(context) now returns empty() when the context is null, also for Baggage.fromContextOrNull(context) returns null. Additionally, added unit tests to cover these scenarios.
I'm happy to update any changes if needed.
Closes #8665