fix(car): classify access=unknown as restricted instead of fully allowed - #7670
Conversation
access=unknown was in access_tag_whitelist, so it was treated identically to access=yes: fully allowed, no restricted class, no distinction from a genuinely verified public way. Move it to restricted_access_tag_list instead, the same tier private/destination/delivery/customers/permit/residents already use. Routability is unchanged (restricted-tier values are not excluded by default, see way_handlers.lua's access_tag_blacklist check being skipped when forward_restricted/backward_restricted is true), this only fixes the "restricted" edge classification so API consumers can see the distinction and, if they choose to, treat it differently. See Project-OSRM#6762. Signed-off-by: Julien Etienne <etienne.julien@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adjusts the car Lua profile’s access classification so the literal OSM tag value access=unknown is treated as a “restricted” access value (instead of being whitelisted as fully allowed), and adds a cucumber scenario to assert the resulting restricted class tagging in routes.
Changes:
- Move
unknownfromaccess_tag_whitelisttorestricted_access_tag_listinprofiles/car.lua. - Add a cucumber scenario in
features/car/classes.featureto verifyaccess=unknownresults in therestrictedclass (including on motorways).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| profiles/car.lua | Reclassifies access=unknown from “whitelisted” to “restricted-tier” access handling. |
| features/car/classes.feature | Adds regression coverage asserting access=unknown produces the restricted class in route annotations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'customers', | ||
| 'permit', | ||
| 'residents', | ||
| 'unknown', | ||
| }, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7670 +/- ##
==========================================
+ Coverage 90.75% 94.15% +3.40%
==========================================
Files 484 484
Lines 37688 37688
==========================================
+ Hits 34205 35487 +1282
+ Misses 3483 2201 -1282 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DennisOSRM
left a comment
There was a problem hiding this comment.
The reasoning holds up. I checked the OSM wiki page for access=unknown, and it confirms the tag means the access status was not surveyed. It does not mean the way is open. In fact, mappers often use this tag specifically when they suspect a restriction but cannot confirm it. So moving it out of the full-access whitelist and into the restricted tier is the right call.
I also verified the routability claim in way_handlers.lua. Restricted-tier values are not blacklisted by default, so no ways become impassable. This only fixes the API classification.
One small note: this also changes turn penalty behavior, since restricted ways get a high penalty unless they are the start or end of a route. Worth mentioning in the PR description for future reference. Not a blocker.
Nice, well-scoped fix. Approved.
Issue
Related to #6762, which raises the broader problem (car profile whitelists all unrecognized access values, including the literal
access=unknown,unknownis even named explicitly in that issue's own real-world test data as one of the values whose current handling changes under a stricter fix).This PR takes a deliberately more conservative position than the patch linked in #6762. That patch treats unrecognized/
unknownaccess values as fully inaccessible. This PR only addresses the literalaccess=unknowntag value (a real, documented OSM tagging convention meaning "access conditions are unverified, survey needed", see https://wiki.openstreetmap.org/wiki/Tag:access%3Dunknown) and moves it fromaccess_tag_whitelistintorestricted_access_tag_list, the same tierprivate/destination/delivery/customers/permit/residentsalready occupy.Reasoning for the softer treatment specifically for
unknown: the tag doesn't assert a known restriction the wayprivate/destination/nodo, it asserts the opposite, that access status hasn't been determined. Treating it as an outright block asserts something the tag doesn't actually claim. Restricted-tier values aren't excluded from routing by default in this profile today (seeway_handlers.lua'saccess_tag_blacklistcheck being skipped whenforward_restricted/backward_restrictedis set), so this change doesn't alter default routability at all, it only gives these ways a correct"restricted"edge class instead of being indistinguishable fromaccess=yes. I think this is worth maintainers' consideration as an alternative (or complement) to the broader fix in #6762, not a replacement for it.Was this change primarily generated using an AI tool? Yes, Claude (Anthropic, Claude Sonnet 5), based on manual reading of
profiles/car.lua,profiles/lib/access.lua, andprofiles/lib/way_handlers.lua, and cross-checked against the existingaccess.feature/classes.featuretest suites.Tasklist
features/car/classes.feature, modeled on the existing "We tag restricted with a class" test; existingaccess.featureroutability assertions forunknownwere checked by hand and remain valid since routability itself doesn't change)Requirements / Relations
References #6762 (broader/stricter version of the same underlying gap). Not based on or blocking that PR's own patch.