fix: Nearest API returning node ID 0 at start of one-way road#7427
Merged
DennisOSRM merged 2 commits intomasterfrom Mar 24, 2026
Merged
fix: Nearest API returning node ID 0 at start of one-way road#7427DennisOSRM merged 2 commits intomasterfrom
DennisOSRM merged 2 commits intomasterfrom
Conversation
When a location snaps to the beginning of a one-way forward segment (fwd_segment_position == 0), the from_node was left as 0 because neither the reverse_segment branch nor the fwd_segment_position > 0 branch applied. Fix: add an explicit else-if for fwd_segment_position == 0 that swaps from/to so the edge runs forward (a → x) instead of returning a zero ID. Addresses code review comments from PR #7048: - Restructured as a separate top-level else-if (per @afarber suggestion) - Added BOOST_ASSERT(forward_geometry.size() >= 2) guard - Added cucumber test verifying non-zero node IDs on a one-way road snap - Updated CHANGELOG Co-authored-by: Muteeb Ali <31405887+muteebali@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an edge-case in the Nearest API where snapping to the very start of a one-way segment could return an uninitialized from_node (serialized as node ID 0) by ensuring a valid (from,to) node pair is always produced, and adds a regression test for it.
Changes:
- Fix
NearestAPI::MakeNodesto handlefwd_segment_position == 0on one-way forward geometry by returning the first two geometry nodes (a → x) instead of leavingfrom_nodeas0. - Add a cucumber regression scenario covering “snap at start of one-way” and verifying waypoint node IDs.
- Extend nearest cucumber step definitions to validate
waypoints[0].nodesvia anodestable column.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/engine/api/nearest_api.hpp | Adds explicit handling for fwd_segment_position == 0 to avoid returning nodes[0] = 0 on one-way segments. |
| features/step_definitions/nearest.js | Adds support for asserting the nodes array in Nearest API responses from cucumber tables. |
| features/nearest/nodes.feature | New regression test scenario reproducing the one-way start snap bug and asserting returned node IDs. |
| CHANGELOG.md | Documents the Nearest API bugfix under Unreleased routing fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+49
to
+54
| } else if (key === 'nodes') { | ||
| const nodeNames = row.nodes.split(',').map(n => n.trim()); | ||
| const fromNode = this.findNodeByName(nodeNames[0]); | ||
| const toNode = this.findNodeByName(nodeNames[1]); | ||
| if (!fromNode) throw new Error(util.format('*** unknown from-node "%s"', nodeNames[0])); | ||
| if (!toNode) throw new Error(util.format('*** unknown to-node "%s"', nodeNames[1])); |
There was a problem hiding this comment.
The nodes column parsing assumes row.nodes always contains exactly two comma-separated node names. If the value is missing a comma (or has extra entries), nodeNames[1] becomes undefined and findNodeByName(undefined) will throw a generic TypeError (accessing .length) or silently ignore additional nodes. Consider validating that the split yields exactly 2 non-empty names and throwing a clearer error (e.g. nodes must be from,to).
Validate that the nodes column contains exactly two comma-separated, non-empty node names before calling findNodeByName, giving a clear error message instead of a cryptic TypeError on malformed input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #7047 — the Nearest API was returning
nodes[0] = 0when a queried location snaps to the very beginning of a one-way road segment (fwd_segment_position == 0). In that case, neither thereverse_segmentbranch nor thefwd_segment_position > 0branch applied, leavingfrom_nodeuninitialised (zero).Supersedes #7048 — this PR is a clean rebase of that work onto current master, with all code review feedback addressed.
Changes
include/engine/api/nearest_api.hpp: Add explicitelse if (fwd_segment_position == 0)branch that swapsfrom_node = to_nodeand setsto_nodeto the next node, so the edge runs forward (a → x) instead of returning a zero ID. AddsBOOST_ASSERT(forward_geometry.size() >= 2)guard.features/nearest/nodes.feature: New cucumber scenario that reproduces the bug (one-way road, query at start node) and verifies both node IDs are valid and non-zero.features/step_definitions/nearest.js: Extend nearest step definitions to support anodescolumn for verifyingwaypoints[0].nodesin test tables.CHANGELOG.md: Updated.Co-authorship
The original fix was proposed by @muteebali in #7048. This PR incorporates that work with the formatting and structural improvements requested during code review.