Graphhopper: WebHelper.errorResponse (currently: RouteResource.errorResponse())

[Nakaner]

This issue refers to the current master branch (with all the changes related to Dropwizard).

I don't fully understand the thoughts behind recent moves of
WebHelper.errorResponse (currently: RouteResource.errorResponse()).

@michaz moved this and other static helper method from the RouteResource to the WebHelper class in commit 69cfd03 because "map matching re-uses them" (quote of the commit message). A few weeks later he moved them back with the comment "Move web service helper methods back to route resource; clean dependencies" in commit 9e2765c and made the method private.

This breaks the map matching module. He tried to fix it in commit 4bee777 of the map matching module but that fix makes map-matching only to compile properly but it is still broken. (That's not covered by a unit test ;-)) If you send broken GPX data to the map matching API and request a XML response, you will get your error message in JSON format but valid input data would result in a valid XML response.

The reason: Yesterday the public method com.graphhopper.http.WebHelper.errorResponse() (part of graphhopper-web-api) was moved to com.graphhopper.resources.RouteResource.errorResponse() in commit 9e2765c and made private. This change reverted the changes of commit 69cfd03 which moved it in the opposite direction.

I think that such a method is useful for anyone who wants to build a GraphHopper like API – be it just the map matching module or an obscure routing engine. :)

[michaz]

I'm moving those methods back and forth because I'm looking for a compromise between no code duplication and smallest surface area. (I don't want to publish lots of public "helper" methods which I'm then not supposed to change anymore.)

That said, _all_ of those methods are refactoring artifacts. Ideally, they shouldn't exist. So no, please don't consider them public library methods which we recommend using when building things.

Thanks for pointing out that map-matching currently doesn't return XML formatted errors (I vaguely think I noticed that while I was doing that). Same thing here: I'm looking for something with the least amount of code, fewest extra else branches for uncommon cases etc., so please give me some time and leeway. When I find a good way to bring it back, I'll bring it back (this is not a release). We currently have too much code that is not about routing, but about moving stuff around.

Let's be fair, the XML error output isn't even specified.

[karussell]

Let's be fair, the XML error output isn't even specified.

Documentation does indeed not specify it but still people built already code around it and we'll get trouble when breaking this.

I don't want to publish lots of public "helper" methods which I'm then not supposed to change anymore

Especially at the beginning of this 'consolidation phase' I would not be this strict. But even when we publish 1.x at some day we can release 2.x that introduces breaking changes.

[devemux86]

GraphHopper lately confuses a lot its users. A public API is a public API. And master branch isn't supposed to be used for experiments without testing, such cases can mature in feature branches?

[michaz]

Hey @devemux86, what's up? Does the Github discussion traffic confuse you, or what exactly? master tests are green. A public _web_ API should be as stable as possible, I commit to that (On our server, that is. I don't have a bad conscience for once in a while having an intemediary state on master which doesn't correspond to what runs on our server, at least when it wasn't test-covered.)

The public _Java_ API, especially the more peripheral stuff, cannot be considered stable on the class- or method level. We just aren't there yet, and wouldn't be able to move at all.

[devemux86]

See #1385 mentioned by @karussell.
BTW since being polite in all my conversations, I'd appreciate the same from others too. :slightly_smiling_face:

[michaz]

Sorry, was supposed to sound the opposite - trying to get the assertion that GraphHopper confuses its users down to the object level. :) So we need CI for the Android build, an "acceptance" test for the XML error message in Map-Matching, and we're good, right?

[michaz]

we need CI for the Android build

Perhaps even one for the current Android module (offline routing), and one which makes sure that the web client is well-behaved, too.

[devemux86]

@michaz really no hard feelings! :slightly_smiling_face:

Not being member of GraphHopper, don't know it's priorities, but can try:
Offline routing specifically on mobile devices differentiates it from other routers.
It's a prime reason that many users combine it with Mapsforge+VTM offline map engines.
So indeed would be nice to have somehow an integration test for commits also on Android.

Unfortunately don't have much time, like I did the old days, to test all GraphHopper commits in several Android versions, but would be beneficial for community.