Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the library to version 2.2.0, introducing the SdmxEndpoint to query the Data Commons SDMX 3.0 REST APIs, and adding support for custom HTTP headers and optional instance validation skipping. The review feedback highlights two key issues: first, the api_key is not passed to resolve_instance_url when custom headers are absent, which can cause authenticated instance validation to fail; second, the constraint builder checks for Sequence but misses set types, leading to incorrect stringification of set-based constraints.
| else: | ||
| self.base_url = resolve_instance_url(dc_instance) |
There was a problem hiding this comment.
When headers is None but api_key is provided, the api_key is not passed to resolve_instance_url. This causes the custom instance validation to run without authentication, which will fail if the custom instance requires an API key. We should pass api_key to resolve_instance_url in the else block as well.
| else: | |
| self.base_url = resolve_instance_url(dc_instance) | |
| else: | |
| self.base_url = resolve_instance_url(dc_instance, api_key=api_key) |
There was a problem hiding this comment.
Fixed. Passed api_key=api_key to resolve_instance_url in the else branch as well.
| if isinstance(value, str): | ||
| values = [value] | ||
| elif isinstance(value, Sequence): | ||
| values = list(value) |
There was a problem hiding this comment.
Using isinstance(value, Sequence) to check for collection types will not match set (since set is not a subclass of Sequence in collections.abc). If a user passes a set of constraint values (e.g., {"country/USA", "country/CAN"}), it will fall back to the else block and be stringified as a single string "{'country/USA', 'country/CAN'}" instead of being joined by commas. We should explicitly support set alongside Sequence.
| if isinstance(value, str): | |
| values = [value] | |
| elif isinstance(value, Sequence): | |
| values = list(value) | |
| if isinstance(value, str): | |
| values = [value] | |
| elif isinstance(value, (Sequence, set)): | |
| values = list(value) |
There was a problem hiding this comment.
Fixed. Added set support alongside Sequence in build_query_params.
Problem
The Data Commons API now serves SDMX 3.0
dataandavailabilityendpoints, butdatacommons-clientonly covers the V2 endpoints (node,observation,resolve). We also need a way to pass custom request headers (such asAuthorizationbearer tokens) when connecting to private Data Commons instances.Solution
SdmxEndpoint(client.sdmx): Addsfetch_dataandfetch_availabilitymethods to query/sdmx/v3/data/dataflow/DC/DF_OBS/1.0.0/*and/sdmx/v3/availability/dataflow/DC/DF_OBS/1.0.0/<component_id>, handling both/sdmx/v3(public Data Commons) and/core/api/sdmx/v3(custom DCP instances).headersandvalidate_instanceparameters toDataCommonsClientandAPIso callers can pass pre-resolved URLs and custom auth headers directly.SdmxClientErrorandSdmxAPIError(inheriting fromAPIError/DataCommonsError), along withparse_filtersandbuild_query_paramshelpers.datacommons_client/README.md.Verification
pytest datacommons_client/tests/)../run_test.sh -l).fetch_dataandfetch_availabilityqueries againsthttps://api.datacommons.org.nodeand SDMXfetch_data/fetch_availabilityqueries againsttestbed-1(datcom-dcp) using IAM OIDC bearer headers.