fix: harden ignore_models_id against explicit null, document levels-union design
Code review findings on the SIENTIAPDE-2011 PR:
- [Major] _compute_query_time_filter used group.get('ignore_models_id', [])
for the intersection, whose default only applies when the key is missing.
receiver_groups has no schema validation, so a document with
ignore_models_id explicitly set to null (key present, value None) made
.get() return None, and set(None) raised TypeError, crashing the whole
activity for every mail type. Fixed with `group.get(...) or []`. Added a
regression test.
- [Minor] CLAUDE.md:48 described Alerts' base data filter as always
`level: ERROR`, which this ticket's own query-time union can now widen.
Updated the line to reflect that.
- [Blocker, not applied] A finding argued the levels union should AND with
default_filter's own level restriction instead of replacing it, to avoid
widening Alerts (ERROR-only real-time channel) when a receiver_group sets
broader levels. Both SIENTIAPDE-2011's ticket text and the parent design
doc explicitly call for the union to replace the hard-coded ERROR-only
default ("whatever levels the active groups actually care about"), so
applying that fix would revert the subtask's actual purpose. The
interim gap the finding correctly identifies (no per-group level filter
exists yet until SIENTIAPDE-2012 lands) is now documented in
_compute_query_time_filter's docstring instead of "fixed" by narrowing
query-time behavior the ticket asked for.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -233,3 +233,23 @@ def test_compute_query_time_filter_union_levels_and_intersection_models(
|
||||
'level': {'$in': ['ERROR', 'WARNING']},
|
||||
'model_id': {'$nin': ['model_a']},
|
||||
}
|
||||
|
||||
|
||||
def test_compute_query_time_filter_ignore_models_id_explicit_none_does_not_raise(
|
||||
load_notification_package,
|
||||
):
|
||||
# receiver_groups has no schema validation, so a document can have
|
||||
# ignore_models_id explicitly set to null (key present, value None) rather
|
||||
# than omitted. `group.get('ignore_models_id', [])` would still return None
|
||||
# in that case (the default only applies when the key is missing), and
|
||||
# `set(None)` raises TypeError -- must not crash the whole activity.
|
||||
sending_configs = [
|
||||
{'group_name': 'group_a', 'levels': ['ERROR'], 'ignore_models_id': None},
|
||||
{'group_name': 'group_b', 'levels': ['ERROR'], 'ignore_models_id': ['model_a']},
|
||||
]
|
||||
default_filter = {}
|
||||
|
||||
result = load_notification_package._compute_query_time_filter(sending_configs, default_filter)
|
||||
|
||||
assert result == {'level': {'$in': ['ERROR']}}
|
||||
assert 'model_id' not in result
|
||||
|
||||
Reference in New Issue
Block a user