Add try/catch around trying to access the connectivityManager APIs. #160

jkwatson · 2021-09-27T17:03:22Z

See https://issuetracker.google.com/issues/175055271 for the root Android bug.

breedx-splk

Looks good to me. I read in a book one time that a method should only do one thing, and that exception handling is also a thing. I think that means you might have cleaner code if you factored out the try body to a method...but dealer's choice.

breedx-splk · 2021-09-27T17:25:00Z

splunk-otel-android/src/test/java/com/splunk/rum/PostApi29NetworkDetectorTest.java

+
+        Network network = mock(Network.class);
+        when(connectivityManager.getActiveNetwork()).thenReturn(network);
+        when(connectivityManager.getNetworkCapabilities(network)).thenThrow(new SecurityException("bug"));


breedx-splk · 2021-09-27T17:25:13Z

splunk-otel-android/src/test/java/com/splunk/rum/SimpleNetworkDetectorTest.java

+        ConnectivityManager connectivityManager =
+                mock(ConnectivityManager.class);
+
+        when(connectivityManager.getActiveNetworkInfo()).thenThrow(new SecurityException("bug"));


breedx-splk · 2021-09-27T17:26:07Z

splunk-otel-android/src/main/java/com/splunk/rum/SimpleNetworkDetector.java

+                    return new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, activeNetwork.getSubtypeName());
+                case ConnectivityManager.TYPE_WIFI:  // Deprecated in API 28
+                    return new CurrentNetwork(NetworkState.TRANSPORT_WIFI, activeNetwork.getSubtypeName());
+                case ConnectivityManager.TYPE_VPN:


Outside the scope of this specific change, but static imports on these would be nice for readability.

I think when I wrote this originally, I wanted to make sure it was clear which of these things were Android's, and which were ours, and keeping the class name prefixes made that a little more clear to me.

breedx-splk · 2021-09-27T17:29:14Z

splunk-otel-android/src/main/java/com/splunk/rum/PostApi29NetworkDetector.java

+            if (capabilities == null) {
+                return NO_NETWORK;
+            }
+            String subType = null;


Unrelated to this change, but looks like the subType should be moved into the if scope block.

jkwatson · 2021-09-27T17:38:12Z

Looks good to me. I read in a book one time that a method should only do one thing, and that exception handling is also a thing. I think that means you might have cleaner code if you factored out the try body to a method...but dealer's choice.

Heh. That method would have to be named something silly, though, wouldn't it? Like "doDetectCurrentNetwork"? or "detectCurrentNetworkUnlessAndroidIsBuggy"? :)

jkwatson · 2021-09-27T17:39:23Z

Looks good to me. I read in a book one time that a method should only do one thing, and that exception handling is also a thing. I think that means you might have cleaner code if you factored out the try body to a method...but dealer's choice.

Heh. That method would have to be named something silly, though, wouldn't it? Like "doDetectCurrentNetwork"? or "detectCurrentNetworkUnlessAndroidIsBuggy"? :)

I guess the other option (probably better) would be to move the exception handling up a level into the ConnectionUtil call. I can make that change if you'd like.

jkwatson · 2021-09-27T19:21:42Z

splunk-otel-android/src/main/java/com/splunk/rum/PostApi29NetworkDetector.java

+import static com.splunk.rum.ConnectionUtil.NO_NETWORK;
+import static com.splunk.rum.ConnectionUtil.UNKNOWN_NETWORK;


no idea why IDEA decided to reorder these imports. 🤷🏽

breedx-splk · 2021-09-27T19:50:58Z

Looks good to me. I read in a book one time that a method should only do one thing, and that exception handling is also a thing. I think that means you might have cleaner code if you factored out the try body to a method...but dealer's choice.

Heh. That method would have to be named something silly, though, wouldn't it? Like "doDetectCurrentNetwork"? or "detectCurrentNetworkUnlessAndroidIsBuggy"? :)

I guess the other option (probably better) would be to move the exception handling up a level into the ConnectionUtil call. I can make that change if you'd like.

I think it's an improvement for sure. I would have gotten rid of the local variable there tho.

jkwatson · 2021-09-27T19:54:18Z

I think it's an improvement for sure. I would have gotten rid of the local variable there tho.

oh yeah, that's totally a good change. I had missed altogether that it wasn't even necessary.

Add try/catch around trying to access the connectivityManager APIs.

640b994

See https://issuetracker.google.com/issues/175055271 for the root Android bug.

jkwatson requested review from a team as code owners September 27, 2021 17:03

breedx-splk approved these changes Sep 27, 2021

View reviewed changes

a small tweak from PR review

b8f9ace

jkwatson added 3 commits September 27, 2021 11:35

catch the exception one layer higher for simplicity and clarity.

ad67cc5

small tweak

bc95ca6

small tweak 2

25f23ee

jkwatson commented Sep 27, 2021

View reviewed changes

remove unneeded local var

c4bdcdd

jkwatson merged commit 1fd6ea5 into main Sep 27, 2021

jkwatson deleted the guard_against_network_security_exception branch September 27, 2021 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add try/catch around trying to access the connectivityManager APIs. #160

Add try/catch around trying to access the connectivityManager APIs. #160

jkwatson commented Sep 27, 2021

breedx-splk left a comment

breedx-splk Sep 27, 2021

breedx-splk Sep 27, 2021

breedx-splk Sep 27, 2021

jkwatson Sep 27, 2021

breedx-splk Sep 27, 2021

jkwatson commented Sep 27, 2021

jkwatson commented Sep 27, 2021

jkwatson Sep 27, 2021

breedx-splk commented Sep 27, 2021

jkwatson commented Sep 27, 2021 •

edited

Loading

		import static com.splunk.rum.ConnectionUtil.NO_NETWORK;
		import static com.splunk.rum.ConnectionUtil.UNKNOWN_NETWORK;

Add try/catch around trying to access the connectivityManager APIs. #160

Add try/catch around trying to access the connectivityManager APIs. #160

Conversation

jkwatson commented Sep 27, 2021

breedx-splk left a comment

Choose a reason for hiding this comment

breedx-splk Sep 27, 2021

Choose a reason for hiding this comment

breedx-splk Sep 27, 2021

Choose a reason for hiding this comment

breedx-splk Sep 27, 2021

Choose a reason for hiding this comment

jkwatson Sep 27, 2021

Choose a reason for hiding this comment

breedx-splk Sep 27, 2021

Choose a reason for hiding this comment

jkwatson commented Sep 27, 2021

jkwatson commented Sep 27, 2021

jkwatson Sep 27, 2021

Choose a reason for hiding this comment

breedx-splk commented Sep 27, 2021

jkwatson commented Sep 27, 2021 • edited Loading

jkwatson commented Sep 27, 2021 •

edited

Loading