fix(ei): return the named collection EI methods promise (#107) - #139
Merged
Merged
Conversation
Every Energy Intelligence method typed `List[Dict[str, Any]]` returned
`response["data"]`, but the EI controllers put their records under a *named*
key inside `data`. `client.ei.rig_counts.by_basin()` returned
`{"report_date": ..., "basins": [...]}` where the signature and the docstring
example promised the basin list, so the documented
`for basin in basins: basin["count"]` iterated dict keys.
Verified live against https://api.oilpriceapi.com on 2026-09-13 with a
Scale-tier key: 27 methods across seven EI resources are wrong the same way,
in both the sync resources and `async_resources.py`.
- `unwrap_ei_collection` / `unwrap_ei_object` / `ei_data` in
`oilpriceapi/resources/ei/_envelopes.py` are now the single place that knows
the envelope shape. The per-method
`if "data" in response: return response["data"]` is gone, and
`unwrap_well_permit_search_response` keeps its name and its error message
but delegates to the shared helper instead of carrying a second copy.
- A success body missing the named collection raises
`OilPriceAPIError(code="MALFORMED_RESPONSE")` instead of handing back the
envelope. An empty collection stays an empty list.
- `ei.well_permits.get()` / `ei.frac_focus.get()` return the record rather
than its `{"well_permit": ...}` wrapper.
- `ei.forecasts.prices()` / `.production()` are typed `Dict[str, Any]`: both
return a mapping keyed by commodity / series code, never a list.
- `ei.well_permits.latest()` / `ei.frac_focus.latest()` deliberately keep
returning the envelope so their pagination and freshness counters stay
reachable; their docstrings now say so.
- Docstring examples use the field names the API actually returns.
Tests drive the real client transport (respx over httpx), not a stubbed
resource, because the defect lives between the HTTP body and the return value.
Valid, empty, missing-collection, malformed-row, 401/403/429 and async parity
are covered, plus a source-level guard that every async EI method's return
expression matches its sync twin character for character.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
Resolves the CHANGELOG conflict with #135 (streaming, #108), the only file that conflicted. Both PRs added an Unreleased entry; #135's landed in the stale mid-file `## [Unreleased]` block that this branch removes, so the streaming bullets move into the one canonical Unreleased section at the top of the file alongside the EI entries. Both entries are preserved verbatim and `## [Unreleased]` appears exactly once, as test_release_readiness.py requires. Code merged cleanly: async_resources.py auto-merged with #138's diesel `state_code` fallback, which touches a different resource class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
Merged
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.
Fixes #107.
Did it still reproduce?
Yes, on current
origin/main(7982b0b), confirmed two ways.Live,
GET https://api.oilpriceapi.com/v1/ei/rig_counts/by_basinwith a Scale-tier key on 2026-09-13:{"data":{"report_date":"2026-08-28","basins":[{"region":"permian","region_type":"basin","count":267,"week_over_week":0,"change_direction":"flat"}, ...]},"meta":{...}}rig_counts.by_basin()is annotatedList[Dict[str, Any]]and its docstring example isfor basin in basins: basin['rig_count']. It returned the wholedataobject, so the documented loop iterated dict keys.The audit found the same class of defect in 27 methods across all seven EI resources, sync and async — not just the three named in the issue. Every envelope below was captured from production on 2026-09-13, not inferred from the controller.
Envelope map — every shipped EI method
datais the top-leveldatakey. Rig counts / oil inventories / OPEC / drilling productivity / forecasts use{data, meta}; well permits and frac focus use{status, data}.ei.rig_countsdatashapelistGET /v1/ei/rig_countsgetGET /v1/ei/rig_counts/{id}latestGET /v1/ei/rig_counts/latest{id, report_date, source, last_updated, us_total, basins, top_states, drilling_type}by_basinGET /v1/ei/rig_counts/by_basin{report_date, basins:[…]}basinslistby_stateGET /v1/ei/rig_counts/by_state{report_date, states:[…]}stateslisthistoricalGET /v1/ei/rig_counts/historical{region, start_date, end_date, records:[…]}recordslistei.drilling_productivitydatashapelist/v1/ei/drilling_productivitiesget/v1/ei/drilling_productivities/{id}latest…/latest{id, report_month, source, last_updated, total_duc, basins}summary…/summary{report_month, total_duc_wells, average_oil_productivity, average_gas_productivity, basins, headline}duc_wells…/duc_wells{report_month, total_duc, by_basin:[…], declining_basins}by_basinlistby_basin…/by_basin{basins:"all", months:[…]}—basinsis the echoed filter string,monthsis the collectionmonthslisthistorical…/historical{basin, basin_name, records:[…]}recordslisttrends…/trends{report_month, analysis_months, trends:[…], declining_duc_basins}trendslistei.forecastsdatashapelist/v1/ei/forecastsget/v1/ei/forecasts/{id}latest…/latest{id, report_date, source, last_updated, summary, forecasts}summary…/summary{report_month, forecasts, headline}prices…/prices{report_month, commodities:{brent:…, wti:…, natural_gas:…}}— a mapping, never a listList[Dict]Dict[str, Any]production…/production{report_month, series:{…}}— a mappingList[Dict]Dict[str, Any]historical…/historical{series_code, actuals:[…]}actualslistcompare…/compare{series_code, month1, month2, comparison}ei.oil_inventoriesdatashapelist/v1/ei/oil_inventoriesget…/{id}latest…/latest{id, report_date, week_ending, source, last_updated, summary, inventories}summary…/summary{week_ending, inventories, headline}by_product…/by_product{week_ending, products:[…]}productslisthistorical…/historical{product_type, location, records:[…]}recordslistcushing…/cushing{location, latest, history}ei.opec_productiondatashapelist/v1/ei/opec_productionsget…/{id}latest…/latest{id, report_month, publication_month, production_month, source, opec_total, countries, headline}total…/total{latest, history, trend}by_country…/by_country{report_month, publication_month, production_month, countries:[…], opec_total}countrieslisthistorical…/historical{country, country_name, records:[…]}recordslisttop_producers…/top_producers{report_month, …, producers:[…], opec_total}producerslistei.well_permitsdatashapelistGET /v1/ei/well-permits{well_permits:[…], meta}well_permitslistget…/{api_number}{well_permit:{…}}latest…/latest{well_permits:[…], meta}summary…/summary{period_days, total_permits, by_state, top_operators, top_formations, by_permit_type, weekly_trend, last_updated, as_of, data_age_days, stale, as_of_basis, by_state_as_of, stale_states}by_state…/by-state{well_permits:[…], state, meta}well_permitslistby_operator…/by-operator{well_permits:[…], operator_query, meta}well_permitslistby_formation…/by-formation{well_permits:[…], formation_query, meta}well_permitslistsearch…/search{well_permits:[…], meta}ei.frac_focusdatashapelistGET /v1/ei/frac-focus{frac_focus_disclosures:[…], meta}frac_focus_disclosureslistget…/{upload_key}{frac_focus_disclosure:{…}}latest…/latest{frac_focus_disclosures:[…], meta}summary…/summary{period_days, total_disclosures, by_state, top_operators, water_usage, monthly_trend, last_updated}by_state…/by-state{frac_focus_disclosures:[…], state, meta}by_operator…/by-operator{frac_focus_disclosures:[…], operator_query, meta}by_chemical…/by-chemical{frac_focus_disclosures:[…], chemical_query, meta}search…/search{frac_focus_disclosures:[…], meta}chemicals…/{upload_key}/chemicals{upload_key, api_number, well_name, operator, chemical_count, chemicals:[…], additives, cas_numbers, suppliers}chemicalslistfor_well…/for-well/{api_number}{api_number, frac_focus_disclosures:[…], count}frac_focus_disclosureslistei.well_timeline()returnsdataas an object and is unchanged; it now calls the sharedei_datahelper instead of its own copy of the unwrap line.Counts: 27 methods changed (24 collections + 2 nested records + 1 shared search helper refactor), 2 retyped with no behaviour change, 29 recorded as no-change.
Public contract
The smallest compatible change, per method:
latest()on well permits and frac focus keeps returning the envelope object. Its annotation (Dict[str, Any]) was already truthful; changing it to a list would break callers and throw away the pagination/freshness counters the issue asks us to preserve. Only the docstring changed, to name the real keys.prices()/production()return the same value as before. Only the annotation moved fromList[Dict[str, Any]]toDict[str, Any], because those endpoints return a mapping keyed by commodity / series code.Breaking for callers who adapted to the bug rather than the docs.
by_basin()["basins"],well_permits.list()["well_permits"]andwell_permits.get(id)["well_permit"]must drop that subscript. Collection methods on well permits and frac focus no longer surfacedata["meta"]— that matches every other EIlist(), which has always droppedmeta. Called out in the CHANGELOG. No version bump; this sits under## [Unreleased].No second abstraction
oilpriceapi/resources/ei/_envelopes.py(ei_data,unwrap_ei_collection,unwrap_ei_object) is now the only place that knows the EI envelope shape. Theif "data" in response: return response["data"]block repeated in 60+ EI methods is gone, andunwrap_well_permit_search_responsekeeps its public name, itsMALFORMED_RESPONSEcode and its exact error message while delegating to the shared helper — one implementation, not two. Its existing tests pass unchanged.Failures are explicit: a 200 whose body is missing the named collection, or whose collection holds a non-record, raises
OilPriceAPIError(code="MALFORMED_RESPONSE")with the raw body attached. An empty collection is an empty list, never an error; a missing collection is never a fabricated empty success.Sync and async are identical
Every changed sync method has the identical change in
oilpriceapi/async_resources.py. That is not a claim, it is pinned bytest_async_ei_unwrapping_matches_sync_line_for_line, which reads both implementations withinspect.getsourceand asserts each async method's trailing return expression matches its sync twin character for character, for all seven resources. Proven red-capable by reverting one async method toei_data(response):Behavioural parity is covered too: every collection case runs through both clients and the results are asserted equal, and the public method sets of each sync/async pair must match.
Tests
tests/unit/test_ei_envelopes.py— 200 tests driving the real client transport.respxintercepts httpx, so the request leaves the client, the response body is the live-captured envelope, and the assertion is on what the resource hands back. Nothing stubsclient.request; the issue calls this out specifically, and a resource-level mock cannot see the defect at all.respx>=0.20.2added to thedevextra.Coverage: valid collection, empty collection, missing collection key, malformed row, nested-record unwrapping, object-envelope passthrough (a regression guard against a broad global unwrap), 401 / 403 / 429, async equivalents, and both parity guards.
Red — the new test file against pre-fix
origin/mainsourcesRun in a detached worktree at
origin/mainwith only the new test file and_envelopes.pycopied in:Green — same file on this branch
Full suite
The 3 failures are identical on both sides and pre-existing:
tests/integration/test_demo_contract.pymakes live calls and gets HTTP 429 from this IP. Environmental, not code. The CI command excludes them:ruff check oilpriceapi/— clean.mypy oilpriceapi/ --ignore-missing-imports—Success: no issues found in 48 source files.CHANGELOG.mdhad two## [Unreleased]headings, the lower one empty;tests/test_release_readiness.pyasserts there is exactly one, so the empty duplicate is removed here.🤖 Generated with Claude Code
https://claude.ai/code/session_015ao5paex73xXvuM424Libo