-
Notifications
You must be signed in to change notification settings - Fork 93
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
Accept request URL in lieu of configuration file #728
Conversation
Force-unwrapping the access token potentially crashes if it isn’t set in the environment. Instead, gently try to get the access token and return an error code if it’s unavailable.
Replaced the --config option with a positional argument that is either a path to the JSON configuration file, as before, or the URL to a Directions or Map Matching API request. Expand a tilde in the configuration file path when validating the arguments. Removed a passage from the documentation that refers to a not-yet-implemented feature.
} | ||
|
||
let hostURL: URL? | ||
if let host = ProcessInfo.processInfo.environment["MAPBOX_HOST"] ?? |
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.
The name of this environment variable is consistent with the Mapbox Python SDK, which similarly reads credentials from the environment. It differs from the MGLMapboxAPIBaseURL
Info.plist key that applications would set, but the naming conventions for environment variables are different than for Info.plist keys anyways.
struct ProcessingOptions: ParsableArguments { | ||
|
||
@Option(name: [.short, .customLong("input")], help: "[Optional] Filepath to the input JSON. If no filepath provided - will fall back to Directions API request using locations in config file.") | ||
var inputPath: String? | ||
|
||
@Option(name: [.short, .customLong("config")], help: "Filepath to the JSON, containing serialized Options data.") | ||
var configPath: String | ||
@Argument(help: "Path to a JSON file containing serialized RouteOptions or MatchOptions properties, or the full URL of a Mapbox Directions API or Mapbox Map Matching API request.") |
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.
This is kind of unwieldy as an argument description. A possible future improvement would be to accept multiple configurations, perhaps a mix of files and URLs, and merge them somehow.
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.
will you document that possible future improvement?
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.
Let’s track it as tail work in #729.
urlWithAccessToken.queryItems = (urlWithAccessToken.queryItems ?? []) + [.init(name: "access_token", value: self.credentials.accessToken)] | ||
let credentials = Credentials(requestURL: urlWithAccessToken.url!) |
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.
This was pretty annoying: Credentials(url:)
calls the required initializer Credentials(accessToken:host:)
. If the URL doesn’t have the access_token
query parameter set, then the required initializer gets a nil
access token, resulting in a precondition failure:
precondition(accessToken != nil && !accessToken!.isEmpty, "A Mapbox access token is required. Go to <https://account.mapbox.com/access-tokens/>. In Info.plist, set the MBXAccessToken key to your access token, or use the Directions(accessToken:host:) initializer.") |
The code here has no opportunity to fall back to the access token from the environment, so instead it has to proactively fall back inside the URL before initializing the Credentials object.
directionsOptions = try decoder.decode(OptionsType.self, from: configData) | ||
} else if let url = URL(string: options.config) { | ||
// Try to convert the URL to an options object. | ||
if let parsedOptions = (RouteOptions(url: url) ?? MatchOptions(url: url)) as? OptionsType { |
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.
This is why DirectionsOptions(url:)
has to return nil
when given a URL whose path conflicts with the abridgedPath
property. If it doesn’t return nil
, then either RouteOptions and MatchOptions would be equally likely to initialize based on the same URL, mooting the coalescing operator.
Maybe it would be cleaner to nix the OptionsType
generic parameter in favor of an enumeration with associated values that gets passed into this method, since the two APIs aren’t all that different. But I’ll leave it for now in case another API needs this kind of flexibility.
Overhauled the command line tool’s configuration option. Instead of the
--config
option that takes a JSON configuration file path, the tool now takes a single positional argument that can either be a JSON configuration file path or a Directions API or Map Matching API request URL. When you pass in a request URL, the URL is round-tripped through a DirectionsOptions object back to a URL, exercising the entire typical code path (and then some).But if you do decide to pass in a JSON configuration file instead of a URL, the command line tool now respects the
MAPBOX_HOST
environment variable alongside theMAPBOX_ACCESS_TOKEN
environment variable. An access token is scoped to a single API endpoint, so this configuration option makes it possible to run the tool against a proxy or staging server for testing purposes.RouteOptions(url:)
now returnsnil
if given a Mapbox Map Matching API request URL, andMatchOptions(url:)
returnsnil
if given a Mapbox Directions API request URL. Previously, you could mix and match URLs to convert between RouteOptions and MatchOptions. Now, you’ll need to use JSONEncoder and JSONDecoder as well:#564 would unblock a more convenient conversion workflow.
Some tangential changes along the way:
MAPBOX_ACCESS_TOKEN
environment variable is unset, the tool exits with an error code instead of crashing.--waypoints
and--url
options from the command line tool’s documentation that probably came from Convert from Directions API URL to RouteOptions #580.Fixes #692 and fixes #724. Supersedes #580. Depends on #727.
/cc @mapbox/navigation-ios @purew