Merge pull request #674 from nolar/deduplicate-better

Deduplicate handlers by their target fields instead of fn alone

docs/filters.rst

@@ -94,6 +94,23 @@ with any value (for update handlers: present before or after the change).
     def field_is_affected(old, new, **_):
         pass
 
+
+Since the field name is part of the handler id (e.g., ``"fn/spec.field"``),
+multiple decorators can be defined to react to different fields with the same
+function, and it will be invoked multiple times with different old & new values
+relevant to the specified fields, so as different :kwarg:`param`:
+
+.. code-block:: python
+
+    @kopf.on.update('kopfexamples', field='spec.field', param='fld')
+    @kopf.on.update('kopfexamples', field='spec.items', param='itm')
+    def one_of_the_fields_is_affected(old, new, **_):
+        pass
+
+However, different causes --mostly resuming + one of creation/update/deletion--
+will not be distinguished, so e.g. resume+create pair with the same field
+will be called only once.
+
 Due to a special nature of the update handlers (``@on.update``, ``@on.field``),
 described in a note below, this filtering semantics is extended for them:
 

docs/kwargs.rst

@@ -63,21 +63,21 @@ possibly with multiple different selectors & filters, for one handler function:
     def child_updated(param, patch, **_):
         patch_parent({'status': {param: {'done': True}}})
 
-Note that Kopf deduplicates the functions to execute on one single occasion,
-so in this example with overlapping criteria, if ``spec.field`` is updated,
-the parameter can be either 1 or 10, and the developers cannot control which
-parameter is actually passed (except as by being more specific with filters):
+Note that Kopf deduplicates the handlers to execute on one single occasion by
+their underlying function and its id, which includes the field name by default.
+
+In this example below with overlapping criteria, if ``spec.field`` is updated,
+the handler will be called twice: one time -- for ``spec`` as a whole,
+another time -- for ``spec.field`` in particular;
+each time with the proper values of old/new/diff/param kwargs for those fields:
 
 .. code-block:: python
 
     import kopf
 
     @kopf.on.update('KopfExample', param=10, field='spec.field')
     @kopf.on.update('KopfExample', param=1, field='spec')
-    def count_updates(param, patch, name, **_): ...
-
-In practice, it is usually the first matching handler (the lowest decorator in
-the list, as Python applies them from bottom to top), but it is not guaranteed.
+    def fn(param, **_): ...
 
 
 .. kwarg:: settings

docs/resources.rst

@@ -150,7 +150,8 @@ Serving everything is better when it is used with filters:
 The resource specifications do not support multiple values, masks or globs.
 To handle multiple independent resources, add multiple decorators
 to the same handler function -- as shown above.
-The handlers are deduplicated by uniqueness of the underlying function,
+The handlers are deduplicated by the underlying function and its handler id
+(which, in turn, equals to the function's name by default unless overridden),
 so one function will never be triggered multiple times for the same resource
 if there are some accidental overlaps in the specifications.
 

kopf/on.py

@@ -171,7 +171,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -227,7 +227,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -285,7 +285,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -342,7 +342,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -452,7 +452,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -510,7 +510,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,
@@ -572,7 +572,7 @@ def decorator(  # lgtm[py/similar-function]
         _verify_filters(labels, annotations)
         real_registry = registry if registry is not None else registries.get_default_registry()
         real_field = dicts.parse_field(field) or None  # to not store tuple() as a no-field case.
-        real_id = registries.generate_id(fn=fn, id=id)
+        real_id = registries.generate_id(fn=fn, id=id, suffix=".".join(real_field or []))
         selector = references.Selector(
             __group_or_groupversion_or_name, __version_or_name, __name,
             group=group, version=version,

kopf/reactor/registries.py

@@ -15,8 +15,8 @@
 import enum
 import functools
 from types import FunctionType, MethodType
-from typing import Any, Callable, Collection, Container, Generic, Iterable, Iterator, \
-                   List, Mapping, MutableMapping, Optional, Sequence, Set, TypeVar, cast
+from typing import Any, Callable, Collection, Container, Generic, Iterable, Iterator, List, \
+                   Mapping, MutableMapping, Optional, Sequence, Set, Tuple, TypeVar, cast
 
 from kopf.reactor import causation, invocation
 from kopf.structs import callbacks, dicts, filters, handlers, references
@@ -303,7 +303,7 @@ def get_callable_id(c: Callable[..., Any]) -> str:
 
 
 def _deduplicated(
-        handlers: Iterable[HandlerT],
+        src: Iterable[HandlerT],
 ) -> Iterator[HandlerT]:
     """
     Yield the handlers deduplicated.
@@ -325,12 +325,13 @@ def fn(**kwargs): pass
     handled) **AND** it is detected as per-existing before operator start.
     But `fn()` should be called only once for this cause.
     """
-    seen_ids: Set[int] = set()
-    for handler in handlers:
-        if id(handler.fn) in seen_ids:
+    seen_ids: Set[Tuple[int, handlers.HandlerId]] = set()
+    for handler in src:
+        key = (id(handler.fn), handler.id)
+        if key in seen_ids:
             pass
         else:
-            seen_ids.add(id(handler.fn))
+            seen_ids.add(key)
             yield handler
 
 

tests/registries/test_decorators.py

@@ -267,6 +267,7 @@ def test_on_resume_with_most_kwargs(mocker, reason, cause_factory, resource):
                     id='id', registry=registry,
                     errors=ErrorsMode.PERMANENT, timeout=123, retries=456, backoff=78,
                     deleted=True,
+                    field='field.subfield', value=999,
                     labels={'somelabel': 'somevalue'},
                     annotations={'someanno': 'somevalue'},
                     when=when)
@@ -277,7 +278,7 @@ def fn(**_):
     assert len(handlers) == 1
     assert handlers[0].fn is fn
     assert handlers[0].reason is None
-    assert handlers[0].id == 'id'
+    assert handlers[0].id == 'id/field.subfield'
     assert handlers[0].errors == ErrorsMode.PERMANENT
     assert handlers[0].timeout == 123
     assert handlers[0].retries == 456
@@ -286,8 +287,8 @@ def fn(**_):
     assert handlers[0].labels == {'somelabel': 'somevalue'}
     assert handlers[0].annotations == {'someanno': 'somevalue'}
     assert handlers[0].when == when
-    assert handlers[0].field is None
-    assert handlers[0].value is None
+    assert handlers[0].field == ('field', 'subfield')
+    assert handlers[0].value == 999
     assert handlers[0].old is None
     assert handlers[0].new is None
 
@@ -302,6 +303,7 @@ def test_on_create_with_most_kwargs(mocker, cause_factory, resource):
     @kopf.on.create(*resource,
                     id='id', registry=registry,
                     errors=ErrorsMode.PERMANENT, timeout=123, retries=456, backoff=78,
+                    field='field.subfield', value=999,
                     labels={'somelabel': 'somevalue'},
                     annotations={'someanno': 'somevalue'},
                     when=when)
@@ -312,16 +314,16 @@ def fn(**_):
     assert len(handlers) == 1
     assert handlers[0].fn is fn
     assert handlers[0].reason == Reason.CREATE
-    assert handlers[0].id == 'id'
+    assert handlers[0].id == 'id/field.subfield'
     assert handlers[0].errors == ErrorsMode.PERMANENT
     assert handlers[0].timeout == 123
     assert handlers[0].retries == 456
     assert handlers[0].backoff == 78
     assert handlers[0].labels == {'somelabel': 'somevalue'}
     assert handlers[0].annotations == {'someanno': 'somevalue'}
     assert handlers[0].when == when
-    assert handlers[0].field is None
-    assert handlers[0].value is None
+    assert handlers[0].field == ('field', 'subfield')
+    assert handlers[0].value == 999
     assert handlers[0].old is None
     assert handlers[0].new is None
 
@@ -336,6 +338,7 @@ def test_on_update_with_most_kwargs(mocker, cause_factory, resource):
     @kopf.on.update(*resource,
                     id='id', registry=registry,
                     errors=ErrorsMode.PERMANENT, timeout=123, retries=456, backoff=78,
+                    field='field.subfield', value=999,
                     labels={'somelabel': 'somevalue'},
                     annotations={'someanno': 'somevalue'},
                     when=when)
@@ -346,16 +349,16 @@ def fn(**_):
     assert len(handlers) == 1
     assert handlers[0].fn is fn
     assert handlers[0].reason == Reason.UPDATE
-    assert handlers[0].id == 'id'
+    assert handlers[0].id == 'id/field.subfield'
     assert handlers[0].errors == ErrorsMode.PERMANENT
     assert handlers[0].timeout == 123
     assert handlers[0].retries == 456
     assert handlers[0].backoff == 78
     assert handlers[0].labels == {'somelabel': 'somevalue'}
     assert handlers[0].annotations == {'someanno': 'somevalue'}
     assert handlers[0].when == when
-    assert handlers[0].field is None
-    assert handlers[0].value is None
+    assert handlers[0].field == ('field', 'subfield')
+    assert handlers[0].value == 999
     assert handlers[0].old is None
     assert handlers[0].new is None
 
@@ -375,6 +378,7 @@ def test_on_delete_with_most_kwargs(mocker, cause_factory, optional, resource):
                     id='id', registry=registry,
                     errors=ErrorsMode.PERMANENT, timeout=123, retries=456, backoff=78,
                     optional=optional,
+                    field='field.subfield', value=999,
                     labels={'somelabel': 'somevalue'},
                     annotations={'someanno': 'somevalue'},
                     when=when)
@@ -385,16 +389,16 @@ def fn(**_):
     assert len(handlers) == 1
     assert handlers[0].fn is fn
     assert handlers[0].reason == Reason.DELETE
-    assert handlers[0].id == 'id'
+    assert handlers[0].id == 'id/field.subfield'
     assert handlers[0].errors == ErrorsMode.PERMANENT
     assert handlers[0].timeout == 123
     assert handlers[0].retries == 456
     assert handlers[0].backoff == 78
     assert handlers[0].labels == {'somelabel': 'somevalue'}
     assert handlers[0].annotations == {'someanno': 'somevalue'}
     assert handlers[0].when == when
-    assert handlers[0].field is None
-    assert handlers[0].value is None
+    assert handlers[0].field == ('field', 'subfield')
+    assert handlers[0].value == 999
     assert handlers[0].old is None
     assert handlers[0].new is None
 
@@ -408,9 +412,10 @@ def test_on_field_with_most_kwargs(mocker, cause_factory, resource):
 
     when = lambda **_: False
 
-    @kopf.on.field(*resource, field='field.subfield',
+    @kopf.on.field(*resource,
                    id='id', registry=registry,
                    errors=ErrorsMode.PERMANENT, timeout=123, retries=456, backoff=78,
+                   field='field.subfield', value=999,
                    labels={'somelabel': 'somevalue'},
                    annotations={'someanno': 'somevalue'},
                    when=when)
@@ -430,7 +435,7 @@ def fn(**_):
     assert handlers[0].annotations == {'someanno': 'somevalue'}
     assert handlers[0].when == when
     assert handlers[0].field == ('field', 'subfield')
-    assert handlers[0].value is None
+    assert handlers[0].value == 999
     assert handlers[0].old is None
     assert handlers[0].new is None
 

tests/registries/test_matching_for_changing.py

@@ -778,17 +778,17 @@ def some_fn_5(**_): ...  # used
     assert handlers[1].fn is some_fn_3
     assert handlers[2].fn is some_fn_5
 
+
 #
 # Same function should not be returned twice for the same event/cause.
 # Only actual for the cases when the event/cause can match multiple handlers.
 #
-
 @matching_reason_and_decorator
-def test_deduplicated(
+def test_deduplication_by_fn_and_id(
         cause_with_diff, registry, resource, reason, decorator):
 
     # Note: the decorators are applied bottom-up -- hence, the order of ids:
-    @decorator(*resource, id='b')
+    @decorator(*resource, id='a')
     @decorator(*resource, id='a')
     def some_fn(**_): ...
 
@@ -798,3 +798,37 @@ def some_fn(**_): ...
 
     assert len(handlers) == 1
     assert handlers[0].id == 'a'  # the first found one is returned
+
+
+@matching_reason_and_decorator
+def test_deduplication_distinguishes_different_fns(
+        cause_with_diff, registry, resource, reason, decorator):
+
+    # Note: the decorators are applied bottom-up -- hence, the order of ids:
+    @decorator(*resource, id='a')
+    def some_fn1(**_): ...
+
+    @decorator(*resource, id='a')
+    def some_fn2(**_): ...
+
+    cause = cause_with_diff
+    cause.reason = reason
+    handlers = registry._resource_changing.get_handlers(cause)
+
+    assert len(handlers) == 2
+
+
+@matching_reason_and_decorator
+def test_deduplication_distinguishes_different_ids(
+        cause_with_diff, registry, resource, reason, decorator):
+
+    # Note: the decorators are applied bottom-up -- hence, the order of ids:
+    @decorator(*resource, id='b')
+    @decorator(*resource, id='a')
+    def some_fn(**_): ...
+
+    cause = cause_with_diff
+    cause.reason = reason
+    handlers = registry._resource_changing.get_handlers(cause)
+
+    assert len(handlers) == 2