From 646bd77464d6cc14bc6beb04ff2c83eff3f5aa29 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Tue, 22 Oct 2019 23:44:39 +0200 Subject: [PATCH 1/6] some android lint ref. --- .../core/DefaultAndroidEventProcessor.java | 4 +- .../java/io/sentry/core/DiagnosticLogger.java | 5 +-- .../java/io/sentry/core/EnvelopeReader.java | 2 +- .../src/main/java/io/sentry/core/Hub.java | 44 +++++++++++++------ .../main/java/io/sentry/core/NoOpLogger.java | 2 +- .../src/main/java/io/sentry/core/Scope.java | 8 +++- .../src/main/java/io/sentry/core/Sentry.java | 2 +- .../src/test/java/io/sentry/core/HubTest.kt | 36 +++++++++++++++ .../java/io/sentry/core/SentryEnvelopeTest.kt | 4 +- .../core/transport/HttpTransportTest.kt | 2 +- 10 files changed, 81 insertions(+), 28 deletions(-) create mode 100644 sentry-core/src/test/java/io/sentry/core/HubTest.kt diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java index 2967c6614..594202ae3 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java @@ -23,8 +23,8 @@ import java.util.*; public class DefaultAndroidEventProcessor implements EventProcessor { - Context context; - SentryOptions options; + final Context context; + final SentryOptions options; // it could also be a parameter and get from Sentry.init(...) private static final Date appStartTime = DateUtils.getCurrentDateTime(); diff --git a/sentry-core/src/main/java/io/sentry/core/DiagnosticLogger.java b/sentry-core/src/main/java/io/sentry/core/DiagnosticLogger.java index bb0463d26..bedf5d7b3 100644 --- a/sentry-core/src/main/java/io/sentry/core/DiagnosticLogger.java +++ b/sentry-core/src/main/java/io/sentry/core/DiagnosticLogger.java @@ -5,8 +5,8 @@ /** Sentry SDK internal diagnostic logger. */ public class DiagnosticLogger implements ILogger { - private SentryOptions options; - private ILogger logger; + private final SentryOptions options; + private final ILogger logger; /** * Creates a new instance of DiagnosticLogger with the wrapped ILogger. @@ -16,7 +16,6 @@ public class DiagnosticLogger implements ILogger { */ public DiagnosticLogger(SentryOptions options, @Nullable ILogger logger) { this.options = Objects.requireNonNull(options, "SentryOptions is required."); - ; this.logger = logger; } diff --git a/sentry-core/src/main/java/io/sentry/core/EnvelopeReader.java b/sentry-core/src/main/java/io/sentry/core/EnvelopeReader.java index 1e39f4871..529535107 100644 --- a/sentry-core/src/main/java/io/sentry/core/EnvelopeReader.java +++ b/sentry-core/src/main/java/io/sentry/core/EnvelopeReader.java @@ -27,7 +27,7 @@ SentryEnvelopeItemHeader.class, new SentryEnvelopeItemHeaderAdapter()) public @Nullable SentryEnvelope read(InputStream stream) throws IOException { byte[] buffer = new byte[1024]; - int currentLength = 0; + int currentLength; int streamOffset = 0; // Offset of the line break defining the end of the envelope header int envelopeEndHeaderOffset = -1; diff --git a/sentry-core/src/main/java/io/sentry/core/Hub.java b/sentry-core/src/main/java/io/sentry/core/Hub.java index d66f5371b..014dccdae 100644 --- a/sentry-core/src/main/java/io/sentry/core/Hub.java +++ b/sentry-core/src/main/java/io/sentry/core/Hub.java @@ -37,8 +37,7 @@ private Hub(SentryOptions options, @Nullable StackItem rootStackItem) { static StackItem createRootStackItem(SentryOptions options) { Scope scope = new Scope(); ISentryClient client = new SentryClient(options); - StackItem item = new StackItem(client, scope); - return item; + return new StackItem(client, scope); } @Override @@ -50,7 +49,7 @@ public boolean isEnabled() { public SentryId captureEvent(SentryEvent event) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = item.client.captureEvent(event, item.scope); + sentryId = item != null ? item.client.captureEvent(event, item.scope) : SentryId.EMPTY_ID; this.lastEventId = sentryId; return sentryId; } @@ -59,7 +58,7 @@ public SentryId captureEvent(SentryEvent event) { public SentryId captureMessage(String message) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = item.client.captureMessage(message, item.scope); + sentryId = item != null ? item.client.captureMessage(message, item.scope) : SentryId.EMPTY_ID; this.lastEventId = sentryId; return sentryId; } @@ -68,7 +67,8 @@ public SentryId captureMessage(String message) { public SentryId captureException(Throwable throwable) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = item.client.captureException(throwable, item.scope); + sentryId = + item != null ? item.client.captureException(throwable, item.scope) : SentryId.EMPTY_ID; this.lastEventId = sentryId; return sentryId; } @@ -77,14 +77,18 @@ public SentryId captureException(Throwable throwable) { public void close() { // Close the top-most client StackItem item = stack.peek(); - item.client.close(); + if (item != null) { + item.client.close(); + } isEnabled = false; } @Override public void addBreadcrumb(Breadcrumb breadcrumb) { StackItem item = stack.peek(); - item.scope.addBreadcrumb(breadcrumb); + if (item != null) { + item.scope.addBreadcrumb(breadcrumb); + } } @Override @@ -95,9 +99,13 @@ public SentryId getLastEventId() { @Override public void pushScope() { StackItem item = stack.peek(); - Scope clone = item.scope.clone(); - StackItem newItem = new StackItem(item.client, clone); - stack.push(newItem); + if (item != null) { + Scope clone = item.scope.clone(); + if (clone != null) { + StackItem newItem = new StackItem(item.client, clone); + stack.push(newItem); + } + } } @Override @@ -115,7 +123,9 @@ public void withScope(ScopeCallback callback) { pushScope(); try { StackItem item = stack.peek(); - callback.run(item.scope); + if (item != null) { + callback.run(item.scope); + } } finally { popScope(); } @@ -124,19 +134,25 @@ public void withScope(ScopeCallback callback) { @Override public void configureScope(ScopeCallback callback) { StackItem item = stack.peek(); - callback.run(item.scope); + if (item != null) { + callback.run(item.scope); + } } @Override public void bindClient(SentryClient client) { StackItem item = stack.peek(); - item.client = client; + if (item != null) { + item.client = client; + } } @Override public void flush(long timeoutMills) { StackItem item = stack.peek(); - item.client.flush(timeoutMills); + if (item != null) { + item.client.flush(timeoutMills); + } } @Override diff --git a/sentry-core/src/main/java/io/sentry/core/NoOpLogger.java b/sentry-core/src/main/java/io/sentry/core/NoOpLogger.java index 70de56b9a..4be64670f 100644 --- a/sentry-core/src/main/java/io/sentry/core/NoOpLogger.java +++ b/sentry-core/src/main/java/io/sentry/core/NoOpLogger.java @@ -3,7 +3,7 @@ /** No-op implementation of ILogger */ class NoOpLogger implements ILogger { - private static NoOpLogger instance = new NoOpLogger(); + private static final NoOpLogger instance = new NoOpLogger(); public static NoOpLogger getInstance() { return instance; diff --git a/sentry-core/src/main/java/io/sentry/core/Scope.java b/sentry-core/src/main/java/io/sentry/core/Scope.java index 07b391edf..415e66909 100644 --- a/sentry-core/src/main/java/io/sentry/core/Scope.java +++ b/sentry-core/src/main/java/io/sentry/core/Scope.java @@ -77,7 +77,11 @@ public void setExtra(String key, String value) { @Override protected Scope clone() { - // TODO: clone me - return new Scope(); + Scope d = null; + try { + d = (Scope) super.clone(); + } catch (CloneNotSupportedException ignored) { + } // Won't happen + return d; } } diff --git a/sentry-core/src/main/java/io/sentry/core/Sentry.java b/sentry-core/src/main/java/io/sentry/core/Sentry.java index b3dc1bb35..66e65e2e0 100644 --- a/sentry-core/src/main/java/io/sentry/core/Sentry.java +++ b/sentry-core/src/main/java/io/sentry/core/Sentry.java @@ -8,7 +8,7 @@ public final class Sentry { private Sentry() {} - private static ThreadLocal currentHub = new ThreadLocal<>(); + private static final ThreadLocal currentHub = new ThreadLocal<>(); private static volatile IHub mainHub = NoOpHub.getInstance(); diff --git a/sentry-core/src/test/java/io/sentry/core/HubTest.kt b/sentry-core/src/test/java/io/sentry/core/HubTest.kt new file mode 100644 index 000000000..c36c81c46 --- /dev/null +++ b/sentry-core/src/test/java/io/sentry/core/HubTest.kt @@ -0,0 +1,36 @@ +package io.sentry.core + +import io.sentry.core.protocol.User +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNotSame + +class HubTest { + + @Test + fun `when cloning Scope it returns the same values`() { + val scope = Scope() + scope.extra["test"] = "test" + val breadcrumb = Breadcrumb() + breadcrumb.message = "test" + scope.breadcrumbs.add(breadcrumb) + scope.level = SentryLevel.DEBUG + scope.transaction = "test" + scope.fingerprint.add("test") + scope.tags["test"] = "test" + val user = User() + user.email = "a@a.com" + scope.user = user + + val clone = scope.clone() + assertNotNull(clone) + assertNotSame(scope, clone) + assertEquals("test", clone.extra["test"]) + assertEquals("test", clone.breadcrumbs[0].message) + assertEquals("test", scope.transaction) + assertEquals("test", scope.fingerprint[0]) + assertEquals("test", clone.tags["test"]) + assertEquals("a@a.com", clone.user.email) + } +} diff --git a/sentry-core/src/test/java/io/sentry/core/SentryEnvelopeTest.kt b/sentry-core/src/test/java/io/sentry/core/SentryEnvelopeTest.kt index 96cf57225..f59fe2c68 100644 --- a/sentry-core/src/test/java/io/sentry/core/SentryEnvelopeTest.kt +++ b/sentry-core/src/test/java/io/sentry/core/SentryEnvelopeTest.kt @@ -4,7 +4,6 @@ import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.whenever import java.io.InputStream -import java.nio.charset.Charset import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -12,7 +11,6 @@ import kotlin.test.assertNotNull import kotlin.test.assertNull class SentryEnvelopeTest { - private val UTF_8 = Charset.forName("UTF-8") @Test fun `deserialize sample envelope with event and two attachments`() { @@ -46,7 +44,7 @@ class SentryEnvelopeTest { @Test fun `when envelope is empty, reader throws illegal argument`() { val envelopeReader = EnvelopeReader() - var stream = mock() + val stream = mock() whenever(stream.read(any())).thenReturn(-1) val exception = assertFailsWith { envelopeReader.read(stream) } assertEquals("Empty stream.", exception.message) diff --git a/sentry-core/src/test/java/io/sentry/core/transport/HttpTransportTest.kt b/sentry-core/src/test/java/io/sentry/core/transport/HttpTransportTest.kt index a03fb7967..bf7b1324c 100644 --- a/sentry-core/src/test/java/io/sentry/core/transport/HttpTransportTest.kt +++ b/sentry-core/src/test/java/io/sentry/core/transport/HttpTransportTest.kt @@ -21,7 +21,7 @@ import kotlin.test.assertTrue class HttpTransportTest { private class Fixture { - var dsn = URI.create("http://key@localhost/proj").toURL() + val dsn: URL = URI.create("http://key@localhost/proj").toURL() val serializer = mock() var proxy: Proxy? = null var requestUpdater = IConnectionConfigurator {} From e81358bbc40127a84151970bbacf7fe3220b9b6b Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Wed, 23 Oct 2019 10:12:07 +0200 Subject: [PATCH 2/6] code review --- sentry-core/src/main/java/io/sentry/core/Hub.java | 15 +++++++++++++-- .../src/main/java/io/sentry/core/Scope.java | 9 ++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/Hub.java b/sentry-core/src/main/java/io/sentry/core/Hub.java index 014dccdae..70eff8380 100644 --- a/sentry-core/src/main/java/io/sentry/core/Hub.java +++ b/sentry-core/src/main/java/io/sentry/core/Hub.java @@ -1,5 +1,7 @@ package io.sentry.core; +import static io.sentry.core.ILogger.log; + import io.sentry.core.protocol.SentryId; import io.sentry.core.util.Nullable; import java.util.Deque; @@ -100,7 +102,16 @@ public SentryId getLastEventId() { public void pushScope() { StackItem item = stack.peek(); if (item != null) { - Scope clone = item.scope.clone(); + Scope clone = null; + try { + clone = item.scope.clone(); + } catch (CloneNotSupportedException e) { + log( + options.getLogger(), + SentryLevel.ERROR, + "An error has occurred when cloning a Scope", + e); + } if (clone != null) { StackItem newItem = new StackItem(item.client, clone); stack.push(newItem); @@ -143,7 +154,7 @@ public void configureScope(ScopeCallback callback) { public void bindClient(SentryClient client) { StackItem item = stack.peek(); if (item != null) { - item.client = client; + item.client = client != null ? client : NoOpSentryClient.getInstance(); } } diff --git a/sentry-core/src/main/java/io/sentry/core/Scope.java b/sentry-core/src/main/java/io/sentry/core/Scope.java index 415e66909..a61c2d1df 100644 --- a/sentry-core/src/main/java/io/sentry/core/Scope.java +++ b/sentry-core/src/main/java/io/sentry/core/Scope.java @@ -76,12 +76,7 @@ public void setExtra(String key, String value) { } @Override - protected Scope clone() { - Scope d = null; - try { - d = (Scope) super.clone(); - } catch (CloneNotSupportedException ignored) { - } // Won't happen - return d; + protected Scope clone() throws CloneNotSupportedException { + return (Scope) super.clone(); } } From f0c8a11baa9062419a18272491cc8393df1c2537 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Wed, 23 Oct 2019 16:40:22 +0200 Subject: [PATCH 3/6] logging error if stack head is null --- .../src/main/java/io/sentry/core/Hub.java | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/Hub.java b/sentry-core/src/main/java/io/sentry/core/Hub.java index 70eff8380..794e09551 100644 --- a/sentry-core/src/main/java/io/sentry/core/Hub.java +++ b/sentry-core/src/main/java/io/sentry/core/Hub.java @@ -51,8 +51,13 @@ public boolean isEnabled() { public SentryId captureEvent(SentryEvent event) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = item != null ? item.client.captureEvent(event, item.scope) : SentryId.EMPTY_ID; - this.lastEventId = sentryId; + if (item != null) { + sentryId = item.client.captureEvent(event, item.scope); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when captureEvent"); + sentryId = SentryId.EMPTY_ID; + } + this.lastEventId = event.getEventId(); return sentryId; } @@ -60,7 +65,12 @@ public SentryId captureEvent(SentryEvent event) { public SentryId captureMessage(String message) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = item != null ? item.client.captureMessage(message, item.scope) : SentryId.EMPTY_ID; + if (item != null) { + sentryId = item.client.captureMessage(message, item.scope); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when captureMessage"); + sentryId = SentryId.EMPTY_ID; + } this.lastEventId = sentryId; return sentryId; } @@ -69,8 +79,12 @@ public SentryId captureMessage(String message) { public SentryId captureException(Throwable throwable) { SentryId sentryId; StackItem item = stack.peek(); - sentryId = - item != null ? item.client.captureException(throwable, item.scope) : SentryId.EMPTY_ID; + if (item != null) { + sentryId = item.client.captureException(throwable, item.scope); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when captureException"); + sentryId = SentryId.EMPTY_ID; + } this.lastEventId = sentryId; return sentryId; } @@ -81,6 +95,8 @@ public void close() { StackItem item = stack.peek(); if (item != null) { item.client.close(); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when closing Hub"); } isEnabled = false; } @@ -90,6 +106,8 @@ public void addBreadcrumb(Breadcrumb breadcrumb) { StackItem item = stack.peek(); if (item != null) { item.scope.addBreadcrumb(breadcrumb); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when addBreadcrumb"); } } @@ -116,6 +134,8 @@ public void pushScope() { StackItem newItem = new StackItem(item.client, clone); stack.push(newItem); } + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when pushScope"); } } @@ -136,6 +156,8 @@ public void withScope(ScopeCallback callback) { StackItem item = stack.peek(); if (item != null) { callback.run(item.scope); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when withScope"); } } finally { popScope(); @@ -147,6 +169,8 @@ public void configureScope(ScopeCallback callback) { StackItem item = stack.peek(); if (item != null) { callback.run(item.scope); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when configureScope"); } } @@ -155,6 +179,8 @@ public void bindClient(SentryClient client) { StackItem item = stack.peek(); if (item != null) { item.client = client != null ? client : NoOpSentryClient.getInstance(); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when bindClient"); } } @@ -163,6 +189,8 @@ public void flush(long timeoutMills) { StackItem item = stack.peek(); if (item != null) { item.client.flush(timeoutMills); + } else { + log(options.getLogger(), SentryLevel.FATAL, "Stack peek was NULL when flush"); } } From 2b4b666e7d4bd4ab32be49c99c4f4f83c0853578 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Wed, 23 Oct 2019 22:23:08 +0200 Subject: [PATCH 4/6] test --- sentry-sample/build.gradle.kts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sentry-sample/build.gradle.kts b/sentry-sample/build.gradle.kts index 7ab219baf..ae3552f7f 100644 --- a/sentry-sample/build.gradle.kts +++ b/sentry-sample/build.gradle.kts @@ -41,6 +41,12 @@ android { jvmTarget = JavaVersion.VERSION_1_8.toString() } + tasks.forEach { + if (it.name.contains("signingConfigWriter")) { + it.enabled = false + } + } + } dependencies { From e05f8046369103123c7db8a8d47395d4f9c43ee9 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Wed, 23 Oct 2019 22:29:34 +0200 Subject: [PATCH 5/6] skipping signingConfigWriterDebugAndroidTest task which hangs on travis ci --- sentry-sample/build.gradle.kts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sentry-sample/build.gradle.kts b/sentry-sample/build.gradle.kts index ae3552f7f..573f56a1f 100644 --- a/sentry-sample/build.gradle.kts +++ b/sentry-sample/build.gradle.kts @@ -41,9 +41,10 @@ android { jvmTarget = JavaVersion.VERSION_1_8.toString() } - tasks.forEach { - if (it.name.contains("signingConfigWriter")) { - it.enabled = false + tasks.all { + if (this.name == "signingConfigWriterDebugAndroidTest") { + this.enabled = false + println("${this.name} is SKIPPED") } } From e33ee3aab9d77ddeae6bcdcbf45ec1da2e0b64f0 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Wed, 23 Oct 2019 23:23:54 +0200 Subject: [PATCH 6/6] commenting travis workaround --- sentry-sample/build.gradle.kts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sentry-sample/build.gradle.kts b/sentry-sample/build.gradle.kts index e69a39aba..0a85f59bc 100644 --- a/sentry-sample/build.gradle.kts +++ b/sentry-sample/build.gradle.kts @@ -45,12 +45,13 @@ android { } } - tasks.all { - if (this.name == "signingConfigWriterDebugAndroidTest") { - this.enabled = false - println("${this.name} is SKIPPED") - } - } + // if travis ci hangs again on this task, remove comments +// tasks.all { +// if (this.name == "signingConfigWriterDebugAndroidTest") { +// this.enabled = false +// println("${this.name} is SKIPPED") +// } +// } }