Implement API client and add Pydantic classes #56
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: eecavanna <134325062+eecavanna@users.noreply.github.com>
|
Note - this uses a local copy of the pydantic schema file |
|
|
||
| # Create 2dsphere index for geospatial queries on coordinates | ||
| self.db.entities.create_index([('coordinates', pymongo.GEOSPHERE)]) | ||
| self.db.entities.create_index([('geojson', pymongo.GEOSPHERE)]) |
There was a problem hiding this comment.
I am surprised to see indexes being created each time an entity gets inserted (as opposed to after all entities have been inserted).
Edit: Maybe these are effectively "no op"s when the index already exists.
bertron-schema repo
bertron-schema repobertron-schema repo
There was a problem hiding this comment.
Pull Request Overview
This PR integrates Pydantic models into the server and client code, refactors geospatial query handling, and adds a cleanup option to the ingestion workflow.
- Replace raw MongoDB documents with
bertron_schema_pydantic.Entityinstances in server endpoints - Centralize document-to-entity conversion and add logging for validation failures
- Enhance ingestion script with clean flag and switch to GeoJSON field
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server.py | Use Pydantic Entity, refactor geospatial endpoints, add convert_document_to_entity and logging |
| src/bertron_client.py | Update client methods to consume Entity model and reconstruct metadata |
| mongodb/ingest_data.py | Add clean_collections method, switch to geojson field, update indexes |
| mongodb/gold-example.json | Remove outdated example data |
| docker-compose.yml | Add --clean flag to ingestion command |
Comments suppressed due to low confidence (3)
src/server.py:165
- The nearby-entities endpoint no longer returns metadata (
query_type,center,radius_meters) that clients may rely on. Consider adding those fields or a genericmetadataobject to maintain API compatibility.
return {"documents": entities, "count": len(entities)}
src/server.py:238
- The bounding-box endpoint no longer includes
query_typeor the bounding-box coordinates in its response. Clients reconstructing request context may break; consider returning those values or a metadata object.
return {"documents": entities, "count": len(entities)}
src/server.py:42
- [nitpick] The new conversion logic in
get_all_entitiesand the centralizedconvert_document_to_entityare not covered by existing tests. Add unit tests for these to prevent regressions.
def get_all_entities():
|
|
||
| def convert_document_to_entity( | ||
| document: Dict[str, Any], | ||
| ) -> Optional[bertron_schema_pydantic.Entity]: |
There was a problem hiding this comment.
The return type is annotated as Optional[Entity] but the function always returns an Entity. Updating the signature to return Entity improves type accuracy.
| ) -> Optional[bertron_schema_pydantic.Entity]: | |
| ) -> bertron_schema_pydantic.Entity: |
|
Edit: This comment is obsolete. Show/hide commentI renamed the PR (its name will appear in the release notes): - Pydantic models
+ Implement API client and add local copy of Pydantic classes from `bertron-schema` repoI recommend renaming it again once the local copy of the Pydantic class definitions has been removed from this branch (assuming that happens before this branch is merged into |
|
Edit: This comment is obsolete. Show/hide commentHi @shreddd, PR ber-data/bertron-schema#41 has been merged into Here's a command you can use to pip install git+https://github.com/ber-data/bertron-schema.git |
bertron-schema repo|
Note - I updated the code to use the pip installable version instead. |
Summary of changes: