Skip to content

fix: align API clients with the current v3 API and add CI tests - #13

Merged
digizeph merged 3 commits into
mainfrom
fix/broker-api-response-handling
Oct 7, 2026
Merged

digizeph merged 3 commits into
mainfrom
fix/broker-api-response-handling

Conversation

@digizeph

@digizeph digizeph commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Updates the API clients that no longer match the live v3 responses and adds CI coverage:

  • Broker: latest() raised TypeError: BrokerItem.__init__() got an unexpected keyword argument 'delay'. Latest items carry a per-item delay (seconds since the latest catalog update), now modeled on BrokerItem. peers() and collectors() parsed outdated field shapes, and _paginate could loop forever on endpoints that ignore pagination parameters (it now pages until the reported total and also stops when a page adds no new items).
  • AsnLookup: v3 items are nested (as2org, hegemony, peeringdb, population); AsnInfo models them with org_name/org_id lifted from as2org, and Pagination matches the API object (total_pages, total_count, has_next, has_prev).
  • CommunityLookup: /communities is per-ASN with a nested community list; query() returns one CommunityEntry per community, and sources() reads the sources key instead of silently returning an empty list.
  • Roas: targets the v3 ROAS endpoint, paginates to the reported total, and uses the endpoint's 0-indexed page parameter.
  • Shared plumbing (bgpkit/_utils.py): from_item (dataclass parsing that ignores unknown fields), request_json (HTTP error surfacing with a configurable timeout), and BGPKITApiError. Requests now fail loudly instead of returning empty lists.
  • Broker base URL: BGPKIT_BROKER_URL is honored (the same variable the Rust broker client uses); an explicit api_url argument still wins.

Unit tests cover recorded response shapes for every client and a new Tests workflow runs them on push and pull request. The suite is green (17 tests), and all clients were also exercised against the live API.

latest() failed to parse because latest items carry a delay field; peers() and collectors() used outdated field shapes and could loop forever on endpoints that ignore pagination parameters; request errors now surface as BrokerApiError. Adds unit tests over recorded response shapes and BGPKIT_BROKER_URL support for the base URL.
…unit-test CI

AsnLookup parsed a flat item shape; v3 items are nested (as2org, hegemony, peeringdb, population). AsnInfo now models the nested fields and Pagination matches the API object. CommunityLookup described items that do not exist: /communities is per-ASN with a nested community list, so query() returns one entry per community and sources() reads the sources key. Roas now targets the v3 endpoint, paginates to the reported total, and uses the endpoint's 0-indexed page parameter. Shared parsing and HTTP error handling move to bgpkit/_utils (BGPKITApiError, from_item, request_json); the Broker client uses the same helpers. Adds unit tests for every client and a workflow that runs them on push and pull request.
@digizeph digizeph changed the title fix: update Broker client to the current v3 API responses fix: align API clients with the current v3 API and add CI tests Oct 7, 2026
@digizeph
digizeph requested a balanced review from Copilot October 7, 2026 14:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The ASN client skips page zero by default, and transport failures bypass the new public API exception.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Aligns API clients with current v3 response formats and adds automated unit-test coverage.

Changes:

  • Updates Broker, ASN, Community, and ROAS response parsing and pagination.
  • Adds shared HTTP/error-handling utilities.
  • Adds recorded-response unit tests and a CI workflow.
File Description
.github/​workflows/​test.yml Runs unit tests on Python 3.9 and 3.12.
bgpkit/​_utils.py Adds shared parsing and request helpers.
bgpkit/​__init__.py Exports BGPKITApiError.
bgpkit/​bgpkit_asn.py Parses nested v3 ASN responses.
bgpkit/​bgpkit_broker.py Updates Broker models, pagination, and configuration.
bgpkit/​bgpkit_community.py Flattens nested communities and parses sources.
bgpkit/​bgpkit_roas.py Migrates ROAS queries to the v3 endpoint.
tests/​test_asn_unit.py Tests v3 ASN parsing and errors.
tests/​test_broker_unit.py Tests Broker response shapes and pagination.
tests/​test_community_unit.py Tests community flattening and sources.
tests/​test_integration.py Expands live API integration coverage.
tests/​test_roas_unit.py Tests ROAS pagination and errors.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread bgpkit/bgpkit_asn.py Outdated
Comment thread bgpkit/_utils.py Outdated
Default AsnLookup to page 0: the v3 ASN endpoint paginates from zero (verified live: page=0 returns the first page, page=1 the second), so the previous default skipped the first page of any broad query. request_json now wraps requests transport failures (timeouts, connection errors) in BGPKITApiError so all request failures surface through the exported exception. Adds assertions for both.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The Broker full-feed property misclassifies peers exactly at either documented threshold.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Correct inclusive full-feed threshold classification

bgpkit/​bgpkit_broker.py:46

The full-feed thresholds are inclusive in BGPKIT's peer model (>= 700_000 IPv4 or >= 100_000 IPv6), so peers exactly at either threshold are currently misclassified as partial-feed. This can also contradict the result of peers(full_feed=True) for those boundary values.

@digizeph

digizeph commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

On the full-feed boundary note for bgpkit_broker.py:46: keeping the comparison strict is intentional — the property mirrors the endpoint's own full-feed filter, which applies num_v4_pfxs > 700_000 || num_v6_pfxs > 100_000 for full_feed=true (the parameter documentation uses the same strict wording). An inclusive property would disagree with the server-side classification for peers exactly at a threshold. No peer in the current dataset sits exactly at either threshold (3,406 rows checked; nearest v6 counts 99,833 / 100,484), so this is a definitional edge with no behavioral impact today. If the API definition should become inclusive, the property can flip in the same change.

@digizeph
digizeph merged commit b670cdb into main Oct 7, 2026
3 checks passed
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.

2 participants