Re: [Potlatch-dev] [OSM-talk] Way with only one single node
On Thu, Mar 22, 2012 at 3:27 AM, Andy Allan wrote: > Finally, in other projects I follow the convention of "don't code > defensively", as described at > http://www.erlang.se/doc/programming_rules.shtml#HDR11 and elsewhere. Quote: "The function will crash if Option neither normal nor all, and it should do that. The caller is responsible for supplying correct input. " So in their example, the worst that happens is a function fails to do something (and probably raises an exception which is caught at a higher level). In our case, we write data to a shared database that the user has no way of detecting and retrieving (in the case of one-length ways). Defensive programming has its place. > The save code should "trust" that the data is good. If the data isn't > good, whatever made it bad should be fixed. Otherwise it's turtles all > the way down - should the save code check the ids are actually > numerical, just in case, should it do x, y or z, just in case? If any of those things are genuine possibilities, are not checked for by server-side code, and would be harmful, then checking is worth considering. IMHO. > So the > answer is that the input should be checked, somewhere in the > controller code, and then it should be trusted. And our strategy for preventing future such bugs? >My initial thought would be to not create the way until the second >node was drawn. I think that would also remove a lot of the gnarlyness >around POI creation, where we are footling with the >way-that-we-never-actually-wanted as we finish handling the double >click. FYI that change will break my recent fix for undo/redo of new ways. Steve ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] [OSM-talk] Way with only one single node
On 21 March 2012 16:34, Richard Fairhurst wrote: > I've corresponded with Thomas B and found some, ahem, fairly easy steps to > reproduce: > > 1. Click on map (_once_) to start new way > 2. With elastic band still engaged, click 'Save' > 3. Whatever the opposite of PROFIT is > > Will look at it this weekend I guess. My initial thought would be to not create the way until the second node was drawn. I think that would also remove a lot of the gnarlyness around POI creation, where we are footling with the way-that-we-never-actually-wanted as we finish handling the double click. Also, if anyone smarter than I am (Dave?) wants to point me in the right direction, I'd love to bind the controller state and the data state together a bit better, so that when you're doing things like "undo" you not only change the data, but put the controller into the appropriate state at the same time. That's only tangentially related to the above problem, however. Cheers, Andy ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] [OSM-talk] Way with only one single node
Andy wrote: > Anyway, mumble grumble unit testing. When we find out what's the > trigger, for the love of god someone should help me write the unit > test so that when it's fixed, it stays fixed. I've corresponded with Thomas B and found some, ahem, fairly easy steps to reproduce: 1. Click on map (_once_) to start new way 2. With elastic band still engaged, click 'Save' 3. Whatever the opposite of PROFIT is Will look at it this weekend I guess. cheers Richard ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] [OSM-talk] Way with only one single node
On 21 March 2012 12:56, Steve Bennett wrote: > 1) We can fix this, but it's hard to prevent it coming back when new > features are built. I've probably written such bugs. Mumble grumble unit testing. > 3) I still think we should do this. We have a responsibility to not > mess up the database. Saying "well, we should avoid writing bugs" > isn't addressing that: we should both implement this failsafe > mechanism, *and* avoid writing such bugs. Well, I'm not sure I agree on this one. If it was a truly awful thing to do, the API would prevent it. Given that it's not truly awful, it's not something to worry about too much. It's not messing up referential integrity, it's just something unexpected and, at the moment, seemingly useless. Furthermore, handling it at save time is, to me, the wrong thing to do. What do we do when we find our data has an issue? Alert the user? Silently swallow it? Neither is a good thing. Should that way be deleted? Should a node be re-inserted? What did the mapper mean to happen? We can't tell. Finally, in other projects I follow the convention of "don't code defensively", as described at http://www.erlang.se/doc/programming_rules.shtml#HDR11 and elsewhere. The save code should "trust" that the data is good. If the data isn't good, whatever made it bad should be fixed. Otherwise it's turtles all the way down - should the save code check the ids are actually numerical, just in case, should it do x, y or z, just in case? So the answer is that the input should be checked, somewhere in the controller code, and then it should be trusted. Anyway, mumble grumble unit testing. When we find out what's the trigger, for the love of god someone should help me write the unit test so that when it's fixed, it stays fixed. Thanks, Andy ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] [OSM-talk] Way with only one single node
On Wed, Mar 21, 2012 at 10:14 PM, Richard Fairhurst wrote: > 1. In the UI (i.e. don't let the user create them in the first place) > 2. In the entity model and actions (i.e. don't let them get into P2's > internal data) > 3. In the upload code (i.e. don't let them be transmitted to the server) > > Historically P2 has tried to tackle it at 1 - don't let the user create > them. Clearly something's falling through the gaps and I'm in > communication with Thomas B to see if we can find out where there is. > > 2 is also a reasonable place to address it. I recently did this with empty > relations which, like 1-length ways, aren't actually forbidden by the data > model but are discouraged by the community: > https://github.com/systemed/potlatch2/commit/f762b5c7b33c0ff4ecbd37bc1db37634cedb3d8a > > You'll see that, when you remove a member from a relation, this code now > deletes the relation instead if it would result in it having 0 members. We > may be able to do something similar here, though I suspect it would be > less easy. One downside to this approach is that although zero-member relations and one-node ways are useless to save to the database, they may be useful within a session. There's nothing especially wrong with removing every way from a relation, creating some new ways, then adding the relation to those ways. > I'm less keen on 3. Conceptually, uploading should just be a matter of > "send the changes in P2's internal dataset to the server". Adding a check > here is a bit of a sticking plaster - it works but it doesn't fix the > underlying problem. I'd rather see if, with the help of some steps to > reproduce, we can fix 1 and/or 2 first before resorting to 3. I'd say: 1) We can fix this, but it's hard to prevent it coming back when new features are built. I've probably written such bugs. 2) Yeah, reasonable. Strengthen the model interaction to avoid these issues. 3) I still think we should do this. We have a responsibility to not mess up the database. Saying "well, we should avoid writing bugs" isn't addressing that: we should both implement this failsafe mechanism, *and* avoid writing such bugs. (And also, as noted above, I don't fully agree with your concept that uploading is a perfect replication of in-memory data.) Steve ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] [OSM-talk] Way with only one single node
[copied to potlatch-dev, followups probably better there] Steve Bennett wrote: > One thing we could add (in addition to trying to fix bugs in the > various places 1-length ways could be created) would be a > general filter at save time that prevents any 1-length ways > being sent back to the database. What do you think? There's three places you can theoretically trap it: 1. In the UI (i.e. don't let the user create them in the first place) 2. In the entity model and actions (i.e. don't let them get into P2's internal data) 3. In the upload code (i.e. don't let them be transmitted to the server) Historically P2 has tried to tackle it at 1 - don't let the user create them. Clearly something's falling through the gaps and I'm in communication with Thomas B to see if we can find out where there is. 2 is also a reasonable place to address it. I recently did this with empty relations which, like 1-length ways, aren't actually forbidden by the data model but are discouraged by the community: https://github.com/systemed/potlatch2/commit/f762b5c7b33c0ff4ecbd37bc1db37634cedb3d8a You'll see that, when you remove a member from a relation, this code now deletes the relation instead if it would result in it having 0 members. We may be able to do something similar here, though I suspect it would be less easy. I'm less keen on 3. Conceptually, uploading should just be a matter of "send the changes in P2's internal dataset to the server". Adding a check here is a bit of a sticking plaster - it works but it doesn't fix the underlying problem. I'd rather see if, with the help of some steps to reproduce, we can fix 1 and/or 2 first before resorting to 3. cheers Richard ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
