use sets for alert filters #1994
Conversation
ofek1weiss
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
|
Caution Review failedThe pull request is closed. WalkthroughReplaces procedural per-value filtering with set-based, normalized matching in elementary/monitor/data_monitoring/schema.py: removes module-level apply_filter, adds NEGATIVE_OPERATORS, cached normalized values, new FilterSchema APIs (get_matching_values, apply_filter_on_values, apply_filter_on_value), updates FiltersSchema.test_ids typing and selector field mappings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FS as FilterSchema
participant Cache as cached props
rect rgba(231,243,255,0.6)
Caller->>FS: apply_filter_on_values(values)
FS->>FS: normalize_values(values)
FS->>Cache: access _normalized_values (cached)
FS->>FS: get_matching_normalized_values(normalized_input)
alt operator IS / CONTAINS
FS-->>Caller: return True if any match
else operator IS_NOT / NOT_CONTAINS
opt input empty
Note over FS: negative operators pass empty input
end
FS-->>Caller: return True if no matches
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
👋 @ofek1weiss |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
elementary/monitor/data_monitoring/schema.py (4)
57-60: Bug: normalized_status is always empty due to incorrect membership checkstatus is a str and list(Status) yields enum members, so the membership test fails and drops all statuses. This causes FiltersSchema.apply to reject everything when a status filter exists.
Apply this diff to fix:
- def normalized_status(self) -> List[Status]: - return [Status(status) for status in self.statuses if status in list(Status)] + def normalized_status(self) -> List[Status]: + normalized: List[Status] = [] + for s in self.statuses: + try: + normalized.append(Status(s)) + except ValueError: + # ignore invalid values + continue + return normalized
48-56: Mutable default lists in Pydantic model fieldsUsing [] as a default creates a shared mutable default. We already use Field(default_factory=list) elsewhere; please align these.
Apply this diff:
-class FilterFields(BaseModel): - tags: List[str] = [] - models: List[str] = [] - owners: List[str] = [] - statuses: List[str] = [] - resource_types: List[ResourceType] = [] - node_names: List[str] = [] - test_ids: List[str] = [] +class FilterFields(BaseModel): + tags: List[str] = Field(default_factory=list) + models: List[str] = Field(default_factory=list) + owners: List[str] = Field(default_factory=list) + statuses: List[str] = Field(default_factory=list) + resource_types: List[ResourceType] = Field(default_factory=list) + node_names: List[str] = Field(default_factory=list) + test_ids: List[str] = Field(default_factory=list)
155-157: Avoid shared default list for statusesField(default=_get_default_statuses_filter()) evaluates at import time and shares a list across instances.
Apply this diff:
- statuses: List[StatusFilterSchema] = Field(default=_get_default_statuses_filter()) + statuses: List[StatusFilterSchema] = Field(default_factory=_get_default_statuses_filter)
344-359: Mutable default for SelectorFilterSchema.statusesSame mutable-default issue here; use Field(default_factory=...) to avoid shared state.
Apply this diff:
class SelectorFilterSchema(BaseModel): @@ - statuses: Optional[List[Status]] = [ - Status.FAIL, - Status.ERROR, - Status.RUNTIME_ERROR, - Status.WARN, - ] + statuses: Optional[List[Status]] = Field( + default_factory=lambda: [ + Status.FAIL, + Status.ERROR, + Status.RUNTIME_ERROR, + Status.WARN, + ] + )
🧹 Nitpick comments (8)
elementary/monitor/data_monitoring/schema.py (8)
67-67: Avoid duplicating operator groupsNEGATIVE_OPERATORS duplicates ALL_OPERATORS. Reuse the existing constant to keep semantics in one place.
Apply this diff:
-NEGATIVE_OPERATORS = [FilterType.IS_NOT, FilterType.NOT_CONTAINS] +NEGATIVE_OPERATORS = ALL_OPERATORS
79-86: cached_property can get stale if FilterSchema.values mutates at runtimeBoth caches depend on self.values. If the model is mutated post-init, caches won’t refresh. If these models are meant to be immutable, make that explicit to avoid subtle bugs.
Option A (preferred): freeze the model to protect the caches (v1-style config via our shim):
class FilterSchema(BaseModel, Generic[ValueT]): @@ class Config: # Make sure that serializing Enum return values use_enum_values = True + allow_mutation = FalseOption B: if mutation is required, override setattr to invalidate cached attributes when values changes.
88-117: Double-check NOT semantics and case-sensitivity changes
- For IS_NOT/NOT_CONTAINS you return an empty set if any value violates the filter, effectively requiring all input values to satisfy the negative condition. That’s stricter than “any value is acceptable” semantics. Please confirm this is intentional across all call sites.
- IS/IS_NOT perform exact equality with the raw objects. For strings, that is now case-sensitive. If previous behavior was case-insensitive, this is a breaking change.
If case-insensitive equality is desired for strings, one approach is to pre-normalize string-only subsets:
# sketch (not a diff): build a mapping of original->lower and compare on the lowered views
119-126: Empty input handling looks correct; broaden type for flexibilityThe “negative operator + empty input => True” rule matches the spec in the PR summary. Minor nit: accept Iterable[ValueT] for symmetry with get_matching_values.
Apply this diff:
- def apply_filter_on_values(self, values: List[ValueT]) -> bool: + def apply_filter_on_values(self, values: Iterable[ValueT]) -> bool:
152-158: Specify generics for string-based filters for claritytest_ids is typed as List[FilterSchema[str]]. Consider aligning tags/owners/models similarly for consistent type hints.
Apply this diff:
- tags: List[FilterSchema] = Field(default_factory=list) - owners: List[FilterSchema] = Field(default_factory=list) - models: List[FilterSchema] = Field(default_factory=list) + tags: List[FilterSchema[str]] = Field(default_factory=list) + owners: List[FilterSchema[str]] = Field(default_factory=list) + models: List[FilterSchema[str]] = Field(default_factory=list)
274-279: Trim CLI filter tokens and drop emptiesA filter like "tags:a, b" will include " b" with a leading space. Strip tokens and ignore empty entries.
Apply this diff:
def _match_filter_regex(filter_string: str, regex: Pattern) -> List[str]: match = regex.search(filter_string) if match: - return match.group(1).split(",") + return [t.strip() for t in match.group(1).split(",") if t.strip()] return []
286-301: Selector singulars should prefer positive filtersWhen both positive and negative filters exist, picking the first value regardless of type can misrepresent the selection (e.g., tag="foo" when the active filter is is_not foo). Prefer the first IS filter; fall back only if none exist.
Apply this diff:
- tags = self.tags[0].values[0] if self.tags else None - owners = self.owners[0].values[0] if self.owners else None - models = self.models[0].values[0] if self.models else None + tags = next((f.values[0] for f in self.tags if f.type == FilterType.IS and f.values), None) + owners = next((f.values[0] for f in self.owners if f.type == FilterType.IS and f.values), None) + models = next((f.values[0] for f in self.models if f.type == FilterType.IS and f.values), None)
305-341: End-to-end behavior verification recommendedGiven the set-based rewrite, please add/adjust tests to pin the following:
- Negative filters with mixed values: e.g., owners is_not ["alice"] against ["bob","alice"] must fail.
- Empty input with negative filters returns True.
- Case-sensitivity expectations for IS/IS_NOT on strings.
- Status normalization once fixed.
I can draft unit tests targeting FiltersSchema.apply and FilterSchema.get_matching_values for these scenarios if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
elementary/monitor/data_monitoring/schema.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-quality
🔇 Additional comments (2)
elementary/monitor/data_monitoring/schema.py (2)
4-4: Confirm Python version for functools.cached_propertycached_property is available in Python 3.8+. Please confirm our minimum supported Python version; if we support <3.8 anywhere, we’d need a fallback (e.g., backports.cached_property).
5-14: LGTM on typing importsThe expanded typing imports are appropriate for the new generic/set-based approach.
ofek1weiss
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
ofek1weiss
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
null
Summary by CodeRabbit
New Features
Refactor