fix: reject malformed price rows instead of manufacturing zero prices (#15)

[karlwaldman]

Closes #15.

Confirmed live on origin/main (6879501)

Mock-transport probe, successful status: "success" envelopes throughout:

response	origin/main returned
data.prices = [{}] via pastDay()	{"code":"","price":0,"currency":"USD"}
price: "not-a-number"	{"code":"BRENT_CRUDE_USD","price":0,...}
data.prices = "garbage"	[] — an empty successful list
data with no prices key	[] — an empty successful list
data.prices = [{}] via demoPrices()	{"code":"","price":0,"currency":"USD"}

A $0.00 that the SDK invented is indistinguishable from a real quote. It carries a plausible code, a plausible currency and a successful status, so nothing downstream — a model, an invoice, an alert threshold — can tell it apart from a market print.

The fix

One validated boundary, Price::fromArray, now required for every row:

• code must be present, a string, and non-empty after trimming
• price must be present and is_numeric — checked with array_key_exists, never truthiness, so 0, 0.0, "0.00" and -37.63 all pass
• a timestamp that is present but unparseable raises instead of silently becoming null

Client::priceOrFail() routes latest(), the four historical period methods and demoPrices() through it with path context in the message. Client::priceListOrFail() separates the two cases the old code conflated: a missing or non-array prices field is a malformed envelope and raises; prices: [] is a legitimately empty result and still returns [].

Red / green

Red, proven against pre-fix code (git checkout origin/main -- src/Client.php src/Price.php, rerun, restore):

Tests: 67, Assertions: 369, Failures: 26.

1) MalformedPriceRowTest::testHistoricalRejectsMalformedRow@empty row with data ([])
Failed asserting that exception of type "OilPriceAPI\Exception\ApiException" is thrown.
4) MalformedPriceRowTest::testHistoricalRejectsMalformedRow@non-numeric price with data (['BRENT_CRUDE_USD', 'not-a-number'])
Failed asserting that exception of type "OilPriceAPI\Exception\ApiException" is thrown.

Green, with the fix:

OK (67 tests, 369 assertions)

Baseline on clean origin/main before any change: OK (26 tests, 323 assertions). PHP 8.5.8, Composer 2.10.2, PHPUnit 13.3.3.

Coverage: ten malformed-row shapes (empty row, missing/null/non-numeric/empty-string/array price, missing/empty/non-string code, malformed timestamp) run against latest(), the historical methods and demoPrices() so all three agree; plus non-array prices, missing prices, legitimately empty prices, and five legitimate numeric prices including zero and negative.

One deliberate non-change, stated plainly

currency still defaults to 'USD' when the field is absent. I could not verify the live response schema during this session — the demo endpoint was IP rate-limited and no API key was available — and making an absent currency fatal would break every historical call if the production schema ever omits it. The dangerous fabrication is the price, and that is now fixed. Unverified: whether production ever omits currency. Worth a follow-up check against a live response before tightening it.

Compatibility

Price::fromArray() is public and now throws ApiException for rows it previously turned into zeros. That is the defect, not a regression, but it is a behaviour change for anyone calling it directly. ApiException is the documented catch-all, so existing error handling still catches it. The existing 'Unexpected latest price shape from /v1/prices/latest.' message is preserved as a prefix.

Not merged, no auto-merge, no release tagged.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4b2eaf30-d8cc-4144-a198-d5b536d72bed

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[karlwaldman]

---

Updated for main at 3feb505fa (after #17 merged)

origin/main was merged into this branch. src/Client.php merged cleanly — #17's origin guard sits in request() and its own helper block, this branch's validation sits in the price-decoding helpers, and the two do not overlap. The only conflict was the ## Unreleased block in CHANGELOG.md; both entries are kept, ### Security (origin guard) above ### Fixed (this change).

I first resolved this as a rebase, then reproduced it as a merge so the branch updates without a force-push. The resulting trees are byte-identical (5b84303), so what is pushed here is exactly the resolution I verified.

Nothing had to be dropped. Both changes survive in full.

New baseline, clean origin/main at 3feb505f: OK (50 tests, 380 assertions) — up from OK (26 tests, 323 assertions), since #17 added RawPathOriginTest.

Re-proven against the new main (git checkout origin/main -- src/Client.php src/Price.php, rerun, restore):

Tests: 91, Assertions: 426, Failures: 26.

1) MalformedPriceRowTest::testHistoricalRejectsMalformedRow@empty row with data ([])
Failed asserting that exception of type "OilPriceAPI\Exception\ApiException" is thrown.

2) MalformedPriceRowTest::testHistoricalRejectsMalformedRow@missing price with data (['BRENT_CRUDE_USD'])
Failed asserting that exception of type "OilPriceAPI\Exception\ApiException" is thrown.

All 26 failures are MalformedPriceRowTest. In that same pre-fix state RawPathOriginTest still reports OK (24 tests, 57 assertions), which confirms reverting this branch's files leaves #17's guard untouched — the red is this defect and nothing else.

Green, with the fix, on top of the new main: OK (91 tests, 426 assertions). Packaged Composer smoke passes.

The currency question stays open and unchanged, as asked.

[karlwaldman]

PHP expert review of this branch as proposed (cc74b0e). Suite runs green here: OK (91 tests, 426 assertions) on PHP 8.5.8 / PHPUnit 13.3.3. Not touching the branch — rebase in progress.

The core change is right: moving the validation into Price::fromArray() and routing latest(), the historical methods and demoPrices() through one priceOrFail() boundary is the correct shape, and treating a missing prices key as a malformed envelope rather than an empty result is the detail most implementations get wrong. Three things to settle before it merges.

1. It leaves the timestamp half of the fabrication open — filed as #20. if (!$parsed instanceof DateTimeImmutable) throw only catches parsing that fails outright. createFromFormat(ATOM, '2026-13-45T99:99:99Z') returns a valid object (2027-02-18T04:40:39+00:00) and reports the problem only via DateTimeImmutable::getLastErrors(), which is never called; and the new DateTimeImmutable($ts) fallback accepts "now", "next friday", "0000-00-00". Executed on this branch. The PR's test covers one input, 'not-a-date', which is the one case that does throw.

2. This is a breaking change and the version says otherwise. Price::fromArray() is public static on a class with no @internal, and it is documented in the README's public surface. It goes from total (always returns a Price) to partial (throws). Anyone calling it directly on a stored payload gets an uncaught RuntimeException after what currently looks like a patch. Client::VERSION is still '2.1.2' and the changelog entry sits under ### Fixed with no version heading. Under semver this is 3.0.0. If a major is unwanted right now, the alternative is to keep fromArray() total and put the strictness behind a new Price::fromApiRow() that Client alone calls. Either is defensible; shipping it as a patch is not.

The exception design itself is correct — ApiException extends RuntimeException and the three subclasses extend it, so existing catch (ApiException $e) code catches every new throw, and no message or log line carries key material (the credential lives only in the Authorization header; ApiException::$responseBody is server-supplied).

3. Two smaller things.

• priceOrFail() re-wraps ApiException into a fresh ApiException without a previous, so the original throw site vanishes from the trace. ApiException::__construct has no $previous slot at all — worth adding while the constructor is already changing.
• One bad row fails the entire call. Right for price; for updatedAt on pastYear() it discards thousands of good rows because row 4,000 has a bad date. Worth being a deliberate decision rather than inherited from the price rule.

On the currency default, deliberately left open in the issue: it is the same fabrication class, and this repo's own fixture proves it — tests/ClientTest.php:404 uses EU_CARBON_EUR, so the catalogue is not USD-only and ?? 'USD' can label a EUR carbon price as USD. That is worse than a $0.00, which is self-evidently broken; 78.40 USD on an EUA contract is plausible and silently wrong. My recommendation is to make currency required alongside code and price — this PR already breaks BC on the method, so the extra strictness costs nothing — but that is Karl's call, and the remaining (string) coercion on currency/unit/name/source/type/formatted is filed separately as #21.

Full review notes in review-php.md; repro scripts for every claim above are in the review.