Enable properties / filters in geospatial search #114
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).
Add filters to the geospatial searches to allow for searching based on properties