search

digidem / osm-p2p-db

Peer-to-peer database for OpenStreetMap data
BSD 2-Clause "Simplified" License
235 stars 25 forks source link

Test forked joins #36

Closed gmaclennan closed 7 years ago

gmaclennan commented 7 years ago

@noffle can you review that my test makes sense? Should we merge this test into master?

gmaclennan commented 7 years ago

fixed the test, however, it does not pass, and I think it should, if osm-p2p-db is working how we expect it to work.

hackergrrl commented 7 years ago

Nice! I'll ping you here once I fix the refs bug.

gmaclennan commented 7 years ago

We were waiting on a fix to the "refs bug" here, current status?

hackergrrl commented 7 years ago

Hey, I haven't forgotten about this!

I loaded up this PR and took a look. I fixed up a few things:

  1. Rebased it onto master
  2. Changed the test to use create_db.js instead of the usual setup boilerplate
  3. Fixed a use of osm.del(key, {keys: ...}) to be the correct osm.del(key, {links: ...})
  4. Fixed osm-p2p-db to handle refs correctly when there's a deletion of a deletion document (hadn't thought of this one--nice!)

Check out especially my last commit of the bunch, (5): normally you'd expect that if you delete a way, osm.refs.list(key) wouldn't return that way, right? Well, due to https://github.com/digidem/osm-p2p-db/pull/44, we now surface deletions, which means osm.refs.list(key) will return the deleted way's tombstone document. The changes in (5) tweak your tests to expect this.

The big Q: is this behaviour desirable? I can't remember if we discussed this before, but since I wrote the code that does it, I wonder if we were in agreement about it.

If you're feelings are a resounding "yes!", then I say we're good to merge. :tada:

gmaclennan commented 7 years ago

I have been thinking about this behaviour more, specifically refs on deleted ways/relations. The OSM API does not return refs on a deleted node, and from a user perspective it is confusing which nodes are returned on a deleted way from a fork (like with way4 in the test). However, the behaviour we do want is all heads, including deleted heads, being returned from queries.

From a user perspective, the behaviour I think we want is that a bbox query returns deleted ways/relations if there are >1 heads (a fork) and one of them is not deleted. However, an alternative might be to return the non-deleted way(s)/relation(s) which some kind of attribute that indicates that there is a (deleted) fork, and the client app can then manage lookup of the deleted way/relation.

This is tied to how we index deleted ways. I think current behavour will kind of work how we want it to, since I think we're maintaining refs on deleted ways/relations. Let's discuss this more in person this week.