Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables additional filtering capabilities for geospatial search endpoints by adding an optional filter_json parameter that accepts MongoDB-style queries to refine search results beyond geographic constraints.
- Added
filter_jsonparameter to both/bertron/geo/nearbyand/bertron/geo/bboxendpoints - Implemented JSON parsing and filter combination logic using MongoDB's
$andoperator - Added comprehensive test coverage for various filter scenarios including validation, complex queries, and property-based filtering
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/server.py | Added filter_json parameter support and JSON parsing logic to geospatial endpoints |
| tests/test_api.py | Added comprehensive test cases for filtered geospatial searches and edge cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Should find at least the NMDC entity with depth property | ||
| found_nmdc = False | ||
| for entity in entities_data["documents"]: | ||
| properties = [ prop["attribute"]["label"] for prop in entity.get("properties", []) ] |
There was a problem hiding this comment.
[nitpick] Missing space after opening bracket in list comprehension. Should be [prop["attribute"]["label"] for prop in entity.get("properties", [])] for consistency with Python style guidelines.
| properties = [ prop["attribute"]["label"] for prop in entity.get("properties", []) ] | |
| properties = [prop["attribute"]["label"] for prop in entity.get("properties", [])] |
eecavanna
left a comment
eecavanna
left a comment
There was a problem hiding this comment.
Looks good! Thanks for implementing this and including that variety of tests. I left a few comments. No deal-breakers, in my opinion.
| # All returned entities should be from EMSL | ||
| for entity in entities_data["documents"]: | ||
| assert entity["ber_data_source"] == "EMSL" | ||
| self._verify_entity_structure(entity) | ||
|
|
||
| # Should find at least one entity | ||
| assert entities_data["count"] > 0 |
There was a problem hiding this comment.
Nitpick (optional): Swap the order of these blocks (assert there are items, then check the items).
| # Should find entities regardless of data source (empty filter = no additional restrictions) | ||
| for entity in entities_data["documents"]: | ||
| self._verify_entity_structure(entity) |
There was a problem hiding this comment.
Since the comment mentions the data source, I suggest adding an assertion that multiple data sources are represented in the response. The test can get a list of data sources directly from the database beforehand, for comparison.
| for entity in entities_data["documents"]: | ||
| self._verify_entity_structure(entity) | ||
|
|
||
| def test_geo_search_filter_with_complex_query(self, test_client: TestClient, seeded_db: Database): |
There was a problem hiding this comment.
I like this illustrative test.
| if "depth" in properties and entity["id"] == "nmdc:bsm-11-bsf8yq62": | ||
| found_nmdc = True | ||
| self._verify_entity_structure(entity) | ||
|
|
There was a problem hiding this comment.
Add an assertion that the value of the retrieved property is 24 (like in the specified filter).
2 participants
Add filters to the geospatial searches to allow for searching based on properties