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.

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

Clean up JSON response #519

Closed
emiltin opened this issue Nov 27, 2012 · 36 comments
Closed

Clean up JSON response #519

emiltin opened this issue Nov 27, 2012 · 36 comments

Comments

@emiltin
Copy link
Contributor

emiltin commented Nov 27, 2012

the response structure is currently:

"version": ...,
"status":...,
"status_message": ...,
"route_geometry": ...,
"route_instructions": [...], 
"route_summary: {...},
"alternative_geometries": [...],
"alternative_instructions":[...],
"alternative_summaries":[...]
,"route_name":[...],
"alternative_names":[...],
"via_points":[],
"hint_data": ...,
 "locations": [....],
"transactionId": ...

i think it would be cleaner to reorganize routes and alternate routes into a single array, with each element containing all the info for that particular route:

"version": ...,
"status":...,
"status_message": ...,
"routes":[
    {
        "name": ....,
        "geometry" : ...
        "instructions": [...], 
        "summary: {...},   
    },
    {
        "name": ....,
        "geometry" : ...
        "instructions": [...], 
        "summary: {...},   
    },
    ....
]
"via_points":[],
"hint_data": ...,
 "locations": [....],
"transactionId": ...

if you did not request alternative routes, the array contains one element. if you did request alternatives, it could have 2 or more, or 1 if not alternative was found.

at the moment, info for a specific route is scattered across several arrays (which hopefully have the same length).

it also seems natural to move the route names into the route array elements, and drop the 'route_' prefix on the elements in the route array.

@MKergall
Copy link

MKergall commented Dec 8, 2012

I vote! This is more rational, and similar to the Google Maps approach.

But please, when changing the API, document the Server API (https://github.com/DennisOSRM/Project-OSRM/wiki/Server-api) in parallel:

  • Alternative routes are still undocumented (request, response)
  • default for instructions parameter is now set as false, but was documented as true. I just changed that.

@TheMarex TheMarex changed the title reorganize route json reponse Clean up JSON response May 24, 2015
@TheMarex TheMarex added this to the 0.5 milestone May 24, 2015
@TheMarex
Copy link
Member

This will break backwards compatibility but would make handling responses in frontends much simpler.

My goal is to get something similar to what we currently use as Mapbox Directions API:
https://www.mapbox.com/developers/api/directions/ which is pretty similar to what @emiltin suggested.

Problems:

  • Natural language/HTML turn instructions: This is not going to happen at the backend side
  • GeoJSON is rather verbose (especially for simple stuff like via locations) but I don't feel too strongly about this

@perliedman as you are maintaining a popular frontend, any opinions on this?

@perliedman
Copy link

Breaking changes are of course always a hassle, but none the less I'm 👍 for this. The current API has always struck me as a bit weird.

I'm also a fan of using GeoJSON. Using a standardized formatt makes it easy to work with the routes in ways that were not necessarily intended, and makes it really is easy to visualize results in a large number of tools. If there's not a great performance penalty here, I'd vote for this as well.

@zavan
Copy link

zavan commented Oct 23, 2015

I vote for something more similar to the Google Maps Directions API response, spliting the route in legs, the legs in steps and the steps in points (polyline). I think it's much more rational.

@TheMarex
Copy link
Member

First draft of the 5.0 API can be found here.

  • viaroute (aka route)
  • table
  • nearest
  • match
  • trip

Primary changes to 4.9.0 API:

  • Renamed viaroute -> route
  • Versioned / profiled URL structure
  • viaroute response format similar to Mapbox Directions

@perliedman @freenerd any feedback is appreciated. The wiki can be freely edited in case of suggestions.

@MKergall
Copy link

Question about route service, "geometry" attribute:

What will be the exact output for "polyline" and "array" options

Please don't discard the encoded polyline option. On an Android device, for a long route, this is the only acceptable solution (json decoding takes a while).

@TheMarex
Copy link
Member

@MKergall Thanks for the feedback, will specify more clearly. 👍 No the encoded polyline option will stay of course.

@freenerd
Copy link
Member

I looked at the New Server API, generally I'm 👍 but think that there is some cases where the docs should be more precise. Comments below.


I really don't like the repetition and positional query arguments. These queries can be hard to create with some HTTP Libraries, something that we hit in our own test suite. It means that people just have to fallback to building query parameter strings themselves, which feels okay with me. The other option would be indexing parameter keys (e.g. loc[0]/loc[1]) but this makes creating queries fairly harder, since the numbers need to correctly ascend. So we might jsut want to stick with the current design.

The profile component in the base path is meant for enabling loading multiple profiles at the same time in the future #1907? Makes sense to me. We should still enable a way to de-couple path profile name from the name of the lua profile though. This will likely be a parameter for osrm-routed and/or osrm-datastore.

Response
On error the following object is returned.

Could we always return a key like code that holds ok when everything was right, otherwise the service-specific error codes? This makes programming easier, since a programmer could rely on the response being in the format expected when this key is set.

Service route
uturn false (default), true Specify after each loc. Enables/disables u-turn at the via.

Should we enable uturns by default in 5.0? Maybe warrants another ticket.

error_types are supported:
no-segment No matching street segment found.
no-route No route found.

What's the difference between these two? Also will the "Impossible Route" be a no-route as well?

Service table

  • What if all three loc, src, and dst are specified? Will they be merged or will one take precendence?
  • How is precendence when polylines srcs and dests are present?
  • No polyline locs?

Service match
In case of error the following error_types are supported:

Type Description
no-segment No matching street segment found.
no-matchings No matchings found.

What is the difference between these two errors? Are these errors for the whole match or can they happen also on sub-matches? How would they be present in the response then?

If there is no match, will there be an empty matchings array?

Service trip
Response
trips_index

Can it be that a point can't be matched? Or will it always be in a trip (with the most extreme case a trip with only this one point)?

Service table
Service match
Service trip

The examples should include several loc={lat,lon}... like route to indicate that there is more than one loc to be passed.

@TheMarex
Copy link
Member

I really don't like the repetition and positional query arguments. These queries can be hard to create with some HTTP Libraries, something that we hit in our own test suite. It means that people just have to fallback to building query parameter strings themselves, which feels okay with me. The other option would be indexing parameter keys (e.g. loc[0]/loc[1]) but this makes creating queries fairly harder, since the numbers need to correctly ascend. So we might jsut want to stick with the current design.

I would like to stay somewhat with the old query-parameter design. Makes things easier to port and the param[idx] convention would just have different problems.

Could we always return a key like code that holds ok when everything was right, otherwise the service-specific error codes? This makes programming easier, since a programmer could rely on the response being in the format expected when this key is set.

I don't see why this would reduce the effort for the programmer. if (response.error) or if (response.code != 'ok') seems equally complex (while the first one is easier to read IMHO and keeps with node conventions).

Should we enable uturns by default in 5.0? Maybe warrants another ticket.

I don't think it is a safe assumption that executing a u-turn is in general always possible. This can lead to absurd (and unsafe) suggestions like doing u-turns in the middle of high traffic streets.

What's the difference between these two?

Needs clarification: First one means the nearest neighbor search could not find a matching segment. Will update the docs.

What if all three loc, src, and dst are specified? Will they be merged or will one take precendence?

Good catch. Needs clarification. Would like to opt for the same appraoch as node-osrm that only supports either src and dst or loc. Will update the docs.

How is precendence when polylines srcs and dests are present?

Same, needs clarification. Would like to be maximal restrictive and only allow either single point options (loc, dst, src or polyline options).

No polyline locs

Supported for all queries as outlined at the beginning. Not sure if we should back-link to the common options each time?

Are these errors for the whole match or can they happen also on sub-matches

As skipping trace points is expected behaviour this works analogous to route that this indicates that result.matching would be an empty array.

If there is no match, will there be an empty matchings array

In case of error the error object will be returned.

Or will it always be in a trip (with the most extreme case a trip with only this one point)?

Trip length is minimum one waypoint.

The examples should include several loc={lat,lon}... like route to indicate that there is more than one loc to be passed.

Good point.

@freenerd
Copy link
Member

I don't see why this would reduce the effort for the programmer. if (response.error) or if (response.code != 'ok') seems equally complex (while the first one is easier to read IMHO and keeps with node conventions).

Other HTTP status codes like 5XX are possible (e.g. from load balancers in front of osrm-routed). These will likely not emit a response body, thus if (response.error) won't catch them. I imagine client code to look like this:

switch (body.code) {
  case 'ok':
     // normal processing with safe access to body.routes
  case 'no-route':
     // error message to user
  default:
     // panic
}

I don't think it is a safe assumption that executing a u-turn is in general always possible. This can lead to absurd (and unsafe) suggestions like doing u-turns in the middle of high traffic streets.

Moving to #1922

Would like to be maximal restrictive and only allow either single point options (loc, dst, src or polyline options).

Cool. Likely best would be to throw an error instead of taking precedence.

Supported for all queries as outlined at the beginning. Not sure if we should back-link to the common options each time?

I'd re-add it in this case, since it adds clarity.

If there is no match, will there be an empty matchings array
In case of error the error object will be returned.

I think this should say: In case of an error only the error object will be returned.

@TheMarex
Copy link
Member

Other HTTP status codes like 5XX are possible (e.g. from load balancers in front of osrm-routed). These will likely not emit a response body, thus if (response.error) won't catch them.

Ok changed error -> code and added ok.

Cool. Likely best would be to throw an error instead of taking precedence.

👍

In case of error the error object will be returned.

👍

I also slightly modified the draft to split the route into legs now. There is a new RouteLeg object.

@perliedman
Copy link

It looks like RouteStep is missing some kind of reference to the geographic location it refers to: in the current version we have an index into the route's polyline coordinates, to be able to for example zoom in on a step's location. I think this is something we need!

Also, it appers both the Route object and the RouteLeg have geometry properties - does this mean they geometry will be duplicated, or is it rather that the Route's geometry should be removed?

Other than that, I think this is shaping up really nicely, good job!

@perliedman
Copy link

Don't mind my first comment, I found the location property in StepManeuver right after I posted, of course.

@TheMarex
Copy link
Member

Also, it appers both the Route object and the RouteLeg have geometry properties - does this mean they geometry will be duplicated, or is it rather that the Route's geometry should be removed?

Yeah that is also the only problem I see with the leg approach: If you want to provide the aggregated values for duration, distance and geometry as well they will be the same in the single leg case (normal A->B queries). One solution would be to provide no aggregates which is inconvenient, but would remove the duplicates.

@perliedman
Copy link

Duplicating duration and distance seems fine, but geometry is probably a bit more problematic, at least if saving bandwidth is the priority.

How about using indices into the complete route's geometry to specify which part belongs to each leg? Like start and end index?

@freenerd
Copy link
Member

https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api#properties

distance: The distance traveled by the route, in double meters.

I guess double meters means a float, right? Not meters * 2?

overview_geometry: The simplified geometry of the route, depending on the geometry parameter. See RouteStep's geometry field for a parameter documentation. Simplification level is governed by the z parameter.

What is the default z? Above (e.g. https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api#request-2) it's 0 ... 18 (default) so this is 18? Thus it's not simplified by default?

https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api#query

If src is used at least one dst needs to be supplied and reciprocal

Does this mean src and dst need to have the same number of coordinates? I though this was asymmetric?

The docs should note that using loc together with src or dst will result in an error.

The url example should be split into to, with one example for loc and one for src and dst since both together will not work.

The example should include the repetition of query parameters, in the same style as the route service example does it:

http://{server}/route/v1/{profile}.json?loc={lat,lon}&loc={lat,lon}...

http://{server}/route/v1/{profile}.json?src={lat,lon}&src={lat,lon}&dest={lat,lon}...

@TheMarex
Copy link
Member

@freenerd thanks for the feedback. 👍

I hope a lot of the confusion w.r.t. loc options should be cleared up with the new query format:

http://{server}/{service}/{version}/{profile}/{query}.json?option=value&option=value

e.g. for a route query:

http://router.project-osrm.org/route/v1/driving/lat:52.517037,lon:13.388860;lat:52.529407,lon:13.397634,bearing:0;lat:52.523219,lon:13.428555.json?geometry=false

As you can see the query part does not use the usual URL schema based encoding, but a key-value format with , and ; delimiter to separate locations and their options better from each other.

@TheMarex
Copy link
Member

TheMarex commented Feb 2, 2016

An alternative approach was brought up by @jfirebaugh of using nested parameters like this:

http://router.project-osrm.org/route/v1/driving/json?loc[0][lat]=52.517037&loc[0][lon]=13.388860&loc[1][lat]=52.529407&loc[1][lon]=13.397634&loc[1][bearing]=0&loc[2][lat]=52.523219&loc[2][lon]=13.428555&geometry=false

Pro:

  • somewhat established convention (Rails and jQuery both support it)
  • Somewhat similar to our old query API (still no parser would support options that make it compatible with old loc= style queries.)

Con:

  • blown up URL thanks to long prefixes
  • state of client side generators/parser is bad. try loc=23.2&loc=23&loc[0][bearing]=23.3&loc=32&loc[0][range]=4.
    • express seems to use qs? At least similar behavior.
    • qs is just broken (What validation? Let me fix that for you.)
    • querystring does just not support nested properties

Either we go ahead and implement a parser with some actual validation, or we need to live with random results form the query parsing.

@freenerd
Copy link
Member

The request format has now been decided to a version with ; separated values in the query and for query parameters like bearings and radiuses. We also support null values via ;; (nothing between two ;). It's in the wiki docs.

@TheMarex One thing I realized when looking at errors:

  • The docs are introducing {status_type} but i think that could just be {code} and the same as the property name?
  • Can the codes be in Camel Case instead of hyphenated lower-case? This is mostly bike-shedding to keep it in sync with the Mapbox Directions API, there is no technical good/bad here.

@TheMarex
Copy link
Member

Can the codes be in Camel Case instead of hyphenated lower-case? This is mostly bike-shedding to keep it in sync with the Mapbox Directions API, there is no technical good/bad here.

👍 Updating.

@freenerd
Copy link
Member

@TheMarex More feedback/questions/suggestions. Let's discuss! Check boxes when dismissed, or resolved in docs and code.

  • Can we rename {query} to {coordinates}, so it can't be confused with the HTTP URL query string
  • Do we use both terms waypoint and coordinate? From my understanding of the terms, a coordinate is the user's input and a waypoint is the snapped output of a route or nearest request. If so, then we should double-check to not confuse both terms in the docs
  • In https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api#route and https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api#response-3 the term via point is used, which should likely be replaced with waypoint?
  • Should we remove polyline from queries? If I remember correctly ?, & and ) are valid polyline characters, thus they could lead to invalid query strings!?
  • For polyline responses, should we mention precision being 5?
  • The docs are introducing {status_type} but i think that could just be {code} instead?
  • For U-Turns there is a global and a per-coordinate parameter. How are conflicts between both handled? Could we get rid of one of them? What is a use case for a the per-coordinate parameter that couldn't be handled with a global parameter?
  • Bearing and Heading are true north and clockwise, correct?
  • Does a bearing range of 10 and a direction of 0 allow values from 350 - 10 or 0 - 10? If the former, what does 360 allow? Wouldn't 180 be sufficient then?
  • RouteStep.heading_* could be renamed to bearing_* to keep in line with the query parameter? Or we rename bearings to headings in the query?
  • What is the unit for radius? meters, kilometers?
  • What's the use-case for per-coordinate radiuses? Wouldn't one global value serve well?
  • Waypoint rename way_name to name
  • RouteStep rename way_name to name
  • Route rename overview_geometry to geometry
  • RouteStep rename geometries to geometry, since it's only one
  • RouteStep.mode should specifiy which modes exist (or where to look this up)
  • Waypoint.location and StepManeuver.location use latitude,longitude but should be consitent with the input using longitude,latitude instead
  • For clarity in docs I'd like to prefer longitude,latitude over abreviated lon,lat

@TheMarex
Copy link
Member

Do you keep a v4 -> v5 changelog where we can add this?

Sadly no, but we should make one internally. Can you open a ticket for that?

Where do I find the existing modes? Are these per profile, e.g. car modes here

These will be profile defined. Profiles will pass a string (not a number like now) and will be passed through to the client.

TheMarex added a commit that referenced this issue Feb 22, 2016
@freenerd
Copy link
Member

Question regarding U-Turns:

Given that every coordinate can have u-turns enabled/disabled, for which legs are they available then? For the leg before the coordinate, for the leg after the coordinate or for both? My intuition here is that they are valid for the leg after the coordinate, which would mean that the u-turn value for the last coordinate has no effect.

@TheMarex
Copy link
Member

For the leg before the coordinate, for the leg after the coordinate

I think the function is not clear here. uturns enables doing a u-turn at a waypoint. That means the u-turn occurs between legs. For example you enter a street in south->north direction, reach a via and then leave in north->south direction.

Technically you can always do a uturn at the first waypoint because you are not yet constrained in the travel direction.

@freenerd
Copy link
Member

Moved the uturn discussion to #2012

TheMarex added a commit that referenced this issue Mar 1, 2016
TheMarex added a commit that referenced this issue Mar 2, 2016
TheMarex added a commit that referenced this issue Mar 4, 2016
TheMarex added a commit that referenced this issue Mar 4, 2016
TheMarex added a commit that referenced this issue Mar 6, 2016
@jcoupey
Copy link

jcoupey commented Mar 7, 2016

Just reporting on my first attempts with the new server API. There seem to be a few inconsistencies between the current rewrite/new-api branch responses and the documentation at
https://github.com/Project-OSRM/osrm-backend/wiki/New-Server-api.

For a malformed query, I get an invalid-url response code, not InvalidUrl. Just a matter of convention of course, but needs to be sorted out at some point. ;-)

@TheMarex
Copy link
Member

TheMarex commented Mar 7, 2016

@jcoupey oh good catch yes, I forgot to adapt the server code. 👍

TheMarex added a commit that referenced this issue Mar 8, 2016
@jcoupey
Copy link

jcoupey commented Mar 9, 2016

Did some more testing with the rewrite/new-api branch and saw that all durations and distances are currently returned as floats in the json response. Is this a general policy for the new server API or just somehow related to the development phase?

@TheMarex
Copy link
Member

TheMarex commented Mar 9, 2016

@jcoupey no we switched from deci-seconds for duration to actually returning seconds and meters should always have been a float.

@jcoupey
Copy link

jcoupey commented Mar 9, 2016

Related question/concern if you return a number of seconds as a float: what happens in a table request when some routes are not found? The corresponding duration used to be std::numeric_limits<int>::max().

@TheMarex
Copy link
Member

TheMarex commented Mar 9, 2016

@jcoupey good point, there is a lack in docs there right now. It should be null.

TheMarex added a commit that referenced this issue Mar 10, 2016
TheMarex added a commit that referenced this issue Mar 15, 2016
TheMarex added a commit that referenced this issue Mar 17, 2016
TheMarex added a commit that referenced this issue Mar 18, 2016
TheMarex added a commit that referenced this issue Mar 23, 2016
TheMarex added a commit that referenced this issue Mar 25, 2016
lbud pushed a commit that referenced this issue Mar 25, 2016
TheMarex added a commit that referenced this issue Mar 31, 2016
TheMarex added a commit that referenced this issue Apr 5, 2016
@TheMarex
Copy link
Member

TheMarex commented Apr 8, 2016

The new API has been merged into develop. Expect smaller parameter name changes/default changes to happen before RC2. Otherwise the API is fixed now.

@TheMarex TheMarex closed this as completed Apr 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants