Throw an exception if the input GPX file doesn't contain trackpoints

[Nakaner]

Otherwise the result returned by the map matching API would be an GPX file without any trackpoints.

Without this patch:

michael@hatano:~/git/github.com/graphhopper/map-matching$ curl -XPOST -H "Content-Type: application/gpx+xml" -d @matching-core/src/test/resources/test-only-wpt.gpx "localhost:8989/match?vehicle=car&type=gpx"
<?xml version="1.0" encoding="UTF-8" standalone="no" ?><gpx xmlns="http://www.topografix.com/GPX/1/1" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" creator="Graphhopper version 0.11.0" version="1.1" xmlns:gh="https://graphhopper.com/public/schema/gpx/1.1">
<metadata><copyright author="OpenStreetMap contributors"/><link href="http://graphhopper.com"><text>GraphHopper GPX</text></link><time>2018-06-14T11:34:16Z</time></metadata>
<trk><name>GraphHopper Track</name><trkseg>
</trkseg>
</trk>
</gpx>

With this patch, an error message will be returned (the wrong format is a different issue and will be fixed by https://github.com/graphhopper/graphhopper/pull/1390

curl -XPOST -H "Content-Type: application/gpx+xml" -d @matching-core/src/test/resources/test-only-wpt.gpx  "localhost:8989/match?vehicle=car&type=gpx"
{"code":500,"message":"There was an error processing your request. It has been logged (ID 7fb0f76e14f13322)."}

Returning an empty GPX response is confusing.

[michaz]

Hmmmm. How is an empty GPX response to an empty GPX request confusing? Am I being too literal-minded here? Map-matching is an operation on lists, and an empty list is a list, too. How is 0 points more special than, say, 1 point?

I'd say we return an empty list for the same reason that "".reverse() returns "" instead of throwing an Exception.

Or am I missing something, like an empty GPX file being really illegal (not an expert on GPX files)?

[michaz]

Arguments that would convince me otherwise would be, for example:

• If I convert it to a LineString and try to draw it, it fails, because empty LineStrings are illegal. (Is that true?) EDIT: Apparently not, but LineStrings of length 1 are illegal.
• Bad input GPX files are interpreted as empty GPX files, so I get an empty GPX response to a bad input, instead of an error message. That would indeed be confusing. But then I would consider that to be the error.

[karussell]

I also think an empty GPX file should not be handled like an error. Ie. not the exception in the parsing method and especially not as a server error 5xx.

Maybe we handle it as a usage problem and throw a 400 for an empty list i.e. wrong usage? (We do this for /route if no points)

If I convert it to a LineString and try to draw it, it fails, because empty LineStrings are illegal. (Is that true?)

No. An empty LineString is ok as I recently found out :)

[michaz]

Maybe we handle it as a usage problem and throw a 400 for an empty list i.e. wrong usage? (We do this for /route if no points)

But my point was: What's wrong about it? Literally, what's wrong with empty input -> empty output? I tend to think that it's a good thing when code does not fail in corner cases (like throwing exceptions on n=0), so I'm a bit reluctant to let it fail explicitly, without need.

[michaz]

I'm in traffic modelling, so I think of a GPX file as, for example, "things a person was known to be doing on a day". What's wrong with "nothing"?

[karussell]

There is nothing wrong :)

Still we could prevent certain incorrect usage (not saying that we should). What if someone intends to map match a GPX file with wpt and as we consider trk only we would return an empty response? So we would return nothing to a GPX input file filled with content without a warning.

(But we could also just document this behaviour explicitly and change nothing else)

[Nakaner]

Or am I missing something, like an empty GPX file being really illegal (not an expert on GPX files)?

I am no GPX expert either but please keep in mind that GPX files can contain waypoints, tracks (with track segments containing trackpoints) and routes.

I am developing map matching for my railway routing. Unfortunately, my customer took "supports GPX" too literal. He converted a CSV file of measurements into a GPX file using ogr2ogr and had a file of waypoints, not of trackpoints. Luckily, my MatchResource class is implemented a bit different than yours and that's why he got the error message of an exception raised at a later step in the processing (of the map matching). Default GraphHopper would have returned an empty track and my client application to render it—or fail (not tested).

Did you notice that if someone sends a GPX file with more than waypoints or routes, the response only contains the result of the matching of the trackpoints to the network?

My aim is to throw an error if a GPX file is valid (in terms of XML and the schema) but does only contain unuseable items (waypoints and routes). GraphHopper only reads trackpoints. The check if the list of GPXEntry is empty was the easiest solution. Shall I check if the XML document contains any waypoints or routes and throw an error only if no trackpoints but at least one route or waypoint is in the GPX file?

I also think an empty GPX file should not be handled like an error. Especially not as a server error 5xx.

The 500 response is caused by a minor incompatibility of the map matching module and current GraphHopper master branch. I added it to the unit test in order to make the unit tests pass. It is related to https://github.com/graphhopper/graphhopper/issues/1389

You can leave this pull request open until https://github.com/graphhopper/graphhopper/pull/1390 is merged. I will change my pull request in order to return code 400 then.

[michaz]

My aim is to throw an error if a GPX file is valid (in terms of XML and the schema) but does only contain unuseable items (waypoints and routes). GraphHopper only reads trackpoints.

Okay got it, thanks!

[Nakaner]

Shall I add NodeList wpl = doc.getElementsByTagName("waypoint"); and look if it is empty (and the same for route points)?

[michaz]

Shall I add NodeList wpl = doc.getElementsByTagName("waypoint"); and look if it is empty (and the same for route points)?

Almost -- I moved your test to the front, and now consider a GPX input without tracks to (very probably) be a user error.

Properties of that solution:

• A user (like your customer) who doesn't convert their data to tracks, but to waypoint or routes, gets a helpful error message.
• A user who does convert their data to tracks, but sends an empty dataset, will not get an error, because it contains a track, but without trackpoints.
• A user who additionally has waypoints or routes in their GPX does not get an error. This is very likely deliberate, since it is very hard to create a mixture of three data types by accident.
• We don't say that an empty list of points is an illegal map-matching input.
• We don't say that a GPX file without tracks is an illegal GPX file, we just don't want it in that web service.