streetcomplete / StreetComplete

Easy to use OpenStreetMap editor for Android
https://streetcomplete.app
GNU General Public License v3.0
3.85k stars 352 forks source link

412 Precondition Failed on Upload #1411

Closed westnordost closed 5 years ago

westnordost commented 5 years ago

Recently, I am getting some error reports like this on upload:

de.westnordost.osmapi.common.errors.OsmApiException: Precondition Failed (412) - Way xxxxxxxxx requires the nodes with id in xxxxxxxxxxxx, which either do not exist, or are not visible.
at de.westnordost.osmapi.OsmApiErrorFactory.createGenericError(OsmApiErrorFactory.java:49)
at de.westnordost.osmapi.OsmApiErrorFactory.createError(OsmApiErrorFactory.java:40)
at de.westnordost.osmapi.OsmConnection.handleResponseCode(OsmConnection.java:349)
at de.westnordost.osmapi.OsmConnection.makeRequest(OsmConnection.java:195)
at de.westnordost.osmapi.OsmConnection.makeAuthenticatedRequest(OsmConnection.java:161)
at de.westnordost.osmapi.map.MapDataDao.uploadChanges(MapDataDao.java:120)
at de.westnordost.streetcomplete.data.osm.upload.OsmQuestChangeUpload.uploadQuestChange(OsmQuestChangeUpload.java:133)
at de.westnordost.streetcomplete.data.osm.upload.OsmQuestChangeUpload.upload(OsmQuestChangeUpload.java:100)
at de.westnordost.streetcomplete.data.osm.upload.AOsmQuestChangesetsUpload.uploadAndHandleChangesetConflict(AOsmQuestChangesetsUpload.java:166)
at de.westnordost.streetcomplete.data.osm.upload.AOsmQuestChangesetsUpload.upload(AOsmQuestChangesetsUpload.java:107)
at de.westnordost.streetcomplete.data.upload.QuestChangesUploadService.onHandleIntent(QuestChangesUploadService.java:129)
at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:65)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:135)
at android.os.HandlerThread.run(HandlerThread.java:61)

So, the upload failed because some nodes were removed from the way in question and StreetComplete tried to upload an old version of the way where these nodes still exist. The API (currently) checks for the existence of the nodes first before checking if the version the app is trying to upload is the current one. This is probably a bug in the new cgimap implementation of the OSM API: https://github.com/zerebubuth/openstreetmap-cgimap/issues/194

If this is not fixed there, the error code 412 needs to be treated the same as a conflict in the future.

gmischler commented 5 years ago

Treating precondition errors the same way as conflicts sounds reasonable in any case.

But maybe this is an opportunity to think about a general concept of how to deal with unexpected API errors. Right now, especially when immediate uploads are turned off, all further uploads are blocked until either "the other side" stops complaining or the local data are deleted (luckily my remaining queue was short at that time). Besides sending a bug report, an option to skip and ignore the problem item would make such situations a bit less annoying for the user.

Added: I also just noticed that on each such error, an empty change set gets left behind. If no other contents have been uploaded yet, then the app should probably cancel the current change set on the server.

mmd-osm commented 5 years ago

Empty changesets really cause no harm, you can simply leave them as is, they automatically get closed after 1 hour.

westnordost commented 5 years ago

@gmischler Currently,

For download,

are all treated like temporary problems: the download is cancelled and a message is displayed to the user along the lines of "try again later". For any other error encountered, the app asks the user to send a report to me vie email.

For upload

are treated like temporary problems and a message like "try again later" is displayed.

Answer codes 401 and 403 are treated in the way that the user is forced to reauthenticate the app with OAuth.

Answer code 409 is treated like described here. (Tries to solve conflict automatically or drops quest answer if it cannot).

For any other error encountered, the app asks the user to send a report to me vie email.

westnordost commented 5 years ago

all further uploads are blocked until either "the other side" stops complaining or the local data are deleted (luckily my remaining queue was short at that time). Besides sending a bug report, an option to skip and ignore the problem item would make such situations a bit less annoying for the user.

I understand that this is annoying. However, if there is a 4xx error, so, a client error, it could occur for any other upload as well. After all, StreetComplete is talking to only one API to upload - so the same API for each quest upload. It would be even more annoying for the user to find out that this error occurs for every single upload. And more annoying to possibly receive a bug report for every single upload. If the app receives a 4xx error (that it not already handles), it is a general error that needs to be fixed, not mitigated by having the possibility to skip the upload of that one answer.

Any upload that cannot be uploaded successfully will stay in the local storage until it can be uploaded. So, nothing is lost when there is a (temporary) problem with uploading.

mmd-osm commented 5 years ago

Fix is deployed on production now.

westnordost commented 5 years ago

Cool! You are so quick, thank you!

mmd-osm commented 5 years ago

It’s easy when the bug report already includes the solution ;)