digidem / osm-p2p-server

Peer-to-peer OpenStreetMap API v0.6 Server for osm-p2p-db
BSD 2-Clause "Simplified" License
87 stars 10 forks source link

Replace ids regardless of input order #32

Closed danielfdsilva closed 7 years ago

gmaclennan commented 7 years ago

Thanks for this @danielfdsilva, I had not realized that the API 0.6 spec does not enforce this order, but I checked and it does not. I am wondering if this might create some strange edge cases because we lose the ordering of the <create>, <modify>, <delete> blocks, whose order is specified in the API. I'm wondering whether it might be safer to just run the map function twice:

var changesWithIds = changes.map(function (change) {
    var mapped = Object.assign({}, change)
    if (change.action === 'create') {
      if (idMap[change.type][change.id] || !change.id) {
        dupIds.push(change.id)
      }
      var id = util.generateId()
      idMap[change.type][change.id] = id
      mapped.id = id
      mapped.old_id = change.id
    }
    return mapped
}).map(function (change) {
    // no need to Object.assign() here because we have already cloned and can mutate
    if (change.type === 'way' && change.nodes) {
      change.nodes = change.nodes.map(function (ref) {
        return idMap.node[ref] || ref
      })
    }
    if (change.type === 'relation' && change.members) {
      change.members = change.members.map(function (member) {
        if (!idMap[member.type][member.ref]) return Object.assign({}, member)
        return Object.assign({}, member, {ref: idMap[member.type][member.ref]})
      })
    }
    return change
  })

I think performance would be about the same and this way would create fewer objects that would need garbage collected.

danielfdsilva commented 7 years ago

@gmaclennan dig it! Changed the code according to suggestion.

gmaclennan commented 7 years ago

@danielfdsilva thanks for this! LGTM

gmaclennan commented 7 years ago

Sorry I forgot to merge this. I think because of the failing test, but that's an outstanding issue that has been there for a while and needs fixed: we don't record the changeset for deletes.