Skip to content

Fix: URI->new no longer dies on malformed geo: URIs - #202

Open
oalders wants to merge 1 commit into
masterfrom
fix-201
Open

oalders wants to merge 1 commit into
masterfrom
fix-201

Conversation

@oalders

@oalders oalders commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Closes #201

Changes

  • URI::geo::_init probes the latitude under do { local $@; eval { ... } } and only does the pole step when the path parses. A malformed geo: string now builds a URI::geo object, like every other scheme, and the caller's $@ is left alone.
  • The accessors (latitude, longitude, location, field, ...) still croak Badly formed geo uri, so that check now happens when a value is read, not in the constructor.
  • POD DIAGNOSTICS: removed the duplicated Badly formed geo uri item and said where it is raised.
  • POD notes that up to and including 5.37 URI->new died on such input, and that 5.38 changes this.
  • Changes entry under {{$NEXT}}.

Behaviour change: t/geo_basic.t used to assert that URI->new("geo:1") dies. That came with the original port of Andy Armstrong's standalone URI::geo (9d9b36c, #141). Inside URI it is the outlier: URI::urn::_init falls back on a bad NID rather than croaking. That test is replaced with the new behaviour. Code that used eval { URI->new(...) } to validate geo: strings should call an accessor instead.

Testing

  • New regression test in t/geo_basic.t: URI->new("geo:1") returns a URI::geo, leaves a sentinel $@ alone, and ->latitude croaks. It fails on the old geo.pm (dies at line 52) and passes with the fix.
  • prove -lr t: 63 files, 1038 tests, PASS. precious lint is clean.
  • /code-review-intense-flow (general, security, test-value): clean after one fix round (local $@) and again after the POD version note. Local Codex review: clean.

Declined findings

  • Extra tests for geo:90,x, geo:a,b, and the location accessor: these take the same _parse failure path as geo:1.
  • Test for geo:91,0 croaking Latitude out of range: the premise is false. _parse does no range check, so latitude returns 91 (checked).
  • An is_valid helper and a fuzz test: nothing in the issue or in any caller needs them (YAGNI).
  • An extra POD sentence about validation: the DIAGNOSTICS entry already says URI->new does not raise it.

🤖 Generated with Claude Code · Opus 5.5

@oalders
oalders marked this pull request as ready for review October 16, 2026 03:01
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.14%. Comparing base (1f9410f) to head (e5edf89).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   87.13%   87.14%   +0.01%     
==========================================
  Files          64       64              
  Lines        2099     2101       +2     
  Branches      512      512              
==========================================
+ Hits         1829     1831       +2     
  Misses        110      110              
  Partials      160      160              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

URI::geo::_init parsed the path to normalise longitude at the poles,
so any unparseable geo: string made the constructor croak. Skip the
normalisation when the path doesn't parse, without touching the
caller's $@, and leave validation to the accessors, matching every
other scheme. The POD notes that up to and including 5.37 URI->new
died on such input.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@esabol

esabol commented Oct 16, 2026 •

Copy link
Copy Markdown

Why would you not want malformed URIs to die in new()?

@oalders

oalders commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@esabol no other package in this URI distribution behaves this way, so it's an outlier. This change should bring it in line with the other schemes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

URI->new dies on malformed geo: URIs

2 participants