fix: shape endpoint polyline parity with Java (floor encoding, no point reduction)#1076
fix: shape endpoint polyline parity with Java (floor encoding, no point reduction)#1076Ahmedhossamdev wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the ChangesPolyline encoder replacement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/utils/polyline_test.go (1)
19-31: ⚡ Quick winAdd a negative-boundary flooring assertion to harden Java parity.
The flooring test currently covers only a positive near-boundary value. A regression to truncation semantics on negative coordinates could slip through; adding a negative near-zero case closes that gap.
Suggested test extension
func TestEncodePolyline_FloorsNotRounds(t *testing.T) { @@ if got == rounded { t.Errorf("floored encoding should differ from rounded 0.00002 encoding %q", rounded) } + + negGot := EncodePolyline([][]float64{{-0.000019, 0}}) + negFloored := EncodePolyline([][]float64{{-0.00002, 0}}) + negTruncated := EncodePolyline([][]float64{{-0.00001, 0}}) + if negGot != negFloored { + t.Errorf("floor(-0.000019*1e5) should match -0.00002 encoding; got %q want %q", negGot, negFloored) + } + if negGot == negTruncated { + t.Errorf("floored encoding should differ from truncated -0.00001 encoding %q", negTruncated) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/utils/polyline_test.go` around lines 19 - 31, The TestEncodePolyline_FloorsNotRounds test currently only verifies flooring behavior for positive near-boundary coordinates. Add additional test assertions within this function to verify that negative near-boundary values are also properly floored (not truncated or rounded) by EncodePolyline. Include test cases with negative coordinates near zero (such as -0.000019) to ensure the flooring semantics work correctly across the negative boundary, matching the Java implementation behavior and preventing regressions on negative coordinate handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/utils/polyline_test.go`:
- Around line 19-31: The TestEncodePolyline_FloorsNotRounds test currently only
verifies flooring behavior for positive near-boundary coordinates. Add
additional test assertions within this function to verify that negative
near-boundary values are also properly floored (not truncated or rounded) by
EncodePolyline. Include test cases with negative coordinates near zero (such as
-0.000019) to ensure the flooring semantics work correctly across the negative
boundary, matching the Java implementation behavior and preventing regressions
on negative coordinate handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90dc4594-16ed-42c2-a655-1fe7c0f792ef
📒 Files selected for processing (4)
internal/restapi/shapes_handler.gointernal/restapi/shapes_handler_test.gointernal/utils/polyline.gointernal/utils/polyline_test.go
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
1 participant
Fixes: #990, #1075
Description
Fixes two spec gaps in
/api/where/shape/{id}, both verified byte-for-byte against the live Java reference server (all 18 unitrans shapes now identical).1. Encoding rounded instead of flooring
The
go-polylinelibrary rounds coordinates; Java'sPolylineEncoderfloors (floor(coord * 1e5)). This produced ~1e-5° (~1 m) off-by-one errors on ~38% of points in every shape. Addedutils.EncodePolyline, a floor-based encoder mirroring the Java algorithm.2. Dropped consecutive duplicate points
The handler filtered consecutive duplicates, but the spec requires all points included with no simplification (Java's single-shape path does no filtering). Removed the filtering;
lengthandpointsnow reflect every GTFS point.Updated the shape test that asserted the old dedup behavior; added unit tests for the encoder. Full suite + lint pass.
Note:
stops-for-routeshares the same rounding divergence (polyline.EncodeCoords); it'll be migrated toutils.EncodePolylineduring that endpoint's review, after which thego-polylinedep can be dropped.Summary by CodeRabbit
Release Notes