Repository navigation
Fix issues found during the connection-reuse review - #2
Merged
Merged
Conversation
HTTP::Tiny verifies TLS certificates by default only since 0.083, and even then the default can be switched off through an environment variable, so the connection to the API went unverified on older HTTP::Tiny. Pass verify_SSL => 1 explicitly and add the constructor option verify_ssl (default 1; 0 disables the check, for example for a local test endpoint) with a matching accessor. Document that verification needs a CA bundle (Mozilla::CA or the operating system's). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Content-Type: application/json was a default header of the HTTP::Tiny object, so every GET request announced a body type it does not have. The header now travels only with the bulk POST, passed in that request's options (so a supplied ua receives it too), while GET requests send Accept alone. The User-Agent header becomes ipdata-perl/<version>, the module version being available at runtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Single-field lookups (/{ip}/{field}) answer with a bare JSON value.
JSON::PP before 4.0 (core before Perl 5.30) rejects a top-level string,
number or null unless allow_nonref is set, so the module fell back to
returning the raw text: null came back as the string "null", which is
true in Perl, and escape sequences in strings were left undecoded.
Decode with a JSON::PP object that has allow_nonref set; the fallback
remains for bodies that are not JSON at all. Tests cover false, true,
null, escaped strings, numbers and non-JSON bodies.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
t/02-validation.t sent real requests to api.ipdata.co with a placeholder key and accepted any network or HTTP error as proof that validation had passed, so "make test" needed network access although the README says the unit tests need no API key. The requests now go to a stand-in user agent injected through ua, and the tests check the URL that was built instead of the error that came back. Also drop two unused variables from t/03-mock-responses.t. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The example used "use lib '../lib'", a path relative to the current directory, so it only ran from inside examples/. Resolve the lib directory relative to the script itself with FindBin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add the two Perl releases since 5.40 to the CI matrix and set fail-fast: false, so a failure on one Perl no longer cancels the jobs still running on the others and every result is visible in one run. The release workflow now installs the declared prerequisites before building, as the CI workflow does, so the release build tests the versions Makefile.PL requires rather than whatever its Perl bundles. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The API returns threat.is_vpn and threat.scores (vpn_score, proxy_score, threat_score, trust_score), both present in the test fixtures, but the documented response shape in the POD and the README omitted them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The two variables removed from t/03-mock-responses.t had been the second mention of the HTTP::Tiny::get and HTTP::Tiny::post globs, so after their removal Perl warned 'Name "HTTP::Tiny::get" used only once: possible typo' for the assignments that install the mocks, and the CI runner surfaced the warnings as errors. Disable the 'once' warning category in that block, next to the 'redefine' one it already disables. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every CI job logs that actions/checkout@v4 targets the deprecated Node.js 20 runtime and is being forced onto Node.js 24. v5 targets Node.js 24 natively; the setup-perl action already does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jonathan-kosgei
merged commit Nov 3, 2026
df257ef
into
claude/perl-ipdata-sdk-8p48p
16 checks passed
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
Follow-up to #1, working through the items its description listed under "Noticed, not changed". Nine commits on top of
claude/perl-ipdata-sdk-8p48p: TLS certificate verification on every HTTP::Tiny version (newverify_ssloption), request headers (Content-Typeonly on the bulk POST,ipdata-perl/<version>user agent), bare JSON values decoded with every JSON::PP version, a test suite that no longer needs network access, an example that runs from any directory, a CI matrix that covers Perl 5.42 and 5.44 and keeps running after a failure, thethreatshape documented withis_vpnandscores, and two CI-driven follow-ups (aused only oncewarning in t/03,actions/checkout@v5).$VERSIONis unchanged (1.1.0); theChangesentries sit under anUnreleasedheading.Changes
verify_SSL => 1is now passed explicitly: HTTP::Tiny before 0.083 verified only when asked to, and 0.083+ can be told not to throughPERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT. New constructor optionverify_ssl(default 1;0disables the check, for example for a local test endpoint given asbase_url) and accessorverify_ssl; POD and README document the option and the CA bundle requirement (Mozilla::CA or the operating system's). Not applied to a suppliedua.Content-Type: application/jsonis sent only with the bulk POST. It was a default header of the HTTP::Tiny object and so went out with every GET too. It is now passed in the POST request's options (so a supplieduareceives it as well); GET requests sendAccept: application/jsonalone.ipdata-perl/<version>(wasNet-IPData-Perl/<version>), the module version being available at runtime.allow_nonrefis set, so the module fell back to the raw text:nullcame back as the string"null"(true in Perl) and escape sequences stayed undecoded. Booleans were not affected (JSON::PP returns them as blessed objects, which passed its check). The decoder now hasallow_nonrefset; the fallback remains for bodies that are not JSON at all.make testneeds no network access.t/02-validation.tsent requests to api.ipdata.co with a placeholder key and took any network or HTTP error as proof that validation had passed. It now uses a stand-in user agent injected throughuaand checks the URL that was built. The unused$orig_get/$orig_postint/03-mock-responses.tare gone; they had been the second mention of theHTTP::Tiny::getandHTTP::Tiny::postglobs, so the block that installs the mocks now disables theoncewarning category next toredefine(the first CI run otherwise surfacedName "HTTP::Tiny::get" used only once: possible typo).examples/basic.plruns from any directory (FindBininstead ofuse lib '../lib').fail-fast: false).release.ymlinstalls the declared prerequisites before building, asci.ymldoes. Both workflows useactions/checkout@v5: every job logged that v4 targets the deprecated Node.js 20 runtime and is being forced onto Node.js 24.threatshape listsis_vpnandscores(vpn_score,proxy_score,threat_score,trust_score) in POD and README.Not changed
keep_alive_timeoutconstructor argument: an upstream bug; the existing workaround (setting the attribute directly) stays.release.ymlstill builds on a single Perl (5.38): a release build needs one Perl, and the matrix testing happens inci.yml.Scalar::Util::blessed: already in use to validateuasince Reuse HTTPS connections, add new response fields #1; nothing left to do.closemethod and no defaultkeep_alive_timeout: design decisions recorded in Reuse HTTPS connections, add new response fields #1, not defects.Verification
prove -l t:Files=5, Tests=180, Result: PASS, nothing on stderr (t/01 44, t/02 15, t/03 61, t/05 60; t/04 skipped withoutIPDATA_API_KEY). Before this change: 156 tests.unshare -rn prove -l t(a network namespace with no interface, so no connection is possible):Result: PASS.perl Makefile.PL && make && make test: 180 tests, PASS;make distcheck: MANIFEST complete;perl -Ilib -c lib/Net/IPData.pmandpodchecker lib/Net/IPData.pm: OK.@INC. Before the decoder fix three of the new t/03 checks failed (nullreturned as"null",\uand\"escapes undecoded); after it, PASS with all three JSON::PP versions.verify_ssl: against a local HTTPS server with a self-signed certificate answering 401 like the API, the default fails withNetwork error: SSL connection failed ... certificate verify failed;verify_ssl => 0connects and reportsAPI error (401); the default with that certificate trusted throughua => HTTP::Tiny->new(SSL_options => { SSL_ca_file => ... })also reportsAPI error (401).Files=5, Tests=180, Result: PASSin every job; on 5.14 cpanm installs HTTP::Tiny 0.096 while the bundled JSON::PP 2.27105 stays, so the bare-value checks run against it there:5c6f7e5: push run https://github.com/ipdata/perl/actions/runs/41639916419 and pull_request run https://github.com/ipdata/perl/actions/runs/41640062286, success; their logs showed theused only oncewarning and the Node.js 20 notice that the last two commits fix.b2420ea(head): push run https://github.com/ipdata/perl/actions/runs/41640344006 and pull_request run https://github.com/ipdata/perl/actions/runs/41640352817, success.🤖 Generated with Claude Code