[openstreetmap/openstreetmap-website] add bulk_upload (#1995)

[mmd]

 >> The new bulk_upload method is slightly different from old upload method on response in some case, but won't break original logic.

> if it's API compatible then we could take it, but it sounded like it wasn't as there was talk of changes in the response?

I think the new logic in this pull request first processes all create, modify and finally the delete operations, so it could potentially re-order the sequence in which elements are being processed.

While this isn't an issue for e.g. iD (requests are already sorted in this sequence), other API users might send multiple create/modify/delete blocks in more or less random order. If they expect the {old_id,new_id, version} in the response to appear in the same order as in the request, this may cause some issues. Those cases would have to be validated in more detail. This equally applies to other use cases which work in today's implementation (which works step-by-step from top to bottom). One such use case is already mentioned in point 3. above.


> Bigger problem is I'm not sure who would review it

Indeed, that's the most challenging issue. I struggle quite a bit with the Rails syntax, and can't really add much value here. The feedback so far is mostly based on testing, less so from trying to figure out what the code actually does.




-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1995#issuecomment-422465865
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20180918/de1168c0/attachment.html>