From 610ff80ebec2b831a7ea6cc73ad4614152a36ad9 Mon Sep 17 00:00:00 2001 From: dvfeinblum Date: Tue, 24 Apr 2018 21:48:51 -0400 Subject: Added null checking to Span implementations (#1150) This PR adds null checking to classes that implement Span. Specifically, - BlankSpan - NoopSpan - Span - SpanImpl For the latter, I had to use Preconditions.checkNotNull because io.opencensus.internal shouldn't be imported into ImplCore. --- .../main/java/io/opencensus/trace/BlankSpan.java | 31 +++++++++++++++++----- api/src/main/java/io/opencensus/trace/Span.java | 6 +++++ .../test/java/io/opencensus/trace/NoopSpan.java | 22 +++++++++++---- .../io/opencensus/implcore/trace/SpanImpl.java | 13 ++++++--- 4 files changed, 56 insertions(+), 16 deletions(-) diff --git a/api/src/main/java/io/opencensus/trace/BlankSpan.java b/api/src/main/java/io/opencensus/trace/BlankSpan.java index 641bad7e..3109f013 100644 --- a/api/src/main/java/io/opencensus/trace/BlankSpan.java +++ b/api/src/main/java/io/opencensus/trace/BlankSpan.java @@ -16,6 +16,7 @@ package io.opencensus.trace; +import io.opencensus.internal.Utils; import java.util.Map; import javax.annotation.concurrent.Immutable; @@ -42,19 +43,29 @@ public final class BlankSpan extends Span { /** No-op implementation of the {@link Span#putAttribute(String, AttributeValue)} method. */ @Override - public void putAttribute(String key, AttributeValue value) {} + public void putAttribute(String key, AttributeValue value) { + Utils.checkNotNull(key, "key"); + Utils.checkNotNull(value, "value"); + } /** No-op implementation of the {@link Span#putAttributes(Map)} method. */ @Override - public void putAttributes(Map attributes) {} + public void putAttributes(Map attributes) { + Utils.checkNotNull(attributes, "attributes"); + } /** No-op implementation of the {@link Span#addAnnotation(String, Map)} method. */ @Override - public void addAnnotation(String description, Map attributes) {} + public void addAnnotation(String description, Map attributes) { + Utils.checkNotNull(description, "description"); + Utils.checkNotNull(attributes, "attributes"); + } /** No-op implementation of the {@link Span#addAnnotation(Annotation)} method. */ @Override - public void addAnnotation(Annotation annotation) {} + public void addAnnotation(Annotation annotation) { + Utils.checkNotNull(annotation, "annotation"); + } /** No-op implementation of the {@link Span#addNetworkEvent(NetworkEvent)} method. */ @Override @@ -63,14 +74,20 @@ public final class BlankSpan extends Span { /** No-op implementation of the {@link Span#addMessageEvent(MessageEvent)} method. */ @Override - public void addMessageEvent(MessageEvent messageEvent) {} + public void addMessageEvent(MessageEvent messageEvent) { + Utils.checkNotNull(messageEvent, "messageEvent"); + } /** No-op implementation of the {@link Span#addLink(Link)} method. */ @Override - public void addLink(Link link) {} + public void addLink(Link link) { + Utils.checkNotNull(link, "link"); + } @Override - public void setStatus(Status status) {} + public void setStatus(Status status) { + Utils.checkNotNull(status, "status"); + } /** No-op implementation of the {@link Span#end(EndSpanOptions)} method. */ @Override diff --git a/api/src/main/java/io/opencensus/trace/Span.java b/api/src/main/java/io/opencensus/trace/Span.java index 4a8c6f04..cb82038d 100644 --- a/api/src/main/java/io/opencensus/trace/Span.java +++ b/api/src/main/java/io/opencensus/trace/Span.java @@ -96,6 +96,8 @@ public abstract class Span { // Not final because for performance reasons we want to override this in the implementation. // Also a default implementation is needed to not break the compatibility (users may extend this // for testing). + Utils.checkNotNull(key, "key"); + Utils.checkNotNull(value, "value"); putAttributes(Collections.singletonMap(key, value)); } @@ -110,6 +112,7 @@ public abstract class Span { public void putAttributes(Map attributes) { // Not final because we want to start overriding this method from the beginning, this will // allow us to remove the addAttributes faster. All implementations MUST override this method. + Utils.checkNotNull(attributes, "attributes"); addAttributes(attributes); } @@ -130,6 +133,7 @@ public abstract class Span { * @since 0.5 */ public final void addAnnotation(String description) { + Utils.checkNotNull(description, "description"); addAnnotation(description, EMPTY_ATTRIBUTES); } @@ -180,6 +184,7 @@ public abstract class Span { public void addMessageEvent(MessageEvent messageEvent) { // Default implementation by invoking addNetworkEvent() so that any existing derived classes, // including implementation and the mocked ones, do not need to override this method explicitly. + Utils.checkNotNull(messageEvent, "messageEvent"); addNetworkEvent(BaseMessageEventUtils.asNetworkEvent(messageEvent)); } @@ -209,6 +214,7 @@ public abstract class Span { public void setStatus(Status status) { // Implemented as no-op for backwards compatibility (for example gRPC extends Span in tests). // Implementation must override this method. + Utils.checkNotNull(status, "status"); } /** diff --git a/api/src/test/java/io/opencensus/trace/NoopSpan.java b/api/src/test/java/io/opencensus/trace/NoopSpan.java index 7c8d9b7a..e90f68cd 100644 --- a/api/src/test/java/io/opencensus/trace/NoopSpan.java +++ b/api/src/test/java/io/opencensus/trace/NoopSpan.java @@ -16,6 +16,7 @@ package io.opencensus.trace; +import io.opencensus.internal.Utils; import java.util.EnumSet; import java.util.Map; import javax.annotation.Nullable; @@ -33,22 +34,33 @@ public class NoopSpan extends Span { } @Override - public void putAttributes(Map attributes) {} + public void putAttributes(Map attributes) { + Utils.checkNotNull(attributes, "attributes"); + } @Override - public void addAnnotation(String description, Map attributes) {} + public void addAnnotation(String description, Map attributes) { + Utils.checkNotNull(description, "description"); + Utils.checkNotNull(attributes, "attributes"); + } @Override - public void addAnnotation(Annotation annotation) {} + public void addAnnotation(Annotation annotation) { + Utils.checkNotNull(annotation, "annotation"); + } @Override public void addNetworkEvent(NetworkEvent networkEvent) {} @Override - public void addMessageEvent(MessageEvent messageEvent) {} + public void addMessageEvent(MessageEvent messageEvent) { + Utils.checkNotNull(messageEvent, "messageEvent"); + } @Override - public void addLink(Link link) {} + public void addLink(Link link) { + Utils.checkNotNull(link, "link"); + } @Override public void end(EndSpanOptions options) {} diff --git a/impl_core/src/main/java/io/opencensus/implcore/trace/SpanImpl.java b/impl_core/src/main/java/io/opencensus/implcore/trace/SpanImpl.java index f9a7f859..81b27e32 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/trace/SpanImpl.java +++ b/impl_core/src/main/java/io/opencensus/implcore/trace/SpanImpl.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.collect.EvictingQueue; import io.opencensus.common.Clock; import io.opencensus.implcore.internal.CheckerFrameworkUtils; @@ -281,6 +282,7 @@ public final class SpanImpl extends Span implements Element { @Override public void putAttributes(Map attributes) { + Preconditions.checkNotNull(attributes, "attributes"); if (!getOptions().contains(Options.RECORD_EVENTS)) { return; } @@ -295,6 +297,8 @@ public final class SpanImpl extends Span implements Element { @Override public void addAnnotation(String description, Map attributes) { + Preconditions.checkNotNull(description, "description"); + Preconditions.checkNotNull(attributes, "attribute"); if (!getOptions().contains(Options.RECORD_EVENTS)) { return; } @@ -313,6 +317,7 @@ public final class SpanImpl extends Span implements Element { @Override public void addAnnotation(Annotation annotation) { + Preconditions.checkNotNull(annotation, "annotation"); if (!getOptions().contains(Options.RECORD_EVENTS)) { return; } @@ -322,9 +327,7 @@ public final class SpanImpl extends Span implements Element { return; } getInitializedAnnotations() - .addEvent( - new EventWithNanoTime( - clock.nowNanos(), checkNotNull(annotation, "annotation"))); + .addEvent(new EventWithNanoTime(clock.nowNanos(), annotation)); } } @@ -348,6 +351,7 @@ public final class SpanImpl extends Span implements Element { @Override public void addLink(Link link) { + Preconditions.checkNotNull(link, "link"); if (!getOptions().contains(Options.RECORD_EVENTS)) { return; } @@ -356,12 +360,13 @@ public final class SpanImpl extends Span implements Element { logger.log(Level.FINE, "Calling addLink() on an ended Span."); return; } - getInitializedLinks().addEvent(checkNotNull(link, "link")); + getInitializedLinks().addEvent(link); } } @Override public void setStatus(Status status) { + Preconditions.checkNotNull(status, "status"); if (!getOptions().contains(Options.RECORD_EVENTS)) { return; } -- cgit v1.2.3