From 0b8131c79e47a8d6ad4f6e7d94d2f902238e3a9c Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Fri, 7 May 2021 12:49:39 +0900 Subject: [PATCH] Handle nulls in noop / multi (#2939) * Handle nulls in noop / multi * Remove old file --- .../propagation/MultiTextMapPropagator.java | 9 ++++ .../propagation/NoopTextMapPropagator.java | 3 ++ .../MultiTextMapPropagatorTest.java | 51 +++++++++++-------- .../NoopTextMapPropagatorTest.java | 47 +++++++++++++---- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/context/src/main/java/io/opentelemetry/context/propagation/MultiTextMapPropagator.java b/context/src/main/java/io/opentelemetry/context/propagation/MultiTextMapPropagator.java index 2608ce51297..ce2cecb165c 100644 --- a/context/src/main/java/io/opentelemetry/context/propagation/MultiTextMapPropagator.java +++ b/context/src/main/java/io/opentelemetry/context/propagation/MultiTextMapPropagator.java @@ -45,6 +45,9 @@ private static List getAllFields(TextMapPropagator[] textPropagators) { @Override public void inject(Context context, @Nullable C carrier, TextMapSetter setter) { + if (context == null || setter == null) { + return; + } for (TextMapPropagator textPropagator : textPropagators) { textPropagator.inject(context, carrier, setter); } @@ -52,6 +55,12 @@ public void inject(Context context, @Nullable C carrier, TextMapSetter se @Override public Context extract(Context context, @Nullable C carrier, TextMapGetter getter) { + if (context == null) { + return Context.root(); + } + if (getter == null) { + return context; + } for (TextMapPropagator textPropagator : textPropagators) { context = textPropagator.extract(context, carrier, getter); } diff --git a/context/src/main/java/io/opentelemetry/context/propagation/NoopTextMapPropagator.java b/context/src/main/java/io/opentelemetry/context/propagation/NoopTextMapPropagator.java index 3e8331c5599..42117345f6e 100644 --- a/context/src/main/java/io/opentelemetry/context/propagation/NoopTextMapPropagator.java +++ b/context/src/main/java/io/opentelemetry/context/propagation/NoopTextMapPropagator.java @@ -27,6 +27,9 @@ public void inject(Context context, @Nullable C carrier, TextMapSetter se @Override public Context extract(Context context, @Nullable C carrier, TextMapGetter getter) { + if (context == null) { + return Context.root(); + } return context; } } diff --git a/context/src/test/java/io/opentelemetry/context/propagation/MultiTextMapPropagatorTest.java b/context/src/test/java/io/opentelemetry/context/propagation/MultiTextMapPropagatorTest.java index 39507797463..66e80bc95c3 100644 --- a/context/src/test/java/io/opentelemetry/context/propagation/MultiTextMapPropagatorTest.java +++ b/context/src/test/java/io/opentelemetry/context/propagation/MultiTextMapPropagatorTest.java @@ -12,9 +12,12 @@ import static org.mockito.Mockito.when; import io.opentelemetry.context.Context; +import io.opentelemetry.context.ContextKey; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -26,6 +29,8 @@ @ExtendWith(MockitoExtension.class) class MultiTextMapPropagatorTest { + private static final ContextKey KEY = ContextKey.named("key"); + @Mock private TextMapPropagator propagator1; @Mock private TextMapPropagator propagator2; @Mock private TextMapPropagator propagator3; @@ -93,19 +98,6 @@ void inject_allDelegated() { verify(propagator3).inject(context, carrier, setter); } - @Test - void inject_nullContextAllDelegated() { - Map carrier = new HashMap<>(); - Context context = null; - TextMapSetter> setter = Map::put; - - TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2, propagator3); - prop.inject(context, carrier, setter); - verify(propagator1).inject(context, carrier, setter); - verify(propagator2).inject(context, carrier, setter); - verify(propagator3).inject(context, carrier, setter); - } - @Test void extract_noPropagators() { Map carrier = new HashMap<>(); @@ -147,14 +139,33 @@ void extract_notFound() { @Test void extract_nullContext() { - Map carrier = new HashMap<>(); - Context context = null; - when(propagator1.extract(context, carrier, getter)).thenReturn(context); - when(propagator2.extract(context, carrier, getter)).thenReturn(context); + assertThat( + new MultiTextMapPropagator(propagator1, propagator2) + .extract(null, Collections.emptyMap(), getter)) + .isSameAs(Context.root()); + } - TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2); - Context result = prop.extract(context, carrier, getter); + @Test + void extract_nullGetter() { + Context context = Context.current().with(KEY, "treasure"); + assertThat( + new MultiTextMapPropagator(propagator1, propagator2) + .extract(context, Collections.emptyMap(), null)) + .isSameAs(context); + } - assertThat(result).isSameAs(context); + @Test + void inject_nullContext() { + Map carrier = new LinkedHashMap<>(); + new MultiTextMapPropagator(propagator1, propagator2).inject(null, carrier, Map::put); + assertThat(carrier).isEmpty(); + } + + @Test + void inject_nullSetter() { + Map carrier = new LinkedHashMap<>(); + Context context = Context.current().with(KEY, "treasure"); + new MultiTextMapPropagator(propagator1, propagator2).inject(context, carrier, null); + assertThat(carrier).isEmpty(); } } diff --git a/context/src/test/java/io/opentelemetry/context/propagation/NoopTextMapPropagatorTest.java b/context/src/test/java/io/opentelemetry/context/propagation/NoopTextMapPropagatorTest.java index 1d16115d9aa..4b5965df14e 100644 --- a/context/src/test/java/io/opentelemetry/context/propagation/NoopTextMapPropagatorTest.java +++ b/context/src/test/java/io/opentelemetry/context/propagation/NoopTextMapPropagatorTest.java @@ -8,12 +8,18 @@ import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.context.Context; +import io.opentelemetry.context.ContextKey; +import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; import javax.annotation.Nullable; import org.junit.jupiter.api.Test; class NoopTextMapPropagatorTest { + private static final ContextKey KEY = ContextKey.named("key"); + @Test void noopFields() { assertThat(TextMapPropagator.noop().fields()).isEmpty(); @@ -23,28 +29,51 @@ void noopFields() { void extract_contextUnchanged() { Context input = Context.current(); Context result = - TextMapPropagator.noop().extract(input, new HashMap<>(), new HashMapTextMapGetter()); + TextMapPropagator.noop().extract(input, new HashMap<>(), MapTextMapGetter.INSTANCE); assertThat(result).isSameAs(input); } @Test void extract_nullContext() { - Context input = null; - Context result = - TextMapPropagator.noop().extract(input, new HashMap<>(), new HashMapTextMapGetter()); - assertThat(result).isSameAs(input); + assertThat( + TextMapPropagator.noop() + .extract(null, Collections.emptyMap(), MapTextMapGetter.INSTANCE)) + .isSameAs(Context.root()); } - private static class HashMapTextMapGetter - implements TextMapGetter> { + @Test + void extract_nullGetter() { + Context context = Context.current().with(KEY, "treasure"); + assertThat(TextMapPropagator.noop().extract(context, Collections.emptyMap(), null)) + .isSameAs(context); + } + + @Test + void inject_nullContext() { + Map carrier = new LinkedHashMap<>(); + TextMapPropagator.noop().inject(null, carrier, Map::put); + assertThat(carrier).isEmpty(); + } + + @Test + void inject_nullSetter() { + Map carrier = new LinkedHashMap<>(); + Context context = Context.current().with(KEY, "treasure"); + TextMapPropagator.noop().inject(context, carrier, null); + assertThat(carrier).isEmpty(); + } + + enum MapTextMapGetter implements TextMapGetter> { + INSTANCE; + @Override - public Iterable keys(HashMap carrier) { + public Iterable keys(Map carrier) { return null; } @Nullable @Override - public String get(@Nullable HashMap carrier, String key) { + public String get(@Nullable Map carrier, String key) { return null; } }