Removing the accessToken from MapboxHistoryReader constructor

[kmadsen]

The integration test verify_history_files_are_recorded_and_readable verifies the MapboxHistoryRecorder works for recording a history file and then the MapboxHistoryReader can read that recording.

We introduced the access token to ensure the new code contracts are satisfied #4652. But now we are passing an accessToken that can be useless when you are not using the HistoryEventSetRoute

Approaches

• Change the HistoryEventSetRoute.directionsRoute into a String?. This will force the developer to use DirectionsRoute.toJson(event.directionsRoute, accessToken) when they use the MapboxHistoryReader

+ Pro: Explicit for set route event
+ Pro: ~Allows for per route access tokens~ is not needed
- Con: Requires an extra mapping when using `MapboxHistoryReader`

• Make the MapboxHistoryReader.accessToken optional. When it is not given to the reader, the HistoryEventSetRoute is ignored.

+ Pro: Simpler
- Con: Silently does not work for set route

• Update the MapboxHistoryReader to have an editable Mapper + Builder member. And then update the MapboxHistoryMapper to have a builder, similar to ReplayHistoryMapper. The SetRouteHistoryMapper will have the required accessToken.

+ Pro: Explicit for the set route event
- Con: Feels like over-architecting the problem at this point

• Construct the MapboxHistoryReader from a MapboxNavigation extension. For example, mapboxNavigation.historyRecorder and mapboxNavigation.historyReader.

+ Pro: Hides the requirement to pass the access token
- Con: Cannot create a `MapboxHistoryReader` without a `MapboxNavigation` instantiation

• Something else

[kmadsen]

To answer some questions

"Why is the access token needed for the set route event?"

In order to create a DirectionsRoute from a json string, an access token is required in the navigation sdk. The reason this is required, is to support SDK features such as: RouteAlternatives, where a routes are requested from the current one; RouteRefresh, where the current route's traffic annotations are refreshed; and more.

"Why isn't the access token already inside the history set route events?"

More explanation here mapbox/mapbox-java#1267. We are not serializing access tokens to avoid unintentional leaks.

[LukasPaczos]

Thanks for starting the discussion 👍

From the listed options I prefer #1 for both simplicity and flexibility.

There's also another alternative:

1. use the token from NavigationOptions when making the route request
2. or change the MapboxNavigation#requetsRoutes(routeOptions) signature to MapboxNavigation#requetsRoutes(routeOptions, accessToken) (more flexible to target different datasets based on the token type, but we're slowly moving away from this paradigm so it might not be worth it, we can always expose it later if needed)

and remove the token from the RouteOptions altogether. We'd also have to expose MapboxDirections.Builder#accessToken to make this work.

The status quo of having a token in RouteOptions but also not wanting to serialize it for security reasons is becoming more and more of a pain.

What do you think @mapbox/navigation-android?

[kmadsen]

From the listed options I prefer #1 for both simplicity and flexibility.

Thanks and yeah I agree here. Ran with it over here #4667.

As for the other alternative of removing the accessToken from RouteOptions entirely. I am in favor of that because the MapboxNavigation.setRoute would be responsible for injecting the accessToken. From the options, I like getting the token from NavigationOptions because it doesn't seem like we need to support per route access tokens.

That approach would also revert the DirectionsRoute.fromJson(routeJson, accessToken)? If yes we have a few apis to update again. If no, we still need to go with one of the OP suggestions (and we agreed on #1 so lets go for it).

[LukasPaczos]

That approach would also revert the DirectionsRoute.fromJson(routeJson, accessToken)? If yes we have a few apis to update again.

That's right. Let me run with a prototype in mapbox-java and see the scope of changes. Tracking in mapbox/mapbox-java#1275.