Tests and fixes of potential duplicate consecutive node issues in way.js #3676
Conversation
replaceNode, and add some comments.
|
Hey @slhh sorry for taking so long to look at this. I think it's a pretty good improvement. I agree that range checking the index argument and removing the duplicates is worth doing. One thing that worries me is that there may be places elsewhere in the code where we are calling I normally hate exceptions, but I think in this case I'd almost rather throw an out of range exception when trying to call these functions with a weird index value. Rather than silently succeed by adding the node to the beginning or end or something like that. That way we can track down places in the code where the assumptions fall short.
I guess it's a judgement call, but I think the case for exceptions is better for us for debugging and better for OSM not getting bad data put into it. What do you think? Again, overall this looks totally fine.. My worry is about other code in iD that assumes the methods internally use |
This worried me too, therefore, I tried to reproduce the splice behavior. All the index checks are intended to do this, and don't limit the useable index range. I an earlier version I haven't supported the negative index range. The one direct test with negative index failed, but no additional higher level tests failed. And a little manually testing hasn't shown an issue. In case we want to limit the useable index range, which makes sense in my opinion, an exception seems to be useful. Can we throw it in case of debugging only? An exception in the field is only useful in case we want to address the resulting issue with high priority. We should not impair user experience just to fill the bug tracker without working on it. Provided we prevent duplicate nodes in incoming data from the database, we may also throw an exception in case the functions have to remove duplicates. We do only need to compare the resulting lenght with the expected length. I'm not sure if we want to let calling code rely on way.js for duplicate node prevention or want to have all required rules for prevention in the calling code. In the later case an exception can be useful to find the bug. |
The other notable place we throw an exception in iD is in So yes, anytime a user reports something like that, it's very high priority to get it fixed.
I don't think we want an exception when removing duplicate consecutive nodes because I'm sure there are some cases of this in existing OSM data, which is pretty messy from imports, old bugs, even people just messing around with the api. For a little while there were even some ways referencing nodes that had been deleted (there is no referential integrity in the OSM database). |
Just to clarify, this is wrong, and in the way nodes table node_id is a foreign key: https://github.com/openstreetmap/openstreetmap-website/blob/c83f15821d5a48dd424cc5b534939d7f30a9fae0/db/structure.sql#L2184-L2189 |
|
@bhousel I have added a commit changing it to throw exceptions. Feel free to use it as you like.
The best would be if iD were catching the exception, reporting the error, and resuming from the end of the last successful action. I don't know how complex this would be, at least my current knowledge is too limited to do it.
I would agree, but for the reason of making actions easier due to relying on way.js for duplicate prevention.
Of course they are, but that's not a reason, we just had to add some code eliminating the duplicate e.g. in the OSM service. That's why I wrote: "Provided we prevent duplicate nodes in incoming data from the database"
You have missed iD's still existing bugs here. I have watched iD generating a duplicate when testing for my other pull request, which is nicely monitoring such duplicates. I don't think the problem was due to my code modification. There were only the modification of the sidebar, but not the changes to mode select at that stage. And the sidebar haven't been opened when the bug occurs. |
Good find though, can you open a separate issue for this? Maybe it's possible to double click around the endpoint of a way and have iD try to add a midpoint there that ends up merging into the existing node. |
|
Just a quick update, I'm adding a lot of tests (like hundreds) and changing slightly the logic to handle different kinds of duplicates in different places along the nodelist, possibly involving connecting nodes on circular ways. Once we throw those into the mix, things get kind of complicated. It might take another day or two to merge fully, but this work should make these functions a lot safer in the end. |
@bhousel
That was my first idea too, but I haven't been able to reproduce it. Maybe its needs to be too fast to be reproduced intentionally. |
Yes, reading #1296 again, it sounds like you found one of the pathways to cause it. You can add the info there and it hopefully will be fixed by this PR. I wanted to merge this yesterday, but I realized that our area drawing mode does intentionally splice the node array using index |
These changes are needed now that `addNode` * wants to preserve circularity * automatically remove duplicates * range checks index argument (can't call it with -1 anymore)
|
Thanks @slhh for kicking this off.. I just merged the updated version! |
I have added some tests for replaceNode, updateNode, addNode, and removeNode. These are partially failing with the original code, due to some potential issues in view of duplicate nodes.
Reworked code passing these tests is in the second commit.
In addition I have added some code comments.
Related issues are #1296 and #3211.