openstreetmap / openstreetmap-website

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

Don't include stack backtraces on xml errors when importing traces #5226

Closed AntonKhorev closed 1 month ago

AntonKhorev commented 1 month ago

We've been including stack backtraces in gpx failure notifications since https://github.com/openstreetmap/openstreetmap-website/commit/93dab8a1272f8c807da841c0a55b37f74b65b8c3, but they are not useful for xml parse errors. If your file can't be parsed, the error is inside it, not in the code that called the parser.

Not sending the backtrace in case of xml errors will kind of fix #2630. We're still not going to check file types but the error message is going to be limited to a useful part like:

Fatal error: Start tag expected, '<' not found at :1.
AntonKhorev commented 1 month ago

Alternatively or in addition to this we can check file types. We already do, to detect archives https://github.com/openstreetmap/openstreetmap-website/blob/6595d43e3cda580508a069e174d0059febcdc6ea/lib/gpx.rb#L55-L58 I don't know if we can reliably detect gpx. We could detect obviously wrong file types like jpg, but we're not going to test for all possible types.

tomhughes commented 1 month ago

Well they should be detected as xml but I doubt they will be identified any further than that by file magic.

tomhughes commented 1 month ago

Looks good to me, thanks.

tomhughes commented 1 month ago

Well except that the test runner is spitting out the XML error to the console:

Entity: line 1: parser error : Start tag expected, '<' not found
AntonKhorev commented 1 month ago

It's not test runner spitting, it's the xml library. You can pass an option to suppress it. I was going to do another pull request to silence it when I find all of the places where this can happen.

AntonKhorev commented 1 month ago

I'm thinking about detecting the filetype after an xml error happens.

AntonKhorev commented 1 month ago

Regarding "Entity: line 1: parser error : Start tag expected, '<' not found" message, it's introduced by adding test_parse_error_notification which tries to load JPG file with XML reader.

Do you think that if you upload a binary file without this PR, the error won't be printed?

Perhaps the best would be to redirect stderr to temporary file which could be printed if this test fails?

We don't need this error printed because we're additionally logging it with Rails logger.

I disabled xml library error writing for this case but there's also another path for multi-file archives with different errors. And there's also a changeset diff reader elsewhere.

nenad-vujicic commented 1 month ago

Regarding "Entity: line 1: parser error : Start tag expected, '<' not found" message, it's introduced by adding test_parse_error_notification which tries to load JPG file with XML reader.

Do you think that if you upload a binary file without this PR, the error won't be printed?

Ah, I apologize. You are right, I was thinking only for automated testing and forgot it will be printed also by logger when we manually try to upload some binary file (e.g. JPG) ...

Perhaps the best would be to redirect stderr to temporary file which could be printed if this test fails?

We don't need this error printed because we're additionally logging it with Rails logger.

I disabled xml library error writing for this case but there's also another path for multi-file archives with different errors. And there's also a changeset diff reader elsewhere.

Now it works great, thank you very much. There is no more xml library error, neither during automated tests nor during manually uploading trace files.