Extract error information from JSON. #7

mjkillough · 2019-03-11T13:25:41Z

When OSRM returns an error, it (often?) includes an error code/message in the returned JSON object. This change extracts this code/message and returns it to the caller of libosrmc.

This makes it easier to debug failures, but also makes it possible to react to errors such as NoRoute. An example of such a route is between 25.07165,55.402115 and 25.086226,55.385334 in the UAE, using the foot or bicycle profiles.

Add the ability to specify whether the table should be annotated with distance and/or duration, as well as a new API for returning the distance. This is done in a way that allows us to return errors for unrouteable routes (where `null` is returned in the JSON), as in daniel-j-h#7. I'm not 100% sure of the use of `stdbool` in a FFI interface - it's been a while since I had to write C!

daniel-j-h · 2019-03-11T21:42:51Z

Hey there, thanks for this pull request (and the other ones) :)

Quick disclaimer before we go into details: this project was sort of a prototype and is lacking large parts of the API. I also haven't compiled or tested it against libosrm in the last two'ish years, and it's not used by me right now.

That said, if you want to pick it up I'm happy to guide you along and merge your improvements in.

Now to this pull request. I think it's a very good idea in general! We already have an error type here

libosrmc/libosrmc/osrmc.cc

Lines 28 to 30 in a968cfd

    
           struct osrmc_error final { 
        
             std::string message; 
        
           };

Right now we only store a single generic message.

Here's what I think we should do instead:

store two strings: message, and code
parse message and code from the json object (like you do)
code should always be there (assert that)
message is optional, empty string is fine in this case I think
add another function osrmc_error_code similar to the existing osrmc_error_message (see below)

libosrmc/libosrmc/osrmc.h

Line 162 in efa047d

OSRMC_API const char* osrmc_error_message(osrmc_error_t error);

In theory we could also make the error code an enum we expose to the user. But even in upstream libosrm these enum values are strings only and only documentation captures all possible values. Not sure I like this to be honest, see:

https://github.com/Project-OSRM/osrm-backend/search?q=TooBig&type=Code

Let me know what you think.

daniel-j-h · 2019-03-11T21:31:47Z

libosrmc/osrmc.cc

@@ -29,6 +29,21 @@ struct osrmc_error final {
  std::string message;
 };

+void osrmc_error_from_json(osrm::json::Object* json, osrmc_error_t* error) try {


We only use this function in the implementation, right? Then it should be static .

Great point! I'd forgotten about static. :-)

mjkillough · 2019-03-12T10:24:57Z

Quick disclaimer before we go into details: this project was sort of a prototype and is lacking large parts of the API. I also haven't compiled or tested it against libosrm in the last two'ish years, and it's not used by me right now.

That said, if you want to pick it up I'm happy to guide you along and merge your improvements in.

Thank you so much for giving such detailed feedback, so quickly! As this didn't appear to be used, I wasn't sure what to expect. :-)

I'm using this to create a Rust wrapper around libosrm, and it includes everything I think I'll need (except the durations in the Table API, hence the other PR). It seems to work great against the current libosrm, so that was a really nice surprise! I'm very happy to support it for my use-case.

Here's what I think we should do instead:

store two strings: message, and code

parse message and code from the json object (like you do)

code should always be there (assert that)

message is optional, empty string is fine in this case I think

add another function osrmc_error_code similar to the existing osrmc_error_message (see below)

Great idea! This makes it much easier for me to match on the error in my application. I can now match on the code.

I've done all of this, except asserting the code is always there. I've instead defaulted it to "Unknown" if it's missing (or if the JSON is never returned). This felt better, as libosrmc could become out-of-sync with libosrm, and I'd like it to still return something for the error even i

In theory we could also make the error code an enum we expose to the user. But even in upstream libosrm these enum values are strings only and only documentation captures all possible values. Not sure I like this to be honest, see:

https://github.com/Project-OSRM/osrm-backend/search?q=TooBig&type=Code

This would have been great, as it'd allow me to handle the errors without string matching (which is a bit gross). However, I don't think we could achieve this without significant changes to libosrm - I think sticking with this approach is probably good enough.

Add the ability to specify whether the table should be annotated with distance and/or duration, as well as a new API for returning the distance. This is done in a way that allows us to return errors for unrouteable routes (where `null` is returned in the JSON), as in daniel-j-h#7. I'm not 100% sure of the use of `stdbool` in a FFI interface - it's been a while since I had to write C!

daniel-j-h

Good design decision on the "Unknown" fallback instead of assert.

I'm pretty happy with how you changed the implementation; there's a small nitpick with asserting a nullptr (I left an inline comment). If you change that I think we can merge this in.

daniel-j-h · 2019-03-12T20:39:28Z

libosrmc/osrmc.cc

+static void osrmc_error_from_json(osrm::json::Object* json, osrmc_error_t* error) try {
+  if (json == nullptr) {
+    *error = new osrmc_error{"Unknown", ""};
+  }


I think we can assert json != nullptr. The object itself should always be there, otherwise the user is calling this API the wrong way. Check out how we use this API on the call sites, the result json is always an (potentially empty) object but should never be a nullptr.

Great point! I've updated it to assert instead.

Thanks a lot for all the help! 🙂

Add the ability to specify whether the table should be annotated with distance and/or duration, as well as a new API for returning the distance. This is done in a way that allows us to return errors for unrouteable routes (where `null` is returned in the JSON), as in daniel-j-h#7. I'm not 100% sure of the use of `stdbool` in a FFI interface - it's been a while since I had to write C!

daniel-j-h · 2019-03-17T15:13:21Z

libosrmc/osrmc.cc

+}
+
+static void osrmc_error_from_json(osrm::json::Object* json, osrmc_error_t* error) try {
+  assert(json != nullptr);


I just had a look at the call sites and it seems like we only use this internally in the C++ impl.

Then we could change this function to taking a const osrm::json::Object& (since we only need a read-only view for accessing the json values) and don't have to care about null pointers (since refs can't be null).

Good point! Unfortunately it can't be const as, surprisingly, indexing into values adds the key to the unordered_map if it doesn't exist. (This shouldn't actually matter in practise I don't think).

When OSRM returns an error, it (often?) includes an error code/message in the returned JSON object. This change extracts this code/message and returns it to the caller of libosrmc. This makes it easier to debug failures, but also makes it possible to react to errors such as `NoRoute`. The errors are extracted as both `code`/`message`, and a new `osrmc_error_code` is introduced to allow the caller to extract the code.

Add the ability to specify whether the table should be annotated with distance and/or duration, as well as a new API for returning the distance. This is done in a way that allows us to return errors for unrouteable routes (where `null` is returned in the JSON), as in daniel-j-h#7. I'm not 100% sure of the use of `stdbool` in a FFI interface - it's been a while since I had to write C!

Add the ability to specify whether the table should be annotated with distance and/or duration, as well as a new API for returning the distance. This is done in a way that allows us to return errors for unrouteable routes (where `null` is returned in the JSON), as in #7. I'm not 100% sure of the use of `stdbool` in a FFI interface - it's been a while since I had to write C!

daniel-j-h · 2019-03-17T21:00:36Z

Merged with #8 (comment)

mjkillough mentioned this pull request Mar 11, 2019

Add support for Duration & Distance in Table API. #8

Merged

daniel-j-h reviewed Mar 11, 2019

View reviewed changes

mjkillough force-pushed the errors branch from a968cfd to fd986c4 Compare March 12, 2019 10:17

daniel-j-h reviewed Mar 12, 2019

View reviewed changes

mjkillough force-pushed the errors branch from fd986c4 to 3065f8f Compare March 13, 2019 20:29

mjkillough force-pushed the errors branch from 3065f8f to 1fb981a Compare March 13, 2019 21:31

mjkillough force-pushed the errors branch from 1fb981a to f30bd0d Compare March 13, 2019 21:33

daniel-j-h reviewed Mar 17, 2019

View reviewed changes

mjkillough force-pushed the errors branch from f30bd0d to 9578160 Compare March 17, 2019 19:19

daniel-j-h closed this Mar 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract error information from JSON. #7

Extract error information from JSON. #7

mjkillough commented Mar 11, 2019

daniel-j-h commented Mar 11, 2019

daniel-j-h Mar 11, 2019

mjkillough Mar 12, 2019

mjkillough commented Mar 12, 2019

daniel-j-h left a comment

daniel-j-h Mar 12, 2019

mjkillough Mar 13, 2019

daniel-j-h Mar 17, 2019

mjkillough Mar 17, 2019 •

edited

Loading

daniel-j-h commented Mar 17, 2019

Extract error information from JSON. #7

Extract error information from JSON. #7

Conversation

mjkillough commented Mar 11, 2019

daniel-j-h commented Mar 11, 2019

daniel-j-h Mar 11, 2019

Choose a reason for hiding this comment

mjkillough Mar 12, 2019

Choose a reason for hiding this comment

mjkillough commented Mar 12, 2019

daniel-j-h left a comment

Choose a reason for hiding this comment

daniel-j-h Mar 12, 2019

Choose a reason for hiding this comment

mjkillough Mar 13, 2019

Choose a reason for hiding this comment

daniel-j-h Mar 17, 2019

Choose a reason for hiding this comment

mjkillough Mar 17, 2019 • edited Loading

Choose a reason for hiding this comment

daniel-j-h commented Mar 17, 2019

mjkillough Mar 17, 2019 •

edited

Loading