SIENTIAPDE-2149: model_ids allow-list filter for receiver groups
New, separate model_id-based filter distinct from the existing ignore_models_id deny-list (SIENTIAPDE-2012): a group configuring model_ids only receives notifications from those models. Both axes apply together (AND) when a group sets both. Design decisions made while implementing (matching the levels/ ignore_models_id split from SIENTIAPDE-2011/2012, not yet confirmed with the team beyond what's in the ticket): - Query-time filter: union of every active group's model_ids, applied only when ALL active groups set one -- mirrors levels' exact union-or-nothing logic in _compute_query_time_filter. Combines cleanly with ignore_models_id's intersection $nin on the same model_id field. - Per-group filter: extended _check_levels_and_ignore_models_id (now _check_levels_and_model_filters) in slot_manager.py, and the inline condition in formatters.py's filter_notification_reports. - Empty/missing model_ids means no restriction, same convention as levels and ignore_models_id. Covered by new unit tests (slot_manager, formatters, _compute_query_time_filter) and two new e2e tests against real Mongo/Redis/Temporal: per-group allow-list interacting with another unrestricted group, and the query-time union excluding a model outside every active group's allow-list. Not yet verified on the real cluster -- blocked on the dev cluster's package index not auto-syncing new sientia_do releases (separate issue, being investigated for SIENTIAPDE-2013). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -852,6 +852,123 @@ def test_filter_notification_reports_ignore_models_id_restriction(formatters):
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_reports_model_ids_allow_list_restriction(formatters):
|
||||
# SIENTIAPDE-2149: a group with `model_ids` set only receives notifications
|
||||
# from those models, unlike `ignore_models_id` which is a deny-list.
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['reports'],
|
||||
'model_ids': ['model_x'],
|
||||
},
|
||||
],
|
||||
}
|
||||
|
||||
response = formatters.filter_notification_reports(input_data)
|
||||
|
||||
assert response == {
|
||||
'test_group_1': {
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['reports'],
|
||||
'model_ids': ['model_x'],
|
||||
'notifications': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
],
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_reports_model_ids_and_ignore_models_id_together(formatters):
|
||||
# SIENTIAPDE-2149: the allow-list and deny-list apply together (AND), not
|
||||
# as alternatives -- a model must pass both checks.
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_3',
|
||||
'notification_id': 'test_notification_id_3',
|
||||
'model_id': 'model_z',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['reports'],
|
||||
'model_ids': ['model_x', 'model_y'],
|
||||
'ignore_models_id': ['model_x'],
|
||||
},
|
||||
],
|
||||
}
|
||||
|
||||
response = formatters.filter_notification_reports(input_data)
|
||||
|
||||
assert response == {
|
||||
'test_group_1': {
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['reports'],
|
||||
'model_ids': ['model_x', 'model_y'],
|
||||
'ignore_models_id': ['model_x'],
|
||||
'notifications': [
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
],
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_reports_model_ids_explicit_none_does_not_raise(formatters):
|
||||
# Same defensive concern as ignore_models_id: receiver_groups has no schema
|
||||
# validation, so model_ids can be explicitly null instead of omitted.
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{'group_name': 'test_group_1', 'contents': ['reports'], 'model_ids': None},
|
||||
],
|
||||
}
|
||||
|
||||
response = formatters.filter_notification_reports(input_data)
|
||||
|
||||
assert response['test_group_1']['notifications'] == input_data['notification_package']
|
||||
|
||||
|
||||
def test_filter_notification_reports_no_levels_or_ignore_models_id_passes_everything(
|
||||
formatters,
|
||||
):
|
||||
|
||||
@@ -481,6 +481,137 @@ def test_filter_notification_alerts_ignore_models_id_restriction(slot_manager):
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_alerts_model_ids_allow_list_restriction(slot_manager):
|
||||
# SIENTIAPDE-2149: a group with `model_ids` set only receives notifications
|
||||
# from those models, unlike `ignore_models_id` which is a deny-list.
|
||||
slot_manager.redis_repository.get = MagicMock(side_effect=[None, None])
|
||||
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['core_alerts'],
|
||||
'model_ids': ['model_x'],
|
||||
},
|
||||
],
|
||||
'notification_ttl': 300,
|
||||
'mail_type': 'test_mail_type',
|
||||
}
|
||||
|
||||
response = slot_manager.filter_notification_alerts(input_data)
|
||||
|
||||
assert response == {
|
||||
'test_group_1': {
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['core_alerts'],
|
||||
'model_ids': ['model_x'],
|
||||
'notifications': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
],
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_alerts_model_ids_and_ignore_models_id_together(slot_manager):
|
||||
# SIENTIAPDE-2149: the allow-list and deny-list apply together (AND), not
|
||||
# as alternatives -- a model must pass both checks.
|
||||
slot_manager.redis_repository.get = MagicMock(side_effect=[None, None, None])
|
||||
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
{
|
||||
'trigger': 'test_trigger_3',
|
||||
'notification_id': 'test_notification_id_3',
|
||||
'model_id': 'model_z',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['core_alerts'],
|
||||
'model_ids': ['model_x', 'model_y'],
|
||||
'ignore_models_id': ['model_x'],
|
||||
},
|
||||
],
|
||||
'notification_ttl': 300,
|
||||
'mail_type': 'test_mail_type',
|
||||
}
|
||||
|
||||
response = slot_manager.filter_notification_alerts(input_data)
|
||||
|
||||
# model_x: allowed but ignored -> excluded. model_y: allowed and not
|
||||
# ignored -> included. model_z: not in the allow-list -> excluded.
|
||||
assert response == {
|
||||
'test_group_1': {
|
||||
'group_name': 'test_group_1',
|
||||
'contents': ['core_alerts'],
|
||||
'model_ids': ['model_x', 'model_y'],
|
||||
'ignore_models_id': ['model_x'],
|
||||
'notifications': [
|
||||
{
|
||||
'trigger': 'test_trigger_2',
|
||||
'notification_id': 'test_notification_id_2',
|
||||
'model_id': 'model_y',
|
||||
},
|
||||
],
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_filter_notification_alerts_model_ids_explicit_none_does_not_raise(slot_manager):
|
||||
# Same defensive concern as ignore_models_id: receiver_groups has no schema
|
||||
# validation, so model_ids can be explicitly null instead of omitted.
|
||||
slot_manager.redis_repository.get = MagicMock(side_effect=[None])
|
||||
|
||||
input_data = {
|
||||
**metadata,
|
||||
'notification_package': [
|
||||
{
|
||||
'trigger': 'test_trigger_1',
|
||||
'notification_id': 'test_notification_id_1',
|
||||
'model_id': 'model_x',
|
||||
},
|
||||
],
|
||||
'sending_configs': [
|
||||
{'group_name': 'test_group_1', 'contents': ['core_alerts'], 'model_ids': None},
|
||||
],
|
||||
'notification_ttl': 300,
|
||||
'mail_type': 'test_mail_type',
|
||||
}
|
||||
|
||||
response = slot_manager.filter_notification_alerts(input_data)
|
||||
|
||||
assert response['test_group_1']['notifications'] == input_data['notification_package']
|
||||
|
||||
|
||||
def test_filter_notification_alerts_no_levels_or_ignore_models_id_passes_everything(
|
||||
slot_manager,
|
||||
):
|
||||
|
||||
@@ -237,6 +237,65 @@ def test_compute_query_time_filter_model_ignored_by_only_one_group_not_excluded(
|
||||
assert 'model_id' not in result
|
||||
|
||||
|
||||
def test_compute_query_time_filter_model_ids_union_when_all_groups_restrict(
|
||||
load_notification_package,
|
||||
):
|
||||
# SIENTIAPDE-2149: every active group sets a model_ids allow-list -> the
|
||||
# union applies at query time, mirroring how `levels` already behaves.
|
||||
sending_configs = [
|
||||
{'group_name': 'group_a', 'model_ids': ['model_a']},
|
||||
{'group_name': 'group_b', 'model_ids': ['model_b']},
|
||||
]
|
||||
default_filter = {}
|
||||
|
||||
result = load_notification_package._compute_query_time_filter(sending_configs, default_filter)
|
||||
|
||||
assert result == {'model_id': {'$in': ['model_a', 'model_b']}}
|
||||
|
||||
|
||||
def test_compute_query_time_filter_model_ids_no_restriction_when_any_group_missing(
|
||||
load_notification_package,
|
||||
):
|
||||
# One active group has no model_ids restriction -> it wants every model,
|
||||
# so the query can't restrict to an allow-list at all.
|
||||
sending_configs = [
|
||||
{'group_name': 'group_a', 'model_ids': ['model_a']},
|
||||
{'group_name': 'group_b'},
|
||||
]
|
||||
default_filter = {}
|
||||
|
||||
result = load_notification_package._compute_query_time_filter(sending_configs, default_filter)
|
||||
|
||||
assert 'model_id' not in result
|
||||
|
||||
|
||||
def test_compute_query_time_filter_model_ids_union_and_ignore_models_id_intersection_together(
|
||||
load_notification_package,
|
||||
):
|
||||
# SIENTIAPDE-2149: the allow-list union ($in) and the ignore-list
|
||||
# intersection ($nin) are independent axes and can both narrow the same
|
||||
# model_id field at once.
|
||||
sending_configs = [
|
||||
{
|
||||
'group_name': 'group_a',
|
||||
'model_ids': ['model_a', 'model_b'],
|
||||
'ignore_models_id': ['model_a'],
|
||||
},
|
||||
{
|
||||
'group_name': 'group_b',
|
||||
'model_ids': ['model_b', 'model_c'],
|
||||
'ignore_models_id': ['model_a'],
|
||||
},
|
||||
]
|
||||
default_filter = {}
|
||||
|
||||
result = load_notification_package._compute_query_time_filter(sending_configs, default_filter)
|
||||
|
||||
assert result == {
|
||||
'model_id': {'$in': ['model_a', 'model_b', 'model_c'], '$nin': ['model_a']},
|
||||
}
|
||||
|
||||
|
||||
def test_compute_query_time_filter_union_levels_and_intersection_models(
|
||||
load_notification_package,
|
||||
):
|
||||
|
||||
Reference in New Issue
Block a user