From 9b6d055ece369d604141bcafd5fbcc4b05ff9d0d Mon Sep 17 00:00:00 2001 From: Nikita Korshak Date: Tue, 14 Sep 2021 12:31:13 +0300 Subject: [PATCH] review fixes --- .../route/internal/RouterWrapper.kt | 123 +++++++++++------- .../route/internal/RouterWrapperTests.kt | 97 ++++++++++++++ 2 files changed, 172 insertions(+), 48 deletions(-) diff --git a/libnavigation-router/src/main/java/com/mapbox/navigation/route/internal/RouterWrapper.kt b/libnavigation-router/src/main/java/com/mapbox/navigation/route/internal/RouterWrapper.kt index 785c6a827c9..9e95d95e71f 100644 --- a/libnavigation-router/src/main/java/com/mapbox/navigation/route/internal/RouterWrapper.kt +++ b/libnavigation-router/src/main/java/com/mapbox/navigation/route/internal/RouterWrapper.kt @@ -22,6 +22,7 @@ import com.mapbox.navigation.route.internal.util.httpUrl import com.mapbox.navigation.route.internal.util.redactQueryParam import com.mapbox.navigation.route.onboard.OfflineRoute import com.mapbox.navigation.utils.internal.ThreadController +import com.mapbox.navigation.utils.internal.logI import com.mapbox.navigation.utils.internal.logW import com.mapbox.navigator.RouteRefreshOptions import com.mapbox.navigator.RouterInterface @@ -50,28 +51,11 @@ class RouterWrapper( return router.getRoute(offlineRouteUrl) { result, origin -> val urlWithoutToken = URL(offlineRouteUrl.redactQueryParam(ACCESS_TOKEN_QUERY_PARAM)) - - mainJobControl.scope.launch { - if (result.isValue) { - val routes = parseDirectionsResponse(result.value!!, routeOptions) - if (routes.isNullOrEmpty()) { - callback.onFailure( - listOf( - RouterFailure( - urlWithoutToken, - origin.mapToSdkRouteOrigin(), - ROUTES_LIST_EMPTY - ) - ), - routeOptions - ) - } else { - callback.onRoutesReady(routes, origin.mapToSdkRouteOrigin()) - } - } else { - result.error!!.run { - if (error == REQUEST_CANCELLED) { - logW( + result.fold( + { + mainJobControl.scope.launch { + if (it.error == REQUEST_CANCELLED) { + logI( TAG, Message( """ @@ -87,8 +71,8 @@ class RouterWrapper( RouterFailure( url = urlWithoutToken, routerOrigin = origin.mapToSdkRouteOrigin(), - message = error, - code = code + message = it.error, + code = it.code ) ) @@ -105,8 +89,27 @@ class RouterWrapper( callback.onFailure(failureReasons, routeOptions) } } + }, + { + mainJobControl.scope.launch { + val routes = parseDirectionsResponse(result.value!!, routeOptions) + if (routes.isNullOrEmpty()) { + callback.onFailure( + listOf( + RouterFailure( + urlWithoutToken, + origin.mapToSdkRouteOrigin(), + ROUTES_LIST_EMPTY + ) + ), + routeOptions + ) + } else { + callback.onRoutesReady(routes, origin.mapToSdkRouteOrigin()) + } + } } - } + ) } } @@ -115,36 +118,46 @@ class RouterWrapper( legIndex: Int, callback: RouteRefreshCallback ): Long { - val routeOptions = route.routeOptions()!! + val routeOptions = route.routeOptions() + val requestUuid = route.requestUuid() + val routeIndex = route.routeIndex()?.toIntOrNull() + if (routeOptions == null || requestUuid == null || routeIndex == null) { + val errorMessage = + """ + Route refresh failed because of a null param: + routeOptions = $routeOptions + requestUuid = $requestUuid + routeIndex = $routeIndex + """.trimIndent() + + logW(TAG, Message(errorMessage)) + + callback.onError( + RouteRefreshError("Route refresh failed", Exception(errorMessage)) + ) + + return REQUEST_FAILURE + } + val userAndProfile = "${routeOptions.user()}/${routeOptions.profile()}" val refreshOptions = RouteRefreshOptions( - route.requestUuid() ?: "", - route.routeIndex()?.toIntOrNull() ?: 0, + requestUuid, + routeIndex, legIndex, RoutingProfile(userAndProfile), ) return router.getRouteRefresh(refreshOptions, route.toJson()) { result, _ -> - mainJobControl.scope.launch { - if (result.isValue) { - val refreshedRoute = - withContext(ThreadController.IODispatcher) { - DirectionsRoute.fromJson( - result.value!!, - routeOptions, - route.requestUuid() - ) - } - callback.onRefresh(refreshedRoute) - } else { - result.error!!.run { + result.fold( + { + mainJobControl.scope.launch { val errorMessage = """ - Route refresh failed. - error = $error - legIndex = $legIndex - code = $code - requestId = $requestId + Route refresh failed. + error = ${it.error} + legIndex = $legIndex + code = ${it.code} + requestId = ${it.requestId} """.trimIndent() logW(TAG, Message(errorMessage)) @@ -153,8 +166,21 @@ class RouterWrapper( RouteRefreshError("Route refresh failed", Exception(errorMessage)) ) } + }, + { + mainJobControl.scope.launch { + val refreshedRoute = + withContext(ThreadController.IODispatcher) { + DirectionsRoute.fromJson( + result.value!!, + routeOptions, + route.requestUuid() + ) + } + callback.onRefresh(refreshedRoute) + } } - } + ) } } @@ -192,9 +218,10 @@ class RouterWrapper( } private companion object { - private val TAG = Tag("RouterWrapper") + private val TAG = Tag("MbxRouterWrapper") private const val UUID = "uuid" private const val REQUEST_CANCELLED = "Cancelled" private const val ROUTES_LIST_EMPTY = "routes list is empty" + private const val REQUEST_FAILURE = -1L } } diff --git a/libnavigation-router/src/test/java/com/mapbox/navigation/route/internal/RouterWrapperTests.kt b/libnavigation-router/src/test/java/com/mapbox/navigation/route/internal/RouterWrapperTests.kt index 4506aaa5173..79a767fc4fa 100644 --- a/libnavigation-router/src/test/java/com/mapbox/navigation/route/internal/RouterWrapperTests.kt +++ b/libnavigation-router/src/test/java/com/mapbox/navigation/route/internal/RouterWrapperTests.kt @@ -78,6 +78,11 @@ class RouterWrapperTests { every { isError } returns false every { value } returns SUCCESS_RESPONSE every { error } returns null + + val valueSlot = slot>() + every { fold(any(), capture(valueSlot)) } answers { + valueSlot.captured.invoke(this@mockk.value!!) + } } private val routerResultFailure: Expected = mockk { every { isValue } returns false @@ -88,6 +93,11 @@ class RouterWrapperTests { FAILURE_CODE, REQUEST_ID ) + + val errorSlot = slot>() + every { fold(capture(errorSlot), any()) } answers { + errorSlot.captured.invoke(this@mockk.error!!) + } } private val routerResultCancelled: Expected = mockk { every { isValue } returns false @@ -98,18 +108,33 @@ class RouterWrapperTests { FAILURE_CODE, REQUEST_ID ) + + val errorSlot = slot>() + every { fold(capture(errorSlot), any()) } answers { + errorSlot.captured.invoke(this@mockk.error!!) + } } private val routerResultSuccessEmptyRoutes: Expected = mockk { every { isValue } returns true every { isError } returns false every { value } returns SUCCESS_RESPONSE_EMPTY_ROUTES every { error } returns null + + val valueSlot = slot>() + every { fold(any(), capture(valueSlot)) } answers { + valueSlot.captured.invoke(this@mockk.value!!) + } } private val routerRefreshSuccess: Expected = mockk { every { isValue } returns true every { isError } returns false every { value } returns REFRESHED_ROUTE every { error } returns null + + val valueSlot = slot>() + every { fold(any(), capture(valueSlot)) } answers { + valueSlot.captured.invoke(this@mockk.value!!) + } } private val nativeOriginOnline: RouterOrigin = RouterOrigin.ONLINE private val nativeOriginOnboard: RouterOrigin = RouterOrigin.ONBOARD @@ -327,6 +352,78 @@ class RouterWrapperTests { verify(exactly = 1) { router.cancelRequest(refreshIdTwo) } } + @Test + fun `route refresh fails with null routeOptions`() { + val route: DirectionsRoute = mockk(relaxed = true) + every { route.requestUuid() } returns UUID + every { route.routeIndex() } returns "1" + every { route.routeOptions() } returns null + + routerWrapper.getRouteRefresh(route, 0, routerRefreshCallback) + + val expectedErrorMessage = + """ + Route refresh failed because of a null param: + routeOptions = null + requestUuid = $UUID + routeIndex = 1 + """.trimIndent() + + val errorSlot = slot() + verify(exactly = 1) { routerRefreshCallback.onError(capture(errorSlot)) } + verify(exactly = 0) { router.getRouteRefresh(any(), any(), any()) } + assertEquals("Route refresh failed", errorSlot.captured.message) + assertEquals(expectedErrorMessage, errorSlot.captured.throwable?.message) + } + + @Test + fun `route refresh fails with null requestUuid`() { + val route: DirectionsRoute = mockk(relaxed = true) + every { route.requestUuid() } returns null + every { route.routeIndex() } returns "1" + every { route.routeOptions() } returns routerOptions + + routerWrapper.getRouteRefresh(route, 0, routerRefreshCallback) + + val expectedErrorMessage = + """ + Route refresh failed because of a null param: + routeOptions = $routerOptions + requestUuid = null + routeIndex = 1 + """.trimIndent() + + val errorSlot = slot() + verify(exactly = 1) { routerRefreshCallback.onError(capture(errorSlot)) } + verify(exactly = 0) { router.getRouteRefresh(any(), any(), any()) } + assertEquals("Route refresh failed", errorSlot.captured.message) + assertEquals(expectedErrorMessage, errorSlot.captured.throwable?.message) + } + + @Test + fun `route refresh fails with null routeIndex`() { + val route: DirectionsRoute = mockk(relaxed = true) + every { route.requestUuid() } returns UUID + every { route.routeIndex() } returns null + every { route.routeOptions() } returns routerOptions + + routerWrapper.getRouteRefresh(route, 0, routerRefreshCallback) + + val expectedErrorMessage = + """ + Route refresh failed because of a null param: + routeOptions = $routerOptions + requestUuid = $UUID + routeIndex = null + """.trimIndent() + + val errorSlot = slot() + verify(exactly = 1) { routerRefreshCallback.onError(capture(errorSlot)) } + verify(exactly = 0) { router.getRouteRefresh(any(), any(), any()) } + assertEquals("Route refresh failed", errorSlot.captured.message) + assertEquals(expectedErrorMessage, errorSlot.captured.throwable?.message) + } + @Ignore("Remove after https://github.com/mapbox/mapbox-navigation-native/issues/4016") @Test fun `route refresh set right params`() {