non-functional visibility check in `Relation#to_xml_node` #139

Open
wants to merge 2 commits into
from

Projects

None yet

4 participants

@jfirebaugh
OpenStreetMap on GitHub member

What's going on here? p is either 0 or 1, both of which are truthy in Ruby, so the if block is always executed.

Do we need this check?

@tomhughes
OpenStreetMap on GitHub member

I think you'd probably have to ask @zerebubuth what is supposed to be happening here...

@zerebubuth

Not me, honest! Seems to be something that's almost been there from the beginning (see 78b440f and fa13a11) to optimise visibility lookups for members in a relation/full call. I remember we had problems with these, and we've made improvements to the data integrity since, but it seems that this particular bit of code doesn't need that p check.

@tomhughes
OpenStreetMap on GitHub member

Well what it's trying to do is to make sure that any purported member of a relation which has actually been deleted is not returned - the visible_members thing is a way of optimising that check in some cases but the check itself is being attempted in all cases.

Of course we shouldn't ever see this because you shouldn't be able to delete something that is still a member of a relation but historically the API has tried to hide any such broken data if it does somehow occur.

So the question now is whether to assume that broken data won't occur and get rid of the check, or fix the check...

@jfirebaugh
OpenStreetMap on GitHub member

This is now a pull request that removes the check, if we decide we can go that way.

@pnorman

Perhaps run a query to see if there are any members of visible relations that are not visible?

If both 0 and 1 are true in ruby, then the if check is quite pointless, but should we have a functioning one in its place?

@tomhughes
OpenStreetMap on GitHub member

I've checked the database and there is currently no data that would cause this check (if it worked) to fail, which suggests that our upload validations are working I guess (there is no validation at the database level in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment