Reference objects by mandatory name+namespace

Identifying the objects by partial identities or by its body is not used for long time. Let's get rid of this code, and always identify them explicitly by name & namespace, even if namespace is `None` (for cluster-scoped API calls).

For stricter typing, the body's name property is made more strict — it now raises if the critical field (`.metadata.name`) is absent while it is always expected on the objects for URLs. Objects with no name cannot be served.

kopf/clients/fetching.py

@@ -17,8 +17,8 @@ class _UNSET(enum.Enum):
 async def read_obj(
         *,
         resource: resources.Resource,
-        namespace: Optional[str] = None,
-        name: Optional[str] = None,
+        namespace: Optional[str],
+        name: str,
         default: Union[_T, _UNSET] = _UNSET.token,
         context: Optional[auth.APIContext] = None,  # injected by the decorator
 ) -> Union[bodies.RawBody, _T]:

kopf/clients/patching.py

@@ -1,4 +1,4 @@
-from typing import Optional, cast
+from typing import Optional
 
 from kopf.clients import auth, discovery, errors
 from kopf.structs import bodies, patches, resources
@@ -8,18 +8,14 @@
 async def patch_obj(
         *,
         resource: resources.Resource,
+        namespace: Optional[str],
+        name: str,
         patch: patches.Patch,
-        namespace: Optional[str] = None,
-        name: Optional[str] = None,
-        body: Optional[bodies.Body] = None,
         context: Optional[auth.APIContext] = None,  # injected by the decorator
 ) -> Optional[bodies.RawBody]:
     """
     Patch a resource of specific kind.
 
-    Either the namespace+name should be specified, or the body,
-    which is used only to get namespace+name identifiers.
-
     Unlike the object listing, the namespaced call is always
     used for the namespaced resources, even if the operator serves
     the whole cluster (i.e. is not namespace-restricted).
@@ -38,20 +34,9 @@ async def patch_obj(
     if context is None:
         raise RuntimeError("API instance is not injected by the decorator.")
 
-    if body is not None and (name is not None or namespace is not None):
-        raise TypeError("Either body, or name+namespace can be specified. Got both.")
-
-    namespace = body.get('metadata', {}).get('namespace') if body is not None else namespace
-    name = body.get('metadata', {}).get('name') if body is not None else name
-
     is_namespaced = await discovery.is_namespaced(resource=resource, context=context)
     namespace = namespace if is_namespaced else None
 
-    if body is None:
-        body = cast(bodies.Body, {'metadata': {'name': name}})
-        if namespace is not None:
-            body['metadata']['namespace'] = namespace
-
     as_subresource = await discovery.is_status_subresource(resource=resource, context=context)
     body_patch = dict(patch)  # shallow: for mutation of the top-level keys below.
     status_patch = body_patch.pop('status', None) if as_subresource else None

kopf/clients/watching.py

@@ -169,7 +169,7 @@ async def watch_objs(
         *,
         settings: configuration.OperatorSettings,
         resource: resources.Resource,
-        namespace: Optional[str] = None,
+        namespace: Optional[str],
         timeout: Optional[float] = None,
         since: Optional[str] = None,
         context: Optional[auth.APIContext] = None,  # injected by the decorator

kopf/reactor/effects.py

@@ -117,7 +117,12 @@ async def patch_and_check(
     """
     if patch:
         logger.debug(f"Patching with: {patch!r}")
-        resulting_body = await patching.patch_obj(resource=resource, patch=patch, body=body)
+        resulting_body = await patching.patch_obj(
+            resource=resource,
+            namespace=body.metadata.namespace,
+            name=body.metadata.name,
+            patch=patch,
+        )
         inconsistencies = diffs.diff(patch, resulting_body, scope=diffs.DiffScope.LEFT)
         inconsistencies = diffs.Diff(
             diffs.DiffItem(op, field, old, new)

kopf/structs/bodies.py

@@ -148,8 +148,11 @@ def uid(self) -> Optional[str]:
         return cast(Optional[str], self.get('uid'))
 
     @property
-    def name(self) -> Optional[str]:
-        return cast(Optional[str], self.get('name'))
+    def name(self) -> str:
+        result = self.get('name')
+        if result is None:
+            raise ValueError("Name is absent in the metadata.")
+        return cast(str, result)
 
     @property
     def namespace(self) -> Optional[str]:

tests/k8s/test_patching.py

@@ -3,12 +3,11 @@
 
 from kopf.clients.errors import APIError
 from kopf.clients.patching import patch_obj
-from kopf.structs.bodies import Body
 from kopf.structs.patches import Patch
 
 
 @pytest.mark.resource_clustered  # see `resp_mocker`
-async def test_by_name_clustered(
+async def test_clustered(
         resp_mocker, aresponses, hostname, resource):
 
     patch_mock = resp_mocker(return_value=aiohttp.web.json_response({}))
@@ -24,7 +23,7 @@ async def test_by_name_clustered(
     assert data == {'x': 'y'}
 
 
-async def test_by_name_namespaced(
+async def test_namespaced(
         resp_mocker, aresponses, hostname, resource):
 
     patch_mock = resp_mocker(return_value=aiohttp.web.json_response({}))
@@ -40,46 +39,10 @@ async def test_by_name_namespaced(
     assert data == {'x': 'y'}
 
 
-@pytest.mark.resource_clustered  # see `resp_mocker`
-async def test_by_body_clustered(
-        resp_mocker, aresponses, hostname, resource):
-
-    patch_mock = resp_mocker(return_value=aiohttp.web.json_response({}))
-    aresponses.add(hostname, resource.get_url(namespace=None, name='name1'), 'patch', patch_mock)
-
-    body = Body({'metadata': {'name': 'name1'}})
-    patch = Patch({'x': 'y'})
-    await patch_obj(resource=resource, body=body, patch=patch)
-
-    assert patch_mock.called
-    assert patch_mock.call_count == 1
-
-    data = patch_mock.call_args_list[0][0][0].data  # [callidx][args/kwargs][argidx]
-    assert data == {'x': 'y'}
-
-
-async def test_by_body_namespaced(
-        resp_mocker, aresponses, hostname, resource):
-
-    patch_mock = resp_mocker(return_value=aiohttp.web.json_response({}))
-    aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
-
-    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
-    patch = Patch({'x': 'y'})
-    await patch_obj(resource=resource, body=body, patch=patch)
-
-    assert patch_mock.called
-    assert patch_mock.call_count == 1
-
-    data = patch_mock.call_args_list[0][0][0].data  # [callidx][args/kwargs][argidx]
-    assert data == {'x': 'y'}
-
-
 async def test_status_as_subresource_with_combined_payload(
         resp_mocker, aresponses, hostname, resource, version_api_with_substatus):
 
     # Simulate Kopf's initial state and intention.
-    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
     patch = Patch({'spec': {'x': 'y'}, 'status': {'s': 't'}})
 
     # Simulate K8s API's behaviour. Assume something extra is added remotely.
@@ -95,7 +58,7 @@ async def test_status_as_subresource_with_combined_payload(
     aresponses.add(hostname, object_url, 'patch', object_patch_mock)
     aresponses.add(hostname, status_url, 'patch', status_patch_mock)
 
-    reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
+    reconstructed = await patch_obj(resource=resource, namespace='ns1', name='name1', patch=patch)
 
     assert object_patch_mock.called
     assert object_patch_mock.call_count == 1
@@ -116,7 +79,6 @@ async def test_status_as_subresource_with_object_fields_only(
         resp_mocker, aresponses, hostname, resource, version_api_with_substatus):
 
     # Simulate Kopf's initial state and intention.
-    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
     patch = Patch({'spec': {'x': 'y'}})
 
     # Simulate K8s API's behaviour. Assume something extra is added remotely.
@@ -132,7 +94,7 @@ async def test_status_as_subresource_with_object_fields_only(
     aresponses.add(hostname, object_url, 'patch', object_patch_mock)
     aresponses.add(hostname, status_url, 'patch', status_patch_mock)
 
-    reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
+    reconstructed = await patch_obj(resource=resource, namespace='ns1', name='name1', patch=patch)
 
     assert object_patch_mock.called
     assert object_patch_mock.call_count == 1
@@ -150,7 +112,6 @@ async def test_status_as_subresource_with_status_fields_only(
         resp_mocker, aresponses, hostname, resource, version_api_with_substatus):
 
     # Simulate Kopf's initial state and intention.
-    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
     patch = Patch({'status': {'s': 't'}})
 
     # Simulate K8s API's behaviour. Assume something extra is added remotely.
@@ -166,7 +127,7 @@ async def test_status_as_subresource_with_status_fields_only(
     aresponses.add(hostname, object_url, 'patch', object_patch_mock)
     aresponses.add(hostname, status_url, 'patch', status_patch_mock)
 
-    reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
+    reconstructed = await patch_obj(resource=resource, namespace='ns1', name='name1', patch=patch)
 
     assert not object_patch_mock.called
     assert status_patch_mock.called
@@ -182,7 +143,6 @@ async def test_status_as_body_field_with_combined_payload(
         resp_mocker, aresponses, hostname, resource):
 
     # Simulate Kopf's initial state and intention.
-    body = Body({'metadata': {'namespace': 'ns1', 'name': 'name1'}})
     patch = Patch({'spec': {'x': 'y'}, 'status': {'s': 't'}})
 
     # Simulate K8s API's behaviour. Assume something extra is added remotely.
@@ -198,7 +158,7 @@ async def test_status_as_body_field_with_combined_payload(
     aresponses.add(hostname, object_url, 'patch', object_patch_mock)
     aresponses.add(hostname, status_url, 'patch', status_patch_mock)
 
-    reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
+    reconstructed = await patch_obj(resource=resource, namespace='ns1', name='name1', patch=patch)
 
     assert object_patch_mock.called
     assert object_patch_mock.call_count == 1
@@ -212,51 +172,6 @@ async def test_status_as_body_field_with_combined_payload(
                              'status': '...'}
 
 
-async def test_raises_when_body_conflicts_with_namespace(
-        resp_mocker, aresponses, hostname, resource):
-
-    patch_mock = resp_mocker(return_value=aiohttp.web.json_response())
-    aresponses.add(hostname, resource.get_url(namespace=None, name='name1'), 'patch', patch_mock)
-    aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
-
-    patch = {'x': 'y'}
-    body = {'metadata': {'namespace': 'ns1', 'name': 'name1'}}
-    with pytest.raises(TypeError):
-        await patch_obj(resource=resource, body=body, namespace='ns1', patch=patch)
-
-    assert not patch_mock.called
-
-
-async def test_raises_when_body_conflicts_with_name(
-        resp_mocker, aresponses, hostname, resource):
-
-    patch_mock = resp_mocker(return_value=aiohttp.web.json_response())
-    aresponses.add(hostname, resource.get_url(namespace=None, name='name1'), 'patch', patch_mock)
-    aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
-
-    patch = {'x': 'y'}
-    body = {'metadata': {'namespace': 'ns1', 'name': 'name1'}}
-    with pytest.raises(TypeError):
-        await patch_obj(resource=resource, body=body, name='name1', patch=patch)
-
-    assert not patch_mock.called
-
-
-async def test_raises_when_body_conflicts_with_ids(
-        resp_mocker, aresponses, hostname, resource):
-
-    patch_mock = resp_mocker(return_value=aiohttp.web.json_response())
-    aresponses.add(hostname, resource.get_url(namespace=None, name='name1'), 'patch', patch_mock)
-    aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
-
-    patch = {'x': 'y'}
-    body = {'metadata': {'namespace': 'ns1', 'name': 'name1'}}
-    with pytest.raises(TypeError):
-        await patch_obj(resource=resource, body=body, namespace='ns1', name='name1', patch=patch)
-
-    assert not patch_mock.called
-
-
 @pytest.mark.parametrize('namespace', [None, 'ns1'], ids=['without-namespace', 'with-namespace'])
 @pytest.mark.parametrize('status', [404])
 async def test_ignores_absent_objects(
@@ -267,10 +182,9 @@ async def test_ignores_absent_objects(
     aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
 
     patch = {'x': 'y'}
-    body = {'metadata': {'namespace': namespace, 'name': 'name1'}}
-    reconstructed = await patch_obj(resource=resource, body=body, patch=patch)
+    result = await patch_obj(resource=resource, namespace=namespace, name='name1', patch=patch)
 
-    assert reconstructed is None
+    assert result is None
 
 
 @pytest.mark.parametrize('namespace', [None, 'ns1'], ids=['without-namespace', 'with-namespace'])
@@ -283,7 +197,6 @@ async def test_raises_api_errors(
     aresponses.add(hostname, resource.get_url(namespace='ns1', name='name1'), 'patch', patch_mock)
 
     patch = {'x': 'y'}
-    body = {'metadata': {'namespace': namespace, 'name': 'name1'}}
     with pytest.raises(APIError) as e:
-        await patch_obj(resource=resource, body=body, patch=patch)
+        await patch_obj(resource=resource, namespace=namespace, name='name1', patch=patch)
     assert e.value.status == status