Osrm support #198

RomainQuidet · 2017-10-27T14:31:30Z

I just updated the directions init to really support any custom host. This allows us to use any OSRM server.

RomainQuidet · 2017-10-27T14:32:11Z

this is for issue #121

RomainQuidet · 2017-11-03T14:27:46Z

any clue why bitrise fails ? It compiles on Xcode 9.1 / swift 3.2

bsudekum · 2017-11-03T14:57:39Z

It looks like requests are still being made with a Mapbox access token (or it's expecting one to be there):

Selected tests
Test Suite MapboxDirectionsTests.xctest started
V4Tests
    ✗ testPolyline, XCTAssertNil failed: "Error Domain=MBDirectionsErrorDomain Code=-1 "Not Authorized - Invalid Token" UserInfo={NSLocalizedFailureReason=Not Authorized - Invalid Token}" - Error: Error Domain=MBDirectionsErrorDomain Code=-1 "Not Authorized - Invalid Token" UserInfo={NSLocalizedFailureReason=Not Authorized - Invalid Token}
    ✗ testPolyline, XCTAssertNotNil failed -
❌  fatal error: unexpectedly found nil while unwrapping an Optional value

RomainQuidet · 2017-11-03T14:59:06Z

Ha yes sorry I need to check the unit tests ! Thanks.

bsudekum · 2017-11-03T15:05:02Z

MapboxDirections/MBDirections.swift

+			baseURLComponents.scheme = "https"
+			baseURLComponents.host = mapboxHost
+        }
+		else


Could you fix the indentation here and also put curly braces on the same line as the else? } else {

I use tabs for indentation in my Xcode... is that a problem ?
I moved the brackets correctly.

👍 , tabs should be fine. Just trying to keep the library consistent.

bsudekum · 2017-11-03T15:07:32Z

MapboxDirections/MBDirections.swift

+            path = "route/v1/\(options.profileIdentifier.rawValue)/\(queryComponent)"
+        }
+
+        let unparameterizedURL = URL(string: path, relativeTo: apiEndpoint)!


This should be properly unwrapped.

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

bsudekum · 2017-11-03T15:08:34Z

MapboxDirections/MBRouteOptions.swift

@@ -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"


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

Do I have to do it ?

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

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

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.

bsudekum · 2017-11-03T15:10:28Z

MapboxDirections/MBDirections.swift

-        ]
-
-        let unparameterizedURL = URL(string: options.path, relativeTo: apiEndpoint)!
+        let isMapboxHost = apiEndpoint.host == mapboxHost


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.

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

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. 🤷‍♂️

RomainQuidet · 2017-11-10T13:49:54Z

@bsudekum Hi, how can we move forward with this pull request ? Do I have to change something ?

bsudekum · 2017-11-10T14:34:53Z

MapboxDirections/MBDirections.swift

+			baseURLComponents.scheme = "https"
+			baseURLComponents.host = mapboxHost
+        }
+		else {


Can you put the previous curly brace and the else on the same line please?

bsudekum · 2017-11-10T14:39:38Z

MapboxDirections/MBDirections.swift

-        baseURLComponents.scheme = "https"
-        baseURLComponents.host = host ?? "api.mapbox.com"
+		var baseURLComponents: URLComponents
+        if host == nil {


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

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

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

@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 }

bsudekum · 2017-11-10T14:40:26Z

Also cc @1ec5

vincethecoder · 2018-02-16T19:54:13Z

MapboxDirections/MBDirections.swift

+            ]
+        }
+
+        let path: String


shouldn't this be a var?

datwelk · 2018-12-11T12:37:08Z

What's the status of this?

1ec5

I always felt bad about never getting around to reviewing this PR after having entertained the notion in #121. That was about when navigation went from being a side hobby project to something real inside Mapbox. Just to set the record straight about that issue, the original idea was that this library could add some facilities to process raw OSRM responses as easily as Mapbox Directions API responses. As far as I knew, there wasn’t a particular need to support the OSRM request format differently than Mapbox Directions API requests. Later on, the more flexible JSON deserialization in #382 and #669 took away some of the hurdles to parsing workalike responses too.

This PR seems to be focused on the request format, but it isn’t clear to me that requesting routes was ever blocked in the first place. Rather, over the years, Mapbox request and response formats have both accumulated new bits, some of which the navigation SDK has come to depend on. The most prominent part is banner and voice instructions, although the navigation SDK can kind of degrade gracefully without them.

Unfortunately, I’m not sure there’s much to be gained by leaving this PR open, since the library has been refactored so many times since.

1ec5 · 2023-06-21T00:42:50Z

MapboxDirections/MBDirections.swift

-        ]
-
-        let unparameterizedURL = URL(string: options.path, relativeTo: apiEndpoint)!
+        let isMapboxHost = apiEndpoint.host == mapboxHost


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. 🤷‍♂️

1ec5 · 2023-06-21T00:45:29Z

MapboxDirections/MBRouteOptions.swift

@@ -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"


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.

Romain Quidet added 2 commits October 13, 2017 17:36

[OSRM] accept other hosts than mapbox (OSRM server)

0f648aa

Merge commit 'cd2c244160dae4dd94bc8fdc214116d5324b3564' into OSRMSupport

f9ce0de

better custom host string handling

9bb1551

Merge branch 'master' into OSRMSupport

f176ef0

bsudekum suggested changes Nov 3, 2017

View reviewed changes

Romain Quidet added 2 commits November 3, 2017 17:18

correct mapbox v4 api support

2f0d06b

review: correct braket place

12e2423

bsudekum reviewed Nov 10, 2017

View reviewed changes

review: reposition the else

d2cce68

vincethecoder reviewed Feb 16, 2018

View reviewed changes

MapboxDirections/MBDirections.swift

]

}

let path: String

Copy link

Contributor

vincethecoder Feb 16, 2018

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?

1ec5 mentioned this pull request Mar 15, 2018

Changed access level of MBRouteOptions response function & MBRouteStep instructions parameter #246

Closed

1ec5 changed the base branch from master to main October 6, 2020 17:21

S2Ler marked this pull request as draft January 17, 2023 06:04

1ec5 reviewed Jun 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Osrm support #198

Osrm support #198

RomainQuidet commented Oct 27, 2017

RomainQuidet commented Oct 27, 2017

RomainQuidet commented Nov 3, 2017

bsudekum commented Nov 3, 2017

RomainQuidet commented Nov 3, 2017

bsudekum Nov 3, 2017

RomainQuidet Nov 3, 2017

bsudekum Nov 3, 2017

bsudekum Nov 3, 2017

RomainQuidet Nov 3, 2017

bsudekum Nov 3, 2017

RomainQuidet Nov 3, 2017

bsudekum Nov 8, 2017

bsudekum Nov 10, 2017

1ec5 Jun 21, 2023

bsudekum Nov 3, 2017

RomainQuidet Nov 3, 2017

1ec5 Jun 21, 2023

RomainQuidet commented Nov 10, 2017

bsudekum Nov 10, 2017

bsudekum Nov 10, 2017

RomainQuidet Nov 10, 2017

bsudekum Nov 10, 2017

vincethecoder Feb 16, 2018 •

edited

Loading

bsudekum commented Nov 10, 2017

vincethecoder Feb 16, 2018

datwelk commented Dec 11, 2018

1ec5 left a comment •

edited

Loading

1ec5 Jun 21, 2023

1ec5 Jun 21, 2023

Osrm support #198

Are you sure you want to change the base?

Osrm support #198

Conversation

RomainQuidet commented Oct 27, 2017

RomainQuidet commented Oct 27, 2017

RomainQuidet commented Nov 3, 2017

bsudekum commented Nov 3, 2017

RomainQuidet commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainQuidet commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

bsudekum commented Nov 10, 2017

Choose a reason for hiding this comment

datwelk commented Dec 11, 2018

1ec5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Feb 16, 2018 •

edited

Loading

1ec5 left a comment •

edited

Loading