Conversation
|
Please add PR description to help set context for reviewing. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive property filtering capabilities to the BERtron client, allowing users to filter entities based on their metadata properties using MongoDB-style queries. The enhancement enables filtering by property names, IDs, values, and supports various filter types including raw string matching, numeric values, regex patterns, and numeric ranges.
Key changes:
- Added
create_property_filter()method for building MongoDB$elemMatchqueries - Extended geographic and source-based query methods to accept optional property filters
- Added comprehensive example usage in the main execution block
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| elif property_type == "range": | ||
| # For range queries, we need to handle both single numeric_value | ||
| # and minimum_numeric_value/maximum_numeric_value pairs | ||
| # Use $or to match either case | ||
| elem_match_conditions["$or"] = [ | ||
| { | ||
| "numeric_value": { | ||
| "$gte": property_value[0], | ||
| "$lte": property_value[1] | ||
| } | ||
| }, | ||
| { | ||
| "minimum_numeric_value": {"$lte": property_value[1]}, | ||
| "maximum_numeric_value": {"$gte": property_value[0]} | ||
| } | ||
| ] |
There was a problem hiding this comment.
No validation that property_value is a list/tuple with exactly 2 elements before accessing indices [0] and [1]. This will raise an IndexError if a single value or wrong format is passed.
| base_filter = {"ber_data_source": source} | ||
| if filter_dict: | ||
| base_filter.update(filter_dict) |
There was a problem hiding this comment.
Using update() can overwrite the ber_data_source key if filter_dict contains the same key. This could silently change the intended behavior. Consider using a merge strategy that preserves the base filter or validates for conflicts.
| base_filter = {"entity_type": entity_type} | ||
| if filter_dict: | ||
| base_filter.update(filter_dict) |
There was a problem hiding this comment.
Same issue as with find_entities_by_source: using update() can overwrite the entity_type key if filter_dict contains the same key, potentially changing the intended filtering behavior.
| base_filter = {"entity_type": entity_type} | |
| if filter_dict: | |
| base_filter.update(filter_dict) | |
| # Ensure the entity_type argument always takes precedence over filter_dict | |
| base_filter = dict(filter_dict) if filter_dict else {} | |
| base_filter["entity_type"] = entity_type |
eecavanna
left a comment
eecavanna
left a comment
There was a problem hiding this comment.
I'm not too familiar with this codebase. I skimmed the diff on my iPhone and nothing jumped out to me as problematic, other than the things Copilot reported (except one of them, which I clicked "Resolve conversation" on).
2 participants
No description provided.