#7047: Nearest api returning node with id 0 by muteebali · Pull Request #7048 · Project-OSRM/osrm-backend

muteebali · 2024-10-04T09:35:22Z

Issue

Bug: Nearest api returning node with id 0

Tasklist

Change the nearest_api.hpp code
Update ChangeLog.md

Snapshot of bug fix

FYI @jcoupey

github-actions · 2025-04-07T02:08:34Z

This PR seems to be stale. Is it still relevant?

moboge · 2025-04-07T06:32:15Z

@jcoupey any idea on when this PR can be merged into the master?

DennisOSRM · 2025-04-07T18:24:03Z

i have this on my plate for the coming week.

DennisOSRM · 2025-04-16T11:18:29Z

Change looks good to me. What we'd need is a test case that fails without this change and succeeds with it.

afarber · 2025-08-29T08:38:13Z

+                    forward_geometry(phantom_node.fwd_segment_position - 1));
            from_node = static_cast<std::uint64_t>(osm_node_id);
        }
+            else if (phantom_node.fwd_segment_position == 0)


Wouldn't this be more readable?

else if (phantom_node.forward_segment_id.enabled && phantom_node.fwd_segment_position == 0) { // At the very beginning of a one-way forward geometry. // Segment runs from the first OSM node to the second OSM node. // We require at least 2 nodes in forward_geometry to form a valid edge. BOOST_ASSERT(forward_geometry.size() >= 2); auto from_osm_node = facade.GetOSMNodeIDOfNode(forward_geometry[0]); auto to_osm_node = facade.GetOSMNodeIDOfNode(forward_geometry[1]); from_node = static_cast<std::uint64_t>(from_osm_node); to_node = static_cast<std::uint64_t>(to_osm_node); }

github-actions · 2026-02-26T02:16:03Z

This PR seems to be stale. Is it still relevant?

Copilot

Pull request overview

This PR fixes a bug in the Nearest API where it was returning a node with id 0 when a location snaps to the beginning of a one-way road segment. The issue occurred specifically when fwd_segment_position == 0 on one-way roads, where the from_node was left uninitialized (defaulting to 0) instead of being properly set.

Changes:

Added handling for the edge case when fwd_segment_position == 0 in one-way segments
Updated CHANGELOG.md to document the bug fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
include/engine/api/nearest_api.hpp	Adds special handling for when a location snaps to the very beginning (position 0) of a one-way forward segment, ensuring both from_node and to_node are properly set instead of leaving from_node as 0
CHANGELOG.md	Documents the fix for issue #7047 in the changelog under the Misc section

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-27T17:24:22Z

            from_node = static_cast<std::uint64_t>(osm_node_id);
        }


The indentation of this line is inconsistent with the rest of the codebase. It should be indented by 4 more spaces to align with the opening of the if block on line 155. Proper indentation improves code readability and makes the control flow clear.

Suggested change

from_node = static_cast<std::uint64_t>(osm_node_id);

}

from_node = static_cast<std::uint64_t>(osm_node_id);

}

Copilot · 2026-02-27T17:24:22Z

+            else if (phantom_node.fwd_segment_position == 0)
+            {
+                auto osm_node_id = facade.GetOSMNodeIDOfNode(
+                    forward_geometry(phantom_node.fwd_segment_position + 1));


When accessing forward_geometry(phantom_node.fwd_segment_position + 1), there's no assertion or validation that the forward_geometry has at least 2 elements (indices 0 and 1). Following the pattern used elsewhere in the codebase with BOOST_ASSERT, consider adding an assertion like BOOST_ASSERT(forward_geometry.size() >= 2) before accessing position+1 to ensure the geometry has sufficient elements to form a valid edge. This would help catch invalid data early in debug builds.

Copilot · 2026-02-27T17:24:23Z

+        else if (phantom_node.forward_segment_id.enabled)
        {
            // In the case of one way, rely on forward segment only
-            auto osm_node_id =
-                facade.GetOSMNodeIDOfNode(forward_geometry(phantom_node.fwd_segment_position - 1));
+            if (phantom_node.fwd_segment_position > 0)
+            {
+                auto osm_node_id = facade.GetOSMNodeIDOfNode(
+                    forward_geometry(phantom_node.fwd_segment_position - 1));
            from_node = static_cast<std::uint64_t>(osm_node_id);
        }
+            else if (phantom_node.fwd_segment_position == 0)
+            {
+                auto osm_node_id = facade.GetOSMNodeIDOfNode(
+                    forward_geometry(phantom_node.fwd_segment_position + 1));
+                from_node = to_node;
+                to_node = static_cast<std::uint64_t>(osm_node_id);
+            }
+        }


The current implementation uses nested if-else conditions within the one-way segment handling, which reduces readability. As suggested by the human reviewer @afarber, this could be restructured by changing line 152 from else if (phantom_node.forward_segment_id.enabled) to handle the fwd_segment_position == 0 case as a separate top-level else-if condition. This would flatten the nesting, add clearer comments explaining the edge case, and make the code more maintainable. The suggested approach would also allow for better assertion of preconditions (e.g., that forward_geometry has at least 2 nodes when forming an edge).

Copilot · 2026-02-27T17:24:23Z

+            else if (phantom_node.fwd_segment_position == 0)
+            {
+                auto osm_node_id = facade.GetOSMNodeIDOfNode(
+                    forward_geometry(phantom_node.fwd_segment_position + 1));
+                from_node = to_node;
+                to_node = static_cast<std::uint64_t>(osm_node_id);
+            }


This fix addresses a critical bug where the nearest API could return a node with id 0 when snapping to the beginning of a one-way segment. However, there doesn't appear to be a test case added to verify this fix and prevent regression. Consider adding a feature test in features/nearest/ that specifically tests the scenario described in issue #7047: a location that snaps to the beginning of a one-way road segment (fwd_segment_position == 0), ensuring it returns valid non-zero node IDs in the correct order.

@afarber

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>

@afarber

* fix: Nearest API returning node ID 0 at start of one-way road (#7047) 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> * fix: validate nodes column format in nearest step definitions 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> --------- Co-authored-by: Muteeb Ali <31405887+muteebali@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

DennisOSRM · 2026-03-24T13:05:25Z

Thanks again for providing this patch. I grandfathered the change into #7427. Credits are giving on the commit and PR.

Project-OSRM#7047: Nearest api returning node with id 0

573f110

muteebali mentioned this pull request Oct 4, 2024

Nearest api returning node with id 0 #7047

Closed

Update CHANGELOG.md

ec990e1

CodyKlingler mentioned this pull request Mar 20, 2025

nearest service returns OSM node ids [0, 26734302] for 13.370863,52.519569 #7125

Closed

github-actions bot added the Stale label Apr 7, 2025

github-actions bot removed the Stale label Apr 8, 2025

afarber suggested changes Aug 29, 2025

View reviewed changes

github-actions bot added the Stale label Feb 26, 2026

DennisOSRM requested a review from Copilot February 27, 2026 17:16

Copilot started reviewing on behalf of DennisOSRM February 27, 2026 17:19 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

github-actions bot removed the Stale label Feb 28, 2026

DennisOSRM mentioned this pull request Mar 24, 2026

fix: Nearest API returning node ID 0 at start of one-way road #7427

Merged

DennisOSRM closed this Mar 24, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

#7047: Nearest api returning node with id 0#7048

#7047: Nearest api returning node with id 0#7048
muteebali wants to merge 2 commits intoProject-OSRM:masterfrom
muteebali:bug/7047-Nearest-api-retruning-node-with-id-0

muteebali commented Oct 4, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2025

Uh oh!

moboge commented Apr 7, 2025

Uh oh!

DennisOSRM commented Apr 7, 2025

Uh oh!

DennisOSRM commented Apr 16, 2025

Uh oh!

afarber Aug 29, 2025

Uh oh!

github-actions bot commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

DennisOSRM commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

muteebali commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Uh oh!

github-actions bot commented Apr 7, 2025

Uh oh!

moboge commented Apr 7, 2025

Uh oh!

DennisOSRM commented Apr 7, 2025

Uh oh!

DennisOSRM commented Apr 16, 2025

Uh oh!

afarber Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

DennisOSRM commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

muteebali commented Oct 4, 2024 •

edited

Loading