-
-
Notifications
You must be signed in to change notification settings - Fork 354
Fail gracefully when the provided json is not valid #4474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
50ca0f5
b7adc29
66fc07e
ba6ca21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| package io.sentry.react | ||
|
|
||
| import android.content.Context | ||
| import com.facebook.react.bridge.JavaOnlyMap | ||
| import com.facebook.react.bridge.ReadableMap | ||
| import io.sentry.ILogger | ||
| import io.sentry.Sentry.OptionsConfiguration | ||
| import io.sentry.SentryLevel | ||
| import io.sentry.android.core.SentryAndroidOptions | ||
| import org.json.JSONObject | ||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertThrows | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.junit.runners.JUnit4 | ||
| import org.mockito.ArgumentMatchers.contains | ||
| import org.mockito.ArgumentMatchers.eq | ||
| import org.mockito.MockedStatic | ||
| import org.mockito.Mockito.mock | ||
| import org.mockito.Mockito.mockStatic | ||
| import org.mockito.Mockito.verify | ||
| import org.mockito.MockitoAnnotations | ||
|
|
||
| @RunWith(JUnit4::class) | ||
| class RNSentrySDKTest { | ||
| private val configurationFile = "sentry.options.json" | ||
|
|
||
| private lateinit var mockLogger: ILogger | ||
| private lateinit var mockContext: Context | ||
| private lateinit var mockConfiguration: OptionsConfiguration<SentryAndroidOptions> | ||
| private lateinit var mockedRNSentryStart: MockedStatic<RNSentryStart> | ||
| private lateinit var mockedRNSentryJsonUtils: MockedStatic<RNSentryJsonUtils> | ||
|
|
||
| @Before | ||
| fun setUp() { | ||
| MockitoAnnotations.openMocks(this) | ||
| mockLogger = mock(ILogger::class.java) | ||
| mockContext = mock(Context::class.java) | ||
| mockConfiguration = mock(OptionsConfiguration::class.java) as OptionsConfiguration<SentryAndroidOptions> | ||
| mockedRNSentryStart = mockStatic(RNSentryStart::class.java) | ||
| mockedRNSentryJsonUtils = mockStatic(RNSentryJsonUtils::class.java) | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| mockedRNSentryStart.close() | ||
| mockedRNSentryJsonUtils.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `init with passed configuration callback when no valid json file is provided`() { | ||
| val mockJsonObject = null | ||
| mockedRNSentryJsonUtils | ||
| .`when`<JSONObject> { | ||
| RNSentryJsonUtils.getOptionsFromConfigurationFile(mockContext, configurationFile, mockLogger) | ||
| }.thenReturn(mockJsonObject) | ||
| RNSentrySDK.init(mockContext, mockConfiguration, mockLogger) | ||
|
|
||
| verify(mockLogger).log( | ||
| eq(SentryLevel.WARNING), | ||
| contains("Failed to load configuration file(sentry.options.json), starting with configuration callback."), | ||
| ) | ||
|
|
||
| mockedRNSentryStart.verify { | ||
| RNSentryStart.startWithConfiguration(mockContext, mockConfiguration) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `init with passed configuration callback when no valid readable map is created`() { | ||
| val mockJsonObject = JSONObject() | ||
| mockedRNSentryJsonUtils | ||
| .`when`<JSONObject> { | ||
| RNSentryJsonUtils.getOptionsFromConfigurationFile(mockContext, configurationFile, mockLogger) | ||
| }.thenReturn(mockJsonObject) | ||
| val mockReadableMap = null | ||
| mockedRNSentryJsonUtils | ||
| .`when`<ReadableMap> { | ||
| RNSentryJsonUtils.jsonObjectToReadableMap( | ||
| mockJsonObject, | ||
| ) | ||
| }.thenReturn(mockReadableMap) | ||
|
|
||
| RNSentrySDK.init(mockContext, mockConfiguration, mockLogger) | ||
|
|
||
| verify(mockLogger).log( | ||
| eq(SentryLevel.WARNING), | ||
| contains("Failed to load configuration file(sentry.options.json), starting with configuration callback."), | ||
| ) | ||
| mockedRNSentryStart.verify { | ||
| RNSentryStart.startWithConfiguration(mockContext, mockConfiguration) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `init with the json file and the passed configuration when a valid json is provided`() { | ||
| val mockJsonObject = JSONObject() | ||
| mockedRNSentryJsonUtils | ||
| .`when`<JSONObject> { | ||
| RNSentryJsonUtils.getOptionsFromConfigurationFile(mockContext, configurationFile, mockLogger) | ||
| }.thenReturn(mockJsonObject) | ||
| val mockReadableMap = | ||
| JavaOnlyMap.of( | ||
| "dsn", | ||
| "https://[email protected]/1234567", | ||
| ) | ||
|
|
||
| mockedRNSentryJsonUtils | ||
| .`when`<ReadableMap> { | ||
| RNSentryJsonUtils.jsonObjectToReadableMap( | ||
| mockJsonObject, | ||
| ) | ||
| }.thenReturn(mockReadableMap) | ||
|
|
||
| RNSentrySDK.init(mockContext, mockConfiguration, mockLogger) | ||
|
|
||
| mockedRNSentryStart.verify { | ||
| RNSentryStart.startWithOptions(mockContext, mockReadableMap, mockConfiguration, null, mockLogger) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `fails with an error when there is an unhandled exception in initialisation`() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is a bit confusing to me, as the |
||
| mockedRNSentryJsonUtils | ||
| .`when`<JSONObject> { | ||
| RNSentryJsonUtils.getOptionsFromConfigurationFile(mockContext, configurationFile, mockLogger) | ||
| }.thenThrow(RuntimeException("Test exception")) | ||
|
|
||
| val exception = | ||
| assertThrows(RuntimeException::class.java) { | ||
| RNSentrySDK.init(mockContext, mockConfiguration, mockLogger) | ||
| } | ||
|
|
||
| assertEquals("Failed to initialize Sentry's React Native SDK", exception.message) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,28 +25,51 @@ private RNSentrySDK() { | |
| * | ||
| * @param context Android Context | ||
| * @param configuration configuration options | ||
| * @param logger logger | ||
| */ | ||
| public static void init( | ||
| @NotNull final Context context, | ||
| @NotNull Sentry.OptionsConfiguration<SentryAndroidOptions> configuration) { | ||
| @NotNull Sentry.OptionsConfiguration<SentryAndroidOptions> configuration, | ||
| @NotNull ILogger logger) { | ||
| try { | ||
| JSONObject jsonObject = | ||
| RNSentryJsonUtils.getOptionsFromConfigurationFile(context, CONFIGURATION_FILE, logger); | ||
| ReadableMap rnOptions = RNSentryJsonUtils.jsonObjectToReadableMap(jsonObject); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the SDK won't initialize when the jsonObject.get throws. Can we add test for it and avoid the rethrow. |
||
| if (rnOptions == null) { | ||
| logger.log( | ||
| SentryLevel.WARNING, | ||
| "Failed to load configuration file(" | ||
| + CONFIGURATION_FILE | ||
| + "), starting with configuration callback."); | ||
|
Comment on lines
+39
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this log is not necessary/is duplicate as the same information will be logged out in |
||
| RNSentryStart.startWithConfiguration(context, configuration); | ||
| return; | ||
| } | ||
| RNSentryStart.startWithOptions(context, rnOptions, configuration, null, logger); | ||
| } catch (Exception e) { | ||
| logger.log( | ||
| SentryLevel.ERROR, "Failed to start Sentry with options from configuration file.", e); | ||
| throw new RuntimeException(e); | ||
| throw new RuntimeException("Failed to initialize Sentry's React Native SDK", e); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Start the Native Android SDK with the provided options | ||
| * | ||
| * @param context Android Context | ||
| * @param configuration configuration options | ||
| */ | ||
| public static void init( | ||
| @NotNull final Context context, | ||
| @NotNull Sentry.OptionsConfiguration<SentryAndroidOptions> configuration) { | ||
| init(context, configuration, logger); | ||
| } | ||
|
|
||
| /** | ||
| * Start the Native Android SDK with options from `sentry.options.json` configuration file | ||
| * | ||
| * @param context Android Context | ||
| */ | ||
| public static void init(@NotNull final Context context) { | ||
| init(context, options -> {}); | ||
| init(context, options -> {}, logger); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add integration test which won't use the mocks (maybe just to supply the file) and initializes the SDK?