openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.08k stars 906 forks source link

PUT /api/0.6/gpx/:id causes internal server error #1982

Open HarelM opened 5 years ago

HarelM commented 5 years ago

When sending a request with a tag that is empty the server responses with a 500 error. I know this is not a valid request, but an error that explains what I sent wrong would help. Here's the invalid request body:

<osm>
  <gpx_file id='2793086' name='2792362.gpx.gz' lat='31.5943' lon='34.6092' user='Harel M' visibility='public' pending='False' timestamp='2018-09-02T11:26:13Z'>
    <description>2792362.gpx.gz</description>
    <tag />
  </gpx_file>
</osm>
tomhughes commented 5 years ago

Looking at the code I think there should be an Error header in the response with the details - that is the standard way that our API reports errors.

HarelM commented 5 years ago

Thanks for the quick response! I'll check it shortly, but shouldn't I get back a HTTP 400 error and not a HTTP 500?

tomhughes commented 5 years ago

There are a range of error codes you might get depending on the error.

HarelM commented 5 years ago

The error message I'm getting back in the body is:

ActiveRecord::RecordNotSaved: Failed to replace tags because one or more of the new records could not be saved.

The same error appears in the Error header in the response. Which is a bit generic - there's no mention of the fact that empty tag is invalid - which is what I would expect as a return error, and I would expect 400 status code since the data I'm sending is invalid, as far as I understand. When my server returns 500 I know there's a problem in my server's code, but maybe this is just me... Feel free to close this issue if returning 500 is a valid case for you.

tomhughes commented 5 years ago

Well the 500 is a callback - if there is no defined mapping for a given exception then it uses that.

I'm not saying it couldn't do better, but I doubt anybody is going to be rushing to fix it.

mmd-osm commented 5 years ago

Easiest would be to raise some error message, if the tag content is empty:

trace.rb/update_from_xml_node

    self.tags = pt.find("tag").collect do |tag|
      raise OSM::APIBadXMLError.new("trace", pt, "Tag value may not be empty") if tag.content.empty?
      Tracetag.new(:tag => tag.content) 
    end