diff --git a/docs/merge-requests-layer3-rfc.md b/docs/merge-requests-layer3-rfc.md index 5ec0649a..984da67e 100644 --- a/docs/merge-requests-layer3-rfc.md +++ b/docs/merge-requests-layer3-rfc.md @@ -135,9 +135,31 @@ per source branch, ever; both guards are **404**, not 400 (`MergeRequestCreateAc **D1 — Explicit typed parameters, not payload dicts.** House style is named parameters with presence detection inside the method — see `update_config` (`client/configs.py:352`, *"Only -provided (non-None) fields are sent"*). `is_disabled: bool | None` stays tri-state for -consistency with `update_config` / `update_config_row`, not because the API forces it (inside a -non-empty `diff` envelope, omitting it defaults to `false` server-side). +provided (non-None) fields are sent"*). + +Presence detection is the right idiom for `update_config`, which **patches**, and the wrong one +for `rebase_config`, which **replaces**: there, an omitted key is not "leave unchanged" but +"take the server-side default". `diff.configuration` defaults to `{}` and `diff.isDisabled` to +`false`, and an absent `diff.description` to null (`RebaseRequest::mapValidatedData`). So a +tri-state `is_disabled: bool | None` and optional `configuration` / `description` would make +silent data loss the signature's default — a caller resolving a conflict on a disabled config +and passing only `name` / `rows` would wipe the configuration body, drop the description and +re-enable the config, then merge that into production. + +`ConfigurationRebaseService` settles which fields that covers: `$name` / `$description` / +`$configuration` / `$isDisabled` are *"the complete 3-way diff result"* and *"fully replace"* the +resolved version's body. All four are therefore **required** parameters, alongside `rows` (which +the backend rejects the request without). `description` is required-but-nullable — `None` is a +legitimate resolved value meaning "no description", and it omits the key rather than sending an +explicit null, which costs no expressiveness because the two are indistinguishable server-side. + +`change_description` is the one genuine optional: it is not part of the replaced body, and null +selects a default rebase message rather than clearing anything. + +The same reasoning applies to `_optional_mr_fields` in `client/merge_requests.py`, where +presence detection *is* correct (create and update genuinely patch): the helper is keyword-only, +because four of its five parameters are `str | None` and a positional transposition would +type-check cleanly and surface only as a backend 422. **D2 — JSON bodies throughout, deviating from `configs.py`.** Per *Request bodies are JSON*: `json=`, real nested objects, no `json.dumps`, no `"1"` / `"0"` booleans. Every method gets a @@ -329,7 +351,7 @@ Branch-scoped; `branch_id` required (D5). | Method | Endpoint | |---|---| | `get_config_diff(component_id, config_id, branch_id) -> dict` | `GET …/branch/{branch_id}/components/{c}/configs/{cfg}/diff` | -| `rebase_config(component_id, config_id, branch_id, version, name, rows, configuration=None, description=None, change_description=None, is_disabled=None) -> dict` | `POST …/rebase` (keep) | +| `rebase_config(component_id, config_id, branch_id, version, name, rows, configuration, is_disabled, description, change_description=None) -> dict` | `POST …/rebase` (keep) | | `rebase_config_delete(component_id, config_id, branch_id, version) -> dict` | `POST …/rebase` (delete) | `get_config_diff` returns the three-way diff (`base` = dev branch v1, `ours` = dev head, @@ -337,8 +359,10 @@ Branch-scoped; `branch_id` required (D5). Flattening the nested `diff` payload is Layer 2's job. The Python signatures are flat; only the body construction knows about the envelope. -`rebase_config` sends `version` at the top level and puts `name` and `rows` (always) plus any -non-`None` optional inside `diff`; `is_disabled=None` is omitted, `is_disabled=False` is sent. +`rebase_config` sends `version` at the top level and puts the five required content fields +(`name`, `rows`, `configuration`, `is_disabled`, `description`) plus `change_description` when +set inside `diff`. `description=None` is required-but-nullable: it omits the key, which is how +"the resolved config has no description" is expressed. `rebase_config_delete` sends exactly `{"version": N, "diff": {}}`. Component and configuration ids are `quote()`d, as everywhere in `configs.py`. diff --git a/src/keboola_agent_cli/client/configs.py b/src/keboola_agent_cli/client/configs.py index a5109887..5e8de9d1 100644 --- a/src/keboola_agent_cli/client/configs.py +++ b/src/keboola_agent_cli/client/configs.py @@ -685,10 +685,10 @@ def rebase_config( version: int, name: str, rows: list[dict[str, Any]], - configuration: dict[str, Any] | None = None, - description: str | None = None, + configuration: dict[str, Any], + is_disabled: bool, + description: str | None, change_description: str | None = None, - is_disabled: bool | None = None, ) -> dict[str, Any]: """Rebase a dev-branch configuration onto a newer default-branch version. @@ -698,11 +698,21 @@ def rebase_config( The resolved content travels in a ``diff`` envelope mirroring the shape ``get_config_diff`` returns each side in, so a resolved diff - side posts back nearly 1:1. ``name`` and ``rows`` are required by the - backend for a keep rebase (``rows=[]`` legitimately deletes all - rows); to resolve a conflict by DELETING the config, use - ``rebase_config_delete`` -- the two rebase kinds are separate methods - on purpose, so no illegal combination is expressible (RFC, D6). + side posts back nearly 1:1. Rebase REPLACES the configuration rather + than patching it: server-side, ``name`` / ``description`` / + ``configuration`` / ``isDisabled`` are "the complete 3-way diff + result" and "fully replace" the resolved version's body + (``ConfigurationRebaseService``). So all four are required here, not + optional -- an omitted key is not "leave unchanged" but "take the + server-side default", which for ``configuration`` is ``{}``, for + ``isDisabled`` is ``false`` and for ``description`` is null. ``rows`` + is required too, because the backend rejects a keep rebase without + it (``rows=[]`` legitimately deletes all rows). Only + ``change_description`` is genuinely optional: it is not part of the + replaced body, and null selects a default rebase message. To resolve + a conflict by DELETING the config, use ``rebase_config_delete`` -- + the two rebase kinds are separate methods on purpose, so no illegal + combination is expressible (RFC, D6). ``branch_id`` is required with no production fallback (see ``get_config_diff``); the endpoint also requires the @@ -722,28 +732,35 @@ def rebase_config( (``{id?, name?, description?, isDisabled?, configuration?}``); missing/null ``id`` means a new row, duplicates are rejected, array order becomes sort order. - configuration: Resolved configuration body (backend default: {}). - description: Resolved description. ``None`` omits the key -- - which loses nothing: server-side an explicit JSON null and - an absent key are indistinguishable (``isset`` mapping). + configuration: Resolved configuration body. Required: on a + missing key the backend substitutes ``{}``, wiping it. + is_disabled: Resolved disabled flag. Required: on a missing key + the backend substitutes ``false``, re-enabling a config that + was disabled. Not the tri-state of ``update_config`` + (RFC, D1) -- that method patches, this one replaces. + description: Resolved description. Required, but ``None`` is a + legitimate resolved value -- it means the rebased config + ends up with no description. ``None`` omits the key rather + than sending an explicit null, which costs no expressiveness + because server-side the two are indistinguishable (``isset`` + mapping). It has no default precisely because that default + would silently drop an existing description. change_description: Change log message; when ``None``/omitted the backend uses a default rebase message. - is_disabled: When None, omitted (backend defaults to False); - False is sent explicitly -- tri-state for consistency with - ``update_config`` (RFC, D1). Returns: The rebased configuration dict. """ - diff: dict[str, Any] = {"name": name, "rows": rows} - if configuration is not None: - diff["configuration"] = configuration + diff: dict[str, Any] = { + "name": name, + "rows": rows, + "configuration": configuration, + "isDisabled": is_disabled, + } if description is not None: diff["description"] = description if change_description is not None: diff["changeDescription"] = change_description - if is_disabled is not None: - diff["isDisabled"] = is_disabled return self._rebase_request(component_id, config_id, branch_id, version, diff) def rebase_config_delete( diff --git a/src/keboola_agent_cli/client/merge_requests.py b/src/keboola_agent_cli/client/merge_requests.py index 6e17acdb..ed328f85 100644 --- a/src/keboola_agent_cli/client/merge_requests.py +++ b/src/keboola_agent_cli/client/merge_requests.py @@ -79,6 +79,7 @@ def wait_for_storage_job( def _optional_mr_fields( + *, description: str | None, reviewer_ids: _IntList | None, auto_merge_strategy: str | None, @@ -88,7 +89,10 @@ def _optional_mr_fields( """Build the optional-field part of a create/update body. Shared so the two bodies cannot drift: only provided (non-None) fields - are included, under their wire (camelCase) names. + are included, under their wire (camelCase) names. Keyword-only by + signature: four of the five parameters are ``str | None``, so a + positional transposition would type-check cleanly and only surface as a + backend 422 -- the one drift mode this helper otherwise cannot catch. """ body: dict[str, Any] = {} if description is not None: @@ -194,7 +198,11 @@ def create( } body.update( _optional_mr_fields( - description, reviewer_ids, auto_merge_strategy, auto_merge_at, external_id + description=description, + reviewer_ids=reviewer_ids, + auto_merge_strategy=auto_merge_strategy, + auto_merge_at=auto_merge_at, + external_id=external_id, ) ) return self._requester.request("POST", _BASE, json=body).json() @@ -222,7 +230,11 @@ def update( PUTs ``{}``, which the backend treats as a no-op returning the MR. """ body = _optional_mr_fields( - description, reviewer_ids, auto_merge_strategy, auto_merge_at, external_id + description=description, + reviewer_ids=reviewer_ids, + auto_merge_strategy=auto_merge_strategy, + auto_merge_at=auto_merge_at, + external_id=external_id, ) if title is not None: body["title"] = title diff --git a/tests/test_merge_request_client.py b/tests/test_merge_request_client.py index 77712360..2ef9a03b 100644 --- a/tests/test_merge_request_client.py +++ b/tests/test_merge_request_client.py @@ -8,6 +8,7 @@ the namespace-never-touches-the-client seam. """ +import inspect import json from typing import Any @@ -15,7 +16,7 @@ import pytest from keboola_agent_cli.client import KeboolaClient -from keboola_agent_cli.client.merge_requests import MergeRequests +from keboola_agent_cli.client.merge_requests import MergeRequests, _optional_mr_fields from keboola_agent_cli.errors import ErrorCode, KeboolaApiError STACK_URL = "https://connection.keboola.com" @@ -325,9 +326,9 @@ def test_keep_rebase_builds_the_envelope(self, client, httpx_mock) -> None: name="My config", rows=[{"id": "r1", "name": "Row 1"}], configuration={"parameters": {"baseUrl": "https://example.com"}}, + is_disabled=False, description="resolved", change_description="rebase onto v7", - is_disabled=False, ) body = _sent_json(httpx_mock.get_requests()[0]) @@ -346,15 +347,78 @@ def test_keep_rebase_builds_the_envelope(self, client, httpx_mock) -> None: def test_keep_rebase_omits_unset_optionals_and_sends_empty_rows( self, client, httpx_mock ) -> None: - """is_disabled=None is omitted; rows=[] is sent (it deletes all rows).""" + """description=None and an unset change_description are omitted; rows=[] is sent.""" httpx_mock.add_response(url=self.REBASE_URL, json={}) client.rebase_config( - "keboola.ex-http", "cfg-1", branch_id=123, version=7, name="My config", rows=[] + "keboola.ex-http", + "cfg-1", + branch_id=123, + version=7, + name="My config", + rows=[], + configuration={}, + is_disabled=False, + description=None, ) body = _sent_json(httpx_mock.get_requests()[0]) - assert body["diff"] == {"name": "My config", "rows": []} + assert body["diff"] == { + "name": "My config", + "rows": [], + "configuration": {}, + "isDisabled": False, + } + + def test_keep_rebase_always_sends_configuration_and_is_disabled( + self, client, httpx_mock + ) -> None: + """Rebase REPLACES, so no body field may be left to a server-side default. + + A missing ``diff.configuration`` is substituted with ``{}`` and a missing + ``diff.isDisabled`` with ``false``, so omitting either would wipe the + configuration body and re-enable a disabled config. Both are required + parameters; this pins that they always reach the wire. + """ + httpx_mock.add_response(url=self.REBASE_URL, json={}) + + client.rebase_config( + "keboola.ex-http", + "cfg-1", + branch_id=123, + version=7, + name="My config", + rows=[], + configuration={"parameters": {"keep": "me"}}, + is_disabled=True, + description="kept", + ) + + diff = _sent_json(httpx_mock.get_requests()[0])["diff"] + assert diff["configuration"] == {"parameters": {"keep": "me"}} + assert diff["isDisabled"] is True + assert diff["description"] == "kept" + + def test_keep_rebase_requires_every_replaced_body_field(self, client) -> None: + """Omitting any replaced body field is a TypeError, not silent loss on the wire. + + ``name`` / ``description`` / ``configuration`` / ``isDisabled`` are the + tuple the backend calls "the complete 3-way diff result" and fully + replaces the resolved version's body with, so none of them may carry a + default here. ``change_description`` is not part of that tuple and stays + optional -- omitting it selects a default rebase message. + """ + required = { + "name": "N", + "rows": [], + "configuration": {}, + "is_disabled": False, + "description": None, + } + for omitted in required: + kwargs = {k: v for k, v in required.items() if k != omitted} + with pytest.raises(TypeError): + client.rebase_config("keboola.ex-http", "cfg-1", branch_id=123, version=7, **kwargs) def test_delete_rebase_sends_empty_diff_object(self, client, httpx_mock) -> None: """Delete resolution is exactly {"version": N, "diff": {}} -- diff a JSON object.""" @@ -389,6 +453,40 @@ def wait_for_storage_job( assert namespace.list() == [SAMPLE_MR] assert calls == [("GET", "/v2/storage/merge-request")] + +class TestOptionalFieldHelper: + """_optional_mr_fields is keyword-only, so create/update cannot transpose.""" + + def test_every_field_is_keyword_only(self) -> None: + """No parameter may be positional -- a transposition would type-check cleanly. + + Asserted on the signature rather than by making a deliberately wrong + call: a positional call is a static error too (which is the point), so + writing one would just mean fighting ``ty`` to prove ``ty`` is right. + """ + kinds = { + name: param.kind + for name, param in inspect.signature(_optional_mr_fields).parameters.items() + } + assert set(kinds) == { + "description", + "reviewer_ids", + "auto_merge_strategy", + "auto_merge_at", + "external_id", + } + assert all(kind is inspect.Parameter.KEYWORD_ONLY for kind in kinds.values()), kinds + + def test_helper_omits_unset_fields(self) -> None: + """Only non-None fields are included, under their camelCase wire names.""" + assert _optional_mr_fields( + description=None, + reviewer_ids=[7], + auto_merge_strategy=None, + auto_merge_at=None, + external_id="DMD-1701", + ) == {"reviewerIds": [7], "externalId": "DMD-1701"} + def test_namespace_is_cached_on_the_client(self) -> None: """client.merge_requests returns the same namespace instance every time.""" client = KeboolaClient(stack_url=STACK_URL, token=TOKEN)