Skip to content

fix: validate negative distance in pgr_drivingDistance #3079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sakirr05 wants to merge 2 commits into from
Closed

fix: validate negative distance in pgr_drivingDistance #3079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23c4976
fix: validate negative distance in pgr_drivingDistance
Feb 25, 2026
f4a89ef
fix: gate negative_distance pgTap test on min_lib_version 5.0.0
Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions pgtap/dijkstra/driving_distance/edge_cases/negative_distance.pg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* :file: This file is part of the pgRouting project.
:copyright: Copyright (c) 2025-2026 pgRouting developers
:license: Creative Commons Attribution-Share Alike 3.0 https://creativecommons.org/licenses/by-sa/3.0 */

BEGIN;

UPDATE edges SET cost = sign(cost), reverse_cost = sign(reverse_cost);
SELECT CASE WHEN min_version('3.6.0') AND min_lib_version('5.0.0') THEN plan(3) ELSE plan(1) END;

CREATE OR REPLACE FUNCTION test_function()
RETURNS SETOF TEXT AS
$BODY$
BEGIN
IF min_version('3.6.0') AND min_lib_version('5.0.0') THEN
-- Test negative distance throws error
SELECT throws_ok(
$$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, -1.0)$$,
'XX000',
'Invalid value of ''distance''',
'SHOULD THROW because distance is negative'
);

-- Test zero distance throws error
SELECT throws_ok(
$$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, 0.0)$$,
'XX000',
'Invalid value of ''distance''',
'SHOULD THROW because distance is zero'
);

-- Test positive distance works (regression test)
SELECT lives_ok(
$$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, 1.0)$$,
'SHOULD NOT THROW because distance is positive'
);
Comment on lines +16 to +35
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the actual file being reviewed
find . -name "negative_distance.pg" -type f

# Search for patterns in existing pgTAP test files
echo "=== RETURN QUERY SELECT throws_ok pattern ==="
rg -n "RETURN QUERY SELECT throws_ok" --glob "*.pg" pgtap/ | head -20

echo "=== Plain SELECT throws_ok pattern ==="
rg -n "^\s+SELECT throws_ok" --glob "*.pg" pgtap/ | head -20

echo "=== RETURN QUERY SELECT lives_ok pattern ==="
rg -n "RETURN QUERY SELECT lives_ok" --glob "*.pg" pgtap/ | head -20

echo "=== Plain SELECT lives_ok pattern ==="
rg -n "^\s+SELECT lives_ok" --glob "*.pg" pgtap/ | head -20

echo "=== RETURN QUERY SELECT skip pattern ==="
rg -n "RETURN QUERY SELECT skip" --glob "*.pg" pgtap/ | head -10

Repository: pgRouting/pgrouting

Length of output: 7135

🏁 Script executed:

#!/bin/bash
# Check the structure of a few existing pgTAP test files to see the pattern
echo "=== Sample pgTAP test file structure ==="
find pgtap/ -name "*.pg" -type f | head -3 | while read f; do
  echo "File: $f"
  # Show lines with SELECT throws_ok, lives_ok, or skip
  rg -n "(SELECT|RETURN QUERY SELECT).*(throws_ok|lives_ok|skip)" "$f" | head -5
  echo "---"
done

Repository: pgRouting/pgrouting

Length of output: 396

🏁 Script executed:

#!/bin/bash
# Check the specific file being reviewed if it exists
if [ -f "pgtap/dijkstra/driving_distance/edge_cases/negative_distance.pg" ]; then
  echo "=== File exists, checking full context ==="
  cat -n "pgtap/dijkstra/driving_distance/edge_cases/negative_distance.pg"
else
  echo "File not found at expected path"
  find . -name "negative_distance.pg" -type f 2>/dev/null
fi

Repository: pgRouting/pgrouting

Length of output: 1950

Add RETURN QUERY before each assertion — test function returns zero rows without it.

In a plpgsql function returning SETOF TEXT, bare SELECT statements execute the query but discard the results. The throws_ok, lives_ok, and skip return values are never passed to the caller, so SELECT * FROM test_function() returns zero rows instead of three. The plan(3) call will fail with a mismatch between expected and actual assertion count.

Fix: add RETURN QUERY before each assertion 
     IF min_version('3.6.0') THEN
         -- Test negative distance throws error
-        SELECT throws_ok(
+        RETURN QUERY SELECT throws_ok(
             $$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, -1.0)$$,
             'XX000',
             'Invalid value of ''distance''',
             'SHOULD THROW because distance is negative'
         );

         -- Test zero distance throws error  
-        SELECT throws_ok(
+        RETURN QUERY SELECT throws_ok(
             $$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, 0.0)$$,
             'XX000',
             'Invalid value of ''distance''',
             'SHOULD THROW because distance is zero'
         );

         -- Test positive distance works (regression test)
-        SELECT lives_ok(
+        RETURN QUERY SELECT lives_ok(
             $$SELECT * FROM pgr_drivingDistance('SELECT id, source, target, cost FROM edges', 1, 1.0)$$,
             'SHOULD NOT THROW because distance is positive'
         );
     ELSE
-        SELECT skip(1, 'Function standardized on 3.6.0');
+        RETURN QUERY SELECT skip(1, 'Function standardized on 3.6.0');
     END IF;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pgtap/dijkstra/driving_distance/edge_cases/negative_distance.pg` around lines
16 - 35, The test PL/pgSQL function returns SETOF TEXT but currently uses bare
SELECT calls (throws_ok, lives_ok) so their results are discarded; prefix each
assertion call with RETURN QUERY so the results of throws_ok(...) and
lives_ok(...) are emitted to the caller (e.g., change calls to "RETURN QUERY
SELECT throws_ok(...)" / "RETURN QUERY SELECT lives_ok(...)" within the test
function) ensuring the function yields three rows to match plan(3) and that
pgr_drivingDistance negative/zero/positive checks are actually reported.

ELSE
SELECT skip(1, 'Distance validation standardized on pgRouting 5.0.0');
END IF;
END;
$BODY$
LANGUAGE plpgsql;

SELECT * FROM test_function();
SELECT * FROM finish();
ROLLBACK;
8 changes: 4 additions & 4 deletions src/spanningTree/spanningTree_process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ void pgr_process_spanningTree(

switch (val) {
case 1:
if (distance < 0) {
pgr_throw_error("Negative value found on 'distance'", "Must be non negative");
if (distance <= 0) {
pgr_throw_error("Invalid value of 'distance'", "Valid values are greater than 0");
}
Comment on lines +82 to 84
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for pgtap tests related to spanningTree
find . -path "*/pgtap/*" -name "*spanningTree*" -type f

Repository: pgRouting/pgrouting

Length of output: 45

🏁 Script executed:

# Search for any references to the distance error messages
rg -n "Negative value found on 'distance'|Invalid value of 'distance'" --type-list

Repository: pgRouting/pgrouting

Length of output: 452

🏁 Script executed:

# Search for spanningTree test files more broadly
find . -path "*/pgtap/*" -type f | grep -i "span"

Repository: pgRouting/pgrouting

Length of output: 1942

🏁 Script executed:

# Search for the old and new distance error messages in the entire repo
rg "Negative value found on 'distance'|Invalid value of 'distance'" -n

Repository: pgRouting/pgrouting

Length of output: 496

🏁 Script executed:

# Also check the source file itself to see context
cat -n src/spanningTree/spanningTree_process.cpp | sed -n '75,95p'

Repository: pgRouting/pgrouting

Length of output: 896

🏁 Script executed:

# Search for any distance validation error messages
rg "distance" pgtap/spanningTree/ -n | grep -i "error\|throw\|exception" | head -20

Repository: pgRouting/pgrouting

Length of output: 412

🏁 Script executed:

# Read the actual test assertions for the negative distance tests
cat -n pgtap/spanningTree/prim/primDD/edge_cases.pg | sed -n '110,140p'

Repository: pgRouting/pgrouting

Length of output: 1004

🏁 Script executed:

# Also check kruskalDD for comparison
cat -n pgtap/spanningTree/kruskal/kruskalDD/edge_cases.pg | sed -n '110,140p'

Repository: pgRouting/pgrouting

Length of output: 1022

🏁 Script executed:

# Check the full source file around line 106 to understand both distance checks
cat -n src/spanningTree/spanningTree_process.cpp | sed -n '100,115p'

Repository: pgRouting/pgrouting

Length of output: 553

Update pgtap tests to expect the new distance error message.

Tests in pgtap/spanningTree/prim/primDD/edge_cases.pg (lines 116, 131) and pgtap/spanningTree/kruskal/kruskalDD/edge_cases.pg (lines 116, 131) currently assert 'Negative value found on ''distance''', but the code at line 83 now throws 'Invalid value of 'distance''. Update these test assertions to match the new error message, or the tests will fail.

🧰 Tools
🪛 Cppcheck (2.19.0)

[style] 83-83: The function 'pgr_free' is never used.

(unusedFunction)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/spanningTree/spanningTree_process.cpp` around lines 82 - 84, Update the
pgtap assertions that expect the old message "Negative value found on
''distance''" to instead expect the new error text "Invalid value of
'distance'"; locate where the tests assert that string in the spanningTree prim
and kruskal edge_cases pgtap tests and change the expected message so it matches
the pgr_throw_error call that now throws "Invalid value of 'distance'" when
distance <= 0.

✅ Addressed in commit 8f1b951

break;
case 2:
Expand All @@ -102,8 +102,8 @@ void pgr_process_spanningTree(
break;

case DIJKSTRADD:
if (distance < 0) {
pgr_throw_error("Negative value found on 'distance'", "Must be positive");
if (distance <= 0) {
pgr_throw_error("Invalid value of 'distance'", "Valid values are greater than 0");
}
break;

Expand Down
Loading