search

daniel-j-h / libosrmc

Pure C bindings for libosrm
MIT License
18 stars 8 forks source link

Extract error information from JSON. #7

Closed mjkillough closed 5 years ago

mjkillough commented 5 years ago

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.

daniel-j-h commented 5 years ago

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

https://github.com/daniel-j-h/libosrmc/blob/a968cfd3469c702127e508f808dd3692a66b2eba/libosrmc/osrmc.cc#L28-L30

Right now we only store a single generic message.

Here's what I think we should do instead:

https://github.com/daniel-j-h/libosrmc/blob/efa047df7b056a25344ff20040eac0d414a78ac3/libosrmc/osrmc.h#L162

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.

mjkillough commented 5 years ago

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:

  • [x] store two strings: message, and code
  • [x] parse message and code from the json object (like you do)
  • [ ] code should always be there (assert that)
  • [x] message is optional, empty string is fine in this case I think
  • [x] 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.

daniel-j-h commented 5 years ago

Merged with https://github.com/daniel-j-h/libosrmc/pull/8#issuecomment-473706030