Skip to content
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.

Sign up for GitHub

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

Osrm support #198

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Osrm support #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0f648aa
[OSRM] accept other hosts than mapbox (OSRM server)
Oct 13, 2017
f9ce0de
Merge commit 'cd2c244160dae4dd94bc8fdc214116d5324b3564' into OSRMSupport
Oct 27, 2017
9bb1551
better custom host string handling
Oct 27, 2017
f176ef0
Merge branch 'master' into OSRMSupport
Nov 3, 2017
2f0d06b
correct mapbox v4 api support
Nov 3, 2017
12e2423
review: correct braket place
Nov 3, 2017
d2cce68
review: reposition the else
Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 45 additions & 15 deletions MapboxDirections/MBDirections.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ public let MBDirectionsErrorDomain = "MBDirectionsErrorDomain"
/// The Mapbox access token specified in the main application bundle’s Info.plist.
let defaultAccessToken = Bundle.main.object(forInfoDictionaryKey: "MGLMapboxAccessToken") as? String

/// The Mapbox public host
internal let mapboxHost = "api.mapbox.com"

/// The user agent string for any HTTP requests performed directly within this library.
let userAgent: String = {
var components: [String] = []
Expand Down Expand Up @@ -114,12 +117,12 @@ open class Directions: NSObject {
*/
@objc(sharedDirections)
open static let shared = Directions(accessToken: nil)

/// The API endpoint to request the directions from.
internal var apiEndpoint: URL

/// The Mapbox access token to associate the request with.
internal let accessToken: String
internal var accessToken: String?

/**
Initializes a newly created directions object with an optional access token and host.
Expand All @@ -128,14 +131,26 @@ open class Directions: NSObject {
- parameter host: An optional hostname to the server API. The [Mapbox Directions API](https://www.mapbox.com/api-documentation/?language=Swift#directions) endpoint is used by default.
*/
public init(accessToken: String?, host: String?) {
let accessToken = accessToken ?? defaultAccessToken
assert(accessToken != nil && !accessToken!.isEmpty, "A Mapbox access token is required. Go to <https://www.mapbox.com/studio/account/tokens/>. In Info.plist, set the MGLMapboxAccessToken key to your access token, or use the Directions(accessToken:host:) initializer.")

self.accessToken = accessToken!

var baseURLComponents = URLComponents()
baseURLComponents.scheme = "https"
baseURLComponents.host = host ?? "api.mapbox.com"
var baseURLComponents: URLComponents
if host == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be cleaner to do a guard let here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite disagree. It's a pure if else. There's nothing to guard against.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're guarding against assuming host is not nil and also this assert. IMO, asserting is great time to throw in a guard.

Copy link
Contributor

@vincethecoder vincethecoder Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomainQuidet upon reviewing the code, I understand your intent. Probably it would be more clear if you safely unwrapped the host variable like so:

if let host = host {
   var urlString = host
   if urlString.hasPrefix("http") == false {
      urlString = "http://\(urlString)"
   }
   
   assert(URL(string: urlString) != nil, "A valid custom host must be provided (ex: my_host.com or full http://my_host.com:8080")
   if let url = URL(string: urlString), let urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false) {
      baseURLComponents = urlComponents
  }
} else {
   let accessToken = accessToken ?? defaultAccessToken
   assert(accessToken != nil && !accessToken!.isEmpty, "A Mapbox access token is required. Go to <https://www.mapbox.com/studio/account/tokens/>. In Info.plist, set the MGLMapboxAccessToken key to your access token, or use the Directions(accessToken:host:) initializer.") 
   self.accessToken = accessToken
   baseURLComponents = URLComponents()
   baseURLComponents.scheme = "https"
   baseURLComponents.host = mapboxHost
}

let accessToken = accessToken ?? defaultAccessToken
assert(accessToken != nil && !accessToken!.isEmpty, "A Mapbox access token is required. Go to <https://www.mapbox.com/studio/account/tokens/>. In Info.plist, set the MGLMapboxAccessToken key to your access token, or use the Directions(accessToken:host:) initializer.")

self.accessToken = accessToken
baseURLComponents = URLComponents()
baseURLComponents.scheme = "https"
baseURLComponents.host = mapboxHost
}
else {
var urlString = host!
if urlString.hasPrefix("http") == false {
urlString = "http://\(urlString)"
}
let url = URL(string: urlString)
assert(url != nil, "A valid custom host must be provided (ex: my_host.com or full http://my_host.com:8080")
baseURLComponents = URLComponents(url: url!, resolvingAgainstBaseURL: false)!
}

self.apiEndpoint = baseURLComponents.url!
}

Expand Down Expand Up @@ -229,11 +244,26 @@ open class Directions: NSObject {
*/
@objc(URLForCalculatingDirectionsWithOptions:)
open func url(forCalculating options: RouteOptions) -> URL {
let params = options.params + [
URLQueryItem(name: "access_token", value: accessToken),
]

let unparameterizedURL = URL(string: options.path, relativeTo: apiEndpoint)!
let isMapboxHost = apiEndpoint.host == mapboxHost
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky. We use access tokens on other hosts that are not equal to mapboxHost. Perhaps we can make mapboxHost public so we can override this when using other mapbox hostnames.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you wish. The goal here is to know when we are using mapbox host.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The access_token parameter is unnecessary when talking to an OSRM endpoint, but it should already be easy enough to set the access token to something bogus, as the unit tests do. If the OSRM endpoint is erroring out because of that parameter, I guess we could’ve made Credentials(accessToken:) take an empty string as a signal to not add the access_token parameter to the URL. 🤷‍♂️

var params = options.params
if isMapboxHost {
params += [
URLQueryItem(name: "access_token", value: accessToken),
]
}

let path: String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be a var?

if isMapboxHost {
path = options.path
}
else
{
assert(!options.queries.isEmpty, "No query")
let queryComponent = options.queries.joined(separator: ";")
path = "route/v1/\(options.profileIdentifier.rawValue)/\(queryComponent)"
}

let unparameterizedURL = URL(string: path, relativeTo: apiEndpoint)!
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be properly unwrapped.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not changed this unparameterizedURL unwrap... It is the original code.

var components = URLComponents(url: unparameterizedURL, resolvingAgainstBaseURL: true)!
components.queryItems = params
return components.url!
Expand Down
8 changes: 4 additions & 4 deletions MapboxDirections/MBRouteOptions.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#import "MBRouteOptions.h"


MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierAutomobile = @"mapbox/driving";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierAutomobileAvoidingTraffic = @"mapbox/driving-traffic";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierCycling = @"mapbox/cycling";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierWalking = @"mapbox/walking";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierAutomobile = @"driving";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierAutomobileAvoidingTraffic = @"driving-traffic";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierCycling = @"cycling";
MBDirectionsProfileIdentifier const MBDirectionsProfileIdentifierWalking = @"walking";
4 changes: 2 additions & 2 deletions MapboxDirections/MBRouteOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ open class RouteOptions: NSObject, NSSecureCoding {
assert(!queries.isEmpty, "No query")

let queryComponent = queries.joined(separator: ";")
return "directions/v5/\(profileIdentifier.rawValue)/\(queryComponent).json"
return "directions/v5/mapbox/\(profileIdentifier.rawValue)/\(queryComponent).json"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we actually pull this out into a public variable profilePrefix?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I have to do it ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomainQuidet yes, sometimes this is not equal to mapbox.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapbox still needs to be pulled out into a public property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be unnecessary, because you can initialize a ProfileIdentifier to hold any arbitrary identifier using ProfileIdentifier(rawValue:). If this is inconvenient, an application can always extend ProfileIdentifier with additional constants as desired.

}

/**
Expand Down Expand Up @@ -531,7 +531,7 @@ open class RouteOptionsV4: RouteOptions {

let profileIdentifier = self.profileIdentifier.rawValue.replacingOccurrences(of: "/", with: ".")
let queryComponent = queries.joined(separator: ";")
return "v4/directions/\(profileIdentifier)/\(queryComponent).json"
return "v4/directions/mapbox.\(profileIdentifier)/\(queryComponent).json"
}

override var params: [URLQueryItem] {
Expand Down