ngageoint / hootenanny

Hootenanny conflates multiple maps into a single seamless map.
GNU General Public License v3.0
358 stars 74 forks source link

Make translation server more defensive and responsive when it is sent garbage #5617

Open maxgrossman opened 1 year ago

maxgrossman commented 1 year ago

5616 motivates making translation server more interrogative of payloads so we can start returning responses that are more useful.

We should start with the problem at hand. If a client sends an empty payload, return a 400 response.

Being curious about error handling in general, I tried sending some bad requests and our responses or lack there of might confuse crowds new to hoot.

For example, if I make a get request to /translateFrom and misspell the translation...

curl http://localhost:8094/translateFrom?translation=TDSv701&fcode=AP030&geom=Line`

I'm returned {"error":"Cannot read property 'toOsm' of undefined"}. Might be better to inform the translation is unknown and give a hint to where the translation could be checked, the /translations endpoint.

Or if I were post to /translateTo with valid data but do a similar misspell...

curl -X POST 'http://localhost:8094/translateTo?translation=TDSv701' -d '<osm version="0.6" upload="true" generator="JOSM"><node id="-1" lon="0" lat="0" version="0"><tag k="building" v="yes"/></node></osm>'

We do not even respond! I get curl: (52) Empty reply from server.

First chunk of work is manage the when payload is empty. We can tackle these other problems and see what else we can throw at the service.

maxgrossman commented 1 year ago

One thing #5619 revealed is that when the error happens inside one of the hoot binding functions like loadMapFromString, the error does not also lead to a node.js error being thrown.

If i make a request like curl -X POST 'http://localhost:8094/translateTo?translation=TDSv70' -d 'not_xml' I am returned the empty response message you see above.

If I hardcode something I know will throw a node js error into the try catch block like undefined.foo() I get a catch-able generic error and an appropriate 500 status can be replied.

Is this a known behavior of our node.js bindings @bmarchant?