Conversation
There was a problem hiding this comment.
Pull request overview
Syncs the API models and responses closer to Beacon v2 schemas, adds catalog-driven configuration endpoints, and introduces tooling to detect schema/model drift.
Changes:
- Updated Pydantic models (responses + entities) with aliases/typed nested structures and added common Beacon types.
- Standardized response building via new
response_utilshelpers and added/configuration,/entry_types,/mapendpoints. - Added schema sync/bundling and drift comparison scripts, plus documentation.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/beacon_api/models/response.py | Aligns response models with Beacon v2 (aliases, handovers, collections result shape). |
| src/beacon_api/models/entities.py | Replaces many dict fields with structured types and adds JSON aliases. |
| src/beacon_api/models/common_types.py | Introduces shared nested types (OntologyTerm, Disease, Procedure, etc.). |
| src/beacon_api/models/common.py | Updates SchemaReference shape to match new catalog/schema usage. |
| src/beacon_api/main.py | Registers new routers for configuration/map/entry types. |
| src/beacon_api/core/beacon_catalog.py | Defines entry-type catalog and builds configuration/map payloads. |
| src/beacon_api/api/response_utils.py | Centralizes meta/summary/resultset/collections response construction. |
| src/beacon_api/api/*.py | Refactors endpoints to use response_utils and updated response shapes. |
| scripts/sync_beacon_schemas.sh | Downloads/bundles upstream schemas and runs drift comparison. |
| scripts/bundle_schemas.js | Bundles/dereferences Beacon schemas into tmp/bundled_schemas. |
| scripts/compare_models.py | Compares bundled schemas vs Pydantic model fields for drift reporting. |
| pyproject.toml | Adds a dev dependency related to model generation tooling. |
| README.md | Documents schema synchronization workflow and scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from beacon_api.core.config import Settings | ||
| from beacon_api.models.common import SchemaReference | ||
|
|
||
| ENTRY_TYPES: dict[str, dict[str, str]] = { |
There was a problem hiding this comment.
ENTRY_TYPES is annotated as dict[str, dict[str, str]], but single_path is set to None for multiple entries. This will fail static type checking (and can cause downstream assumptions about string operations). Update the annotation to allow None (e.g., str | None) or use a TypedDict/dataclass that correctly models optional keys.
| ENTRY_TYPES: dict[str, dict[str, str]] = { | |
| ENTRY_TYPES: dict[str, dict[str, str | None]] = { |
| "analysis": { | ||
| "name": "Analysis", | ||
| "schema": "tmp/bundled_schemas/analysis.json", | ||
| "root_path": "/api/analyses", | ||
| "single_path": None, | ||
| }, |
There was a problem hiding this comment.
ENTRY_TYPES is annotated as dict[str, dict[str, str]], but single_path is set to None for multiple entries. This will fail static type checking (and can cause downstream assumptions about string operations). Update the annotation to allow None (e.g., str | None) or use a TypedDict/dataclass that correctly models optional keys.
src/beacon_api/models/response.py
Outdated
| info: dict[str, Any] | None = Field( | ||
| default=None, | ||
| default_factory=dict, | ||
| description="Additional information", | ||
| ) | ||
| beacon_handovers: list[BeaconHandover] | None = Field( | ||
| default_factory=list, | ||
| alias="beaconHandovers", | ||
| description="Handovers for accessing data externally", | ||
| ) |
There was a problem hiding this comment.
info and beacon_handovers are annotated as optional (| None) but use default_factory, which means they will never be None in practice. This makes the generated OpenAPI/schema misleading and creates ambiguity for consumers. Either (a) make them non-optional (dict[str, Any] / list[BeaconHandover]) with default_factory, or (b) keep them optional and use default=None (and optionally exclude empties at serialization time).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback can you implement a
| default_factory=list, | ||
| alias="beaconHandovers", | ||
| description="Handovers for accessing data externally", | ||
| ) |
There was a problem hiding this comment.
BeaconCollectionsResponse includes meta, responseSummary, response, info, and beaconHandovers, but appears to omit beaconError (present in the other response models in this file). If this model is used for FastAPI response validation, errors won’t be representable/serializable in a schema-consistent way. Consider adding beacon_error: BeaconError | None = Field(default=None, alias=\"beaconError\", ...) for consistency with BeaconResultsetsResponse / BeaconCountResponse / BeaconBooleanResponse.
| ) | |
| ) | |
| beacon_error: BeaconError | None = Field( | |
| default=None, | |
| alias="beaconError", | |
| description="Error information if query failed", | |
| ) |
scripts/compare_models.py
Outdated
| def normalize_field_name(name: str) -> str: | ||
| """Normalize field name for comparison (camelCase to snake_case).""" | ||
| # Simple camelCase to snake_case conversion | ||
| result = [] | ||
| for i, char in enumerate(name): | ||
| if char.isupper() and i > 0: | ||
| result.append("_") | ||
| result.append(char.lower()) | ||
| return "".join(result) |
There was a problem hiding this comment.
The camelCase→snake_case conversion is very naive and will produce odd results for acronyms (e.g., APIResponse → a_p_i_response) and some mixed cases, which can lead to false-positive drift results. Consider switching to a more robust regex-based conversion (handling consecutive capitals) to reduce noisy reports.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: iimpulse <7397665+iimpulse@users.noreply.github.com>
Co-authored-by: iimpulse <7397665+iimpulse@users.noreply.github.com>
Co-authored-by: iimpulse <7397665+iimpulse@users.noreply.github.com>
Co-authored-by: iimpulse <7397665+iimpulse@users.noreply.github.com>
Fix acronym handling in camelCase→snake_case conversion
Remove misleading optional annotations from BeaconBooleanResponse fields
No description provided.