diff --git a/src/keboola_agent_cli/client/_client.py b/src/keboola_agent_cli/client/_client.py index ce753950..b27c2dad 100644 --- a/src/keboola_agent_cli/client/_client.py +++ b/src/keboola_agent_cli/client/_client.py @@ -1,11 +1,12 @@ """Composition of the Keboola API client from its endpoint-family mixins. ``KeboolaClient`` is assembled here from the per-family mixins (storage tables, -storage files, configs, queue, tokens, branches, stream, query, workspaces, -billing, misc) over the shared ``_CoreClient`` plumbing base. It stays a -single class exposing every Storage/Queue method at its original signature, -so ``keboola_agent_cli.Client`` and its ``.raw`` accessor are unaffected by -the split of the former single-file ``client.py`` into a package (issue #520). +storage files, configs, queue, tokens, branches, merge requests, stream, +query, workspaces, billing, misc) over the shared ``_CoreClient`` plumbing +base. It stays a single class exposing every Storage/Queue method at its +original signature, so ``keboola_agent_cli.Client`` and its ``.raw`` accessor +are unaffected by the split of the former single-file ``client.py`` into a +package (issue #520). Inherits shared retry/error logic from BaseHttpClient (via _CoreClient). """ @@ -16,6 +17,7 @@ from .billing import _BillingMixin from .branches import _BranchesMixin from .configs import _ConfigsMixin +from .merge_requests import _MergeRequestsMixin from .misc import _MiscMixin from .query import _QueryMixin from .queue import _QueueMixin @@ -33,6 +35,7 @@ class KeboolaClient( _QueueMixin, _TokensMixin, _BranchesMixin, + _MergeRequestsMixin, _StreamMixin, _QueryMixin, _WorkspacesMixin, diff --git a/src/keboola_agent_cli/client/configs.py b/src/keboola_agent_cli/client/configs.py index 6e8f4889..4e784a77 100644 --- a/src/keboola_agent_cli/client/configs.py +++ b/src/keboola_agent_cli/client/configs.py @@ -624,6 +624,171 @@ def delete_config_row( f"{prefix}/components/{quote(component_id)}/configs/{quote(config_id)}/rows/{quote(row_id)}", ) + def get_config_diff( + self, + component_id: str, + config_id: str, + branch_id: int, + ) -> dict[str, Any]: + """Get the three-way diff of a configuration between branches. + + GET /v2/storage/branch/{branch_id}/components/{c}/configs/{cfg}/diff + + Unlike the other config methods, ``branch_id`` is required with no + production fallback: the endpoint is branch-only and answers 400 on + the default branch, so production is made unrepresentable in the + signature instead of a runtime error. + + Returns: + Diff dict with ``base`` (dev branch v1), ``ours`` (dev head) and + ``theirs`` (default head); each side may be null when the config + does not exist there. Flattening the nested ``diff`` payload is + Layer 2's job. + """ + resp = self._request( + "GET", + f"/v2/storage/branch/{branch_id}/components/" + f"{quote(component_id, safe='')}/configs/{quote(config_id, safe='')}/diff", + ) + return resp.json() + + def _rebase_request( + self, + component_id: str, + config_id: str, + branch_id: int, + version: int, + diff: dict[str, Any], + ) -> dict[str, Any]: + """POST the rebase envelope; shared by the keep and delete rebases. + + The only place that knows the wire shape: ``version`` at the top + level, the resolved content (or ``{}`` for a delete) inside ``diff``. + Body MUST be JSON with real types (``json=``, no ``json.dumps``, no + ``"1"``/``"0"`` booleans), unlike this file's form-encoded idiom -- + the backend validates ``version`` as a real integer and form-encoded + values stay strings and fail validation. + """ + resp = self._request( + "POST", + f"/v2/storage/branch/{branch_id}/components/" + f"{quote(component_id, safe='')}/configs/{quote(config_id, safe='')}/rebase", + json={"version": version, "diff": diff}, + ) + return resp.json() + + def rebase_config( + self, + component_id: str, + config_id: str, + branch_id: int, + version: int, + name: str, + rows: list[dict[str, Any]], + configuration: dict[str, Any], + is_disabled: bool, + description: str | None, + change_description: str | None = None, + ) -> dict[str, Any]: + """Rebase a dev-branch configuration onto a newer default-branch version. + + POST /v2/storage/branch/{branch_id}/components/{c}/configs/{cfg}/rebase + (200 + the rebased configuration -- synchronous, no job). Body is + JSON, not this file's form idiom (see ``_rebase_request``). + + 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. 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. + + ``branch_id`` is required with no production fallback (see + ``get_config_diff``); the endpoint also requires the + ``branches-merge-requests`` feature. + + Args: + component_id: Component ID. + config_id: Configuration ID. + branch_id: Dev branch ID (branch-only endpoint). + version: The DEFAULT-BRANCH version being re-anchored onto + (take it from the diff's ``theirs.version``) -- despite the + wire name, NOT the dev-branch config's version. A target + version that is not newer is a 400. Sent at the top level, + outside ``diff``. + name: Resolved configuration name (non-empty after trimming). + rows: Resolved row objects + (``{id?, name?, description?, isDisabled?, configuration?}``); + missing/null ``id`` means a new row, duplicates are rejected, + array order becomes sort order. + 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`` -- + 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. + + Returns: + The rebased configuration dict. + """ + 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 + return self._rebase_request(component_id, config_id, branch_id, version, diff) + + def rebase_config_delete( + self, + component_id: str, + config_id: str, + branch_id: int, + version: int, + ) -> dict[str, Any]: + """Rebase a dev-branch configuration by resolving it as DELETED. + + POST /v2/storage/branch/{branch_id}/components/{c}/configs/{cfg}/rebase + + Sends exactly ``{"version": N, "diff": {}}`` -- the empty ``diff`` + envelope is how the backend distinguishes a delete resolution from a + keep (``rebase_config``). Body is JSON (see ``_rebase_request``); + ``branch_id`` is required with no production fallback (see + ``get_config_diff``). + + Args: + component_id: Component ID. + config_id: Configuration ID. + branch_id: Dev branch ID (branch-only endpoint). + version: The DEFAULT-BRANCH version being re-anchored onto (from + the diff's ``theirs.version``) -- see ``rebase_config``. + """ + return self._rebase_request(component_id, config_id, branch_id, version, diff={}) + def delete_config( self, component_id: str, config_id: str, branch_id: int | None = None ) -> None: diff --git a/src/keboola_agent_cli/client/merge_requests.py b/src/keboola_agent_cli/client/merge_requests.py new file mode 100644 index 00000000..4cf0fb02 --- /dev/null +++ b/src/keboola_agent_cli/client/merge_requests.py @@ -0,0 +1,334 @@ +"""Merge-request endpoints, exposed as ``client.merge_requests`` (DMD-1701). + +This module holds four pieces: + +- ``StorageRequester``: the FUTURE transport interface -- public method names, + defined today. The client-split RFC (draft PR #595; not in this tree yet) + builds the real transport under this seam later; until then the adapter + below satisfies it. +- ``_ClientRequester``: temporary Adapter delegating to ``_CoreClient``'s + protected methods. Dies the day a real transport object exists. +- ``MergeRequests``: the endpoint-family namespace. It never sees the client, + only the Protocol -- which keeps it unit-testable against a stub requester + and makes the future transport swap a one-line change in the mixin. +- ``_MergeRequestsMixin``: exposes the namespace as a cached property on + ``KeboolaClient``. + +Unlike the flat endpoint-family mixins, new endpoint families are added as +namespaces depending on ``StorageRequester``; existing flat families stay +flat unless deliberately migrated. That is normative intent, not just a +description of this module. + +Every endpoint here is project-level (``isAvailableInBranch: false``), so no +path is ever branch-prefixed -- do not copy the ``branch_id or production`` +prefix idiom from the sibling mixins. And unlike ``configs.py``'s +form-encoded idiom, request bodies MUST be JSON (``json=``, real nested +objects): the backend validators require real types (``branchFromId`` is +``Assert\\Type('int')``), and form-encoded values stay strings and fail +validation. +""" + +import functools +from typing import Any, Protocol + +import httpx + +from ..constants import MERGE_JOB_MAX_WAIT, STORAGE_JOB_MAX_WAIT +from ._core import _CoreClient + +_BASE = "/v2/storage/merge-request" + +# ``MergeRequests.list`` shadows the ``list`` builtin inside the class body +# (annotations there would resolve to the method), so class-scope annotations +# spell list types via these module-level aliases. +_DictList = list[dict[str, Any]] +_IntList = list[int] + + +class StorageRequester(Protocol): + """The FUTURE transport interface -- public method names, defined today. + + Keep this minimal: grow it only when a method needs another transport + capability; every method added is a promise the future transport must + keep (see the client-split RFC, draft PR #595). + """ + + def request(self, method: str, path: str, **kwargs: Any) -> httpx.Response: ... + + def wait_for_storage_job( + self, job: dict[str, Any], max_wait: float = STORAGE_JOB_MAX_WAIT + ) -> dict[str, Any]: + """Poll a Storage job to a terminal state; RAISE if it failed. + + Not a description of the current implementation but a REQUIREMENT on + any future one: a failed job must raise ``STORAGE_JOB_FAILED``, + whether the failure arrives in the ``job`` passed in (the Storage API + can fail fast, never reporting ``waiting``) or in a polled response. + ``merge()`` does not re-check the returned job, so an implementation + that *returns* a failed job turns a failed merge into a silent + success. This blind spot was real house-wide until #603 (DMD-1898) + made the poller check-then-fetch; do not reintroduce it under this + Protocol. + """ + ... + + +class _ClientRequester: + """Temporary Adapter: satisfies the Protocol by delegating to the client. + + Dies the day a real transport object exists. + """ + + def __init__(self, client: _CoreClient) -> None: + self._client = client + + def request(self, method: str, path: str, **kwargs: Any) -> httpx.Response: + return self._client._request(method, path, **kwargs) + + def wait_for_storage_job( + self, job: dict[str, Any], max_wait: float = STORAGE_JOB_MAX_WAIT + ) -> dict[str, Any]: + return self._client._wait_for_storage_job(job, max_wait=max_wait) + + +def _optional_mr_fields( + *, + description: str | None, + reviewer_ids: _IntList | None, + auto_merge_strategy: str | None, + auto_merge_at: str | None, + external_id: str | None, +) -> dict[str, Any]: + """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. 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: + body["description"] = description + if reviewer_ids is not None: + body["reviewerIds"] = reviewer_ids + if auto_merge_strategy is not None: + body["autoMergeStrategy"] = auto_merge_strategy + if auto_merge_at is not None: + body["autoMergeAt"] = auto_merge_at + if external_id is not None: + body["externalId"] = external_id + return body + + +class MergeRequests: + """Merge-request endpoints, exposed as ``client.merge_requests``. + + Non-SOX "Branches 2.0" flow (``branches-merge-requests``): create -> + request review -> approve -> merge. All paths are project-level and never + branch-prefixed; all bodies are JSON (module docstring). Returns are raw + parsed JSON, as everywhere in ``client/``. + + The pre-flight feature check (``has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)``) + is deliberately NOT done here -- a missing feature surfaces as a 403 + byte-for-byte identical to a role denial, so only a Layer 2 pre-flight + can word the error. + """ + + def __init__(self, requester: StorageRequester) -> None: # never sees the client + self._requester = requester + + def list(self) -> _DictList: + """List the project's merge requests. + + GET /v2/storage/merge-request + + The endpoint declares no query parameters, so any state filtering is + necessarily client-side (Layer 2's job). + """ + return self._requester.request("GET", _BASE).json() + + def get(self, merge_request_id: int, include_activity_log: bool = False) -> dict[str, Any]: + """Get a merge request's detail. + + GET /v2/storage/merge-request/{id}[?include=activityLog] + + Args: + merge_request_id: Merge request ID. + include_activity_log: When True, the response embeds the MR's + activity log (``include=activityLog``). + """ + params: dict[str, str] = {} + if include_activity_log: + params["include"] = "activityLog" + return self._requester.request("GET", f"{_BASE}/{merge_request_id}", params=params).json() + + def conflicts(self, merge_request_id: int) -> _DictList: + """List the configurations conflicting between the MR's branches. + + GET /v2/storage/merge-request/{id}/conflicts + """ + return self._requester.request("GET", f"{_BASE}/{merge_request_id}/conflicts").json() + + def create( + self, + branch_from_id: int, + branch_into_id: int, + title: str, + description: str | None = None, + reviewer_ids: _IntList | None = None, + auto_merge_strategy: str | None = None, + auto_merge_at: str | None = None, + external_id: str | None = None, + ) -> dict[str, Any]: + """Create a merge request from a dev branch into the default branch. + + POST /v2/storage/merge-request (201 on success). Only provided + (non-None) optional fields are sent. Body is JSON -- ``branchFromId`` + / ``branchIntoId`` must arrive as JSON numbers, ``reviewerIds`` as an + array of integers; form encoding fails validation. + + The backend rejects a non-default target branch and a source branch + that already has an MR (one MR per source branch, ever) -- both as + 404, not 400. + + Args: + branch_from_id: Source dev branch ID. + branch_into_id: Target (default) branch ID. + title: MR title. + description: Optional MR description. + reviewer_ids: Optional reviewer admin IDs (server de-duplicates). + auto_merge_strategy: ``immediately`` | ``scheduled`` | ``none``. + auto_merge_at: ISO 8601 date-time; required by the backend when + ``auto_merge_strategy`` is ``scheduled``. + external_id: Free-form external reference (max 255 chars), e.g. + a ticket ID. + """ + body: dict[str, Any] = { + "branchFromId": branch_from_id, + "branchIntoId": branch_into_id, + "title": title, + } + body.update( + _optional_mr_fields( + 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() + + def update( + self, + merge_request_id: int, + title: str | None = None, + description: str | None = None, + reviewer_ids: _IntList | None = None, + auto_merge_strategy: str | None = None, + auto_merge_at: str | None = None, + external_id: str | None = None, + ) -> dict[str, Any]: + """Update an existing merge request. + + PUT /v2/storage/merge-request/{id} + + Only provided (non-None) fields are sent, as JSON (module docstring). + See ``create`` for the fields' meaning. ``None`` means "leave + unchanged" -- and that is also all the API can express: server-side, + an explicit JSON null and an absent key are indistinguishable + (``?? null`` mapping + ``!== null`` update guards), so no field can + be cleared to null through this endpoint. Calling with no fields set + PUTs ``{}``, which the backend treats as a no-op returning the MR. + """ + body = _optional_mr_fields( + 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 + return self._requester.request("PUT", f"{_BASE}/{merge_request_id}", json=body).json() + + def request_review(self, merge_request_id: int) -> dict[str, Any]: + """Move the MR from ``development`` to ``in_review``. + + PUT /v2/storage/merge-request/{id}/request-review (no body) + """ + return self._requester.request("PUT", f"{_BASE}/{merge_request_id}/request-review").json() + + def approve(self, merge_request_id: int) -> dict[str, Any]: + """Add the caller's approval to the MR. + + PUT /v2/storage/merge-request/{id}/approve (no body) + """ + return self._requester.request("PUT", f"{_BASE}/{merge_request_id}/approve").json() + + def request_changes(self, merge_request_id: int, reason: str | None = None) -> dict[str, Any]: + """Send the MR back to ``development`` to be revised. + + PUT /v2/storage/merge-request/{id}/request-changes + + Deliberately not named ``reject``: the transition is not terminal, + the MR returns to ``development`` for another round. + + Args: + merge_request_id: Merge request ID. + reason: Optional reason, capped at 1000 characters server-side. + """ + body: dict[str, Any] = {} + if reason is not None: + body["reason"] = reason + return self._requester.request( + "PUT", f"{_BASE}/{merge_request_id}/request-changes", json=body + ).json() + + def merge(self, merge_request_id: int) -> dict[str, Any]: + """Merge an approved MR into the default branch (waits for the job). + + PUT /v2/storage/merge-request/{id}/merge answers 202 with a Storage + job; like every Storage-job method in ``client/`` this polls it to a + terminal state and returns the completed job dict, whose ``results`` + carry the MR including its change log. The wait budget is + ``MERGE_JOB_MAX_WAIT`` (following the ``IMPORT_JOB_MAX_WAIT`` / + ``EXPORT_JOB_MAX_WAIT`` precedent) -- merging a many-config branch + can legitimately outlive the default 60 s storage-job budget. A + failed merge (the MR rolls back to ``approved``) raises + ``STORAGE_JOB_FAILED`` instead of masquerading as success -- for a + fast fail (terminal already in the 202 body) as much as for one that + shows up while polling, because the poller checks before it fetches + (#603); this method deliberately does not re-check, see + ``StorageRequester.wait_for_storage_job``. The MR state stays + pollable via ``get()``. + + Caveat: a successful merge always also deletes the source branch, + but that runs as a second job enqueued by the first, with no job + handle returned -- the await covers the merge outcome only. After a + successful return the changes are in production and the branch is + doomed but may still briefly exist; callers must not assume either + way. + + The merge 409 has four causes in two response shapes (three "not + ready" cases carry ``storage.mergeRequests.notReadyToMerge``, a + conflict does not); mapping them is Layer 2's concern. + """ + response = self._requester.request("PUT", f"{_BASE}/{merge_request_id}/merge") + return self._requester.wait_for_storage_job(response.json(), max_wait=MERGE_JOB_MAX_WAIT) + + +class _MergeRequestsMixin(_CoreClient): + """Exposes the merge-request namespace on ``KeboolaClient``. + + ``cached_property`` rather than the house attr-initialized-to-None lazy + pattern because it needs no ``__init__`` change (the client defines no + ``__slots__``). Neither the namespace nor the adapter owns an HTTP + client, base URL, or token; the client<->namespace reference cycle is + harmless because resources are released by the explicit ``close()``. + """ + + @functools.cached_property + def merge_requests(self) -> MergeRequests: + return MergeRequests(_ClientRequester(self)) diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index fb18e948..e65d562d 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -108,6 +108,7 @@ def _resolve_app_name() -> str: STORAGE_JOB_POLL_INTERVAL: float = 1.0 # seconds between polls STORAGE_JOB_MAX_WAIT: float = 60.0 # max seconds to wait for a storage job IMPORT_JOB_MAX_WAIT: float = 600.0 # 10 min for table import jobs (large files) +MERGE_JOB_MAX_WAIT: float = 600.0 # 10 min for merge-request merge jobs (many-config branches) # --- Queue Job Polling --- # Piecewise curve matching FIIA's existing Queue API polling contract @@ -411,6 +412,16 @@ def _resolve_app_name() -> str: # See plugins/kbagent/skills/kbagent/references/storage-types-workflow.md. STORAGE_BRANCHES_FEATURE: str = "storage-branches" +# --- Merge Requests (Branches 2.0) --- +# Feature flag gating the non-SOX merge-request flow. Layer 3 +# (client/merge_requests.py) does no feature check itself -- a missing +# feature is a 403 identical to a role denial -- so the Part 2 service layer +# must call has_feature() with this constant before writes and word the error. It +# also doubles as the SOX fence: server-side, `protected-default-branch` +# passes the same gate, so checking for this flag specifically keeps SOX +# projects out of a flow whose approvals semantics kbagent does not cover. +FEATURE_BRANCHES_MERGE_REQUESTS: str = "branches-merge-requests" + # --- Global Search --- # Feature flag that gates the Storage API ``GET /v2/storage/global-search`` # endpoint used by ``kbagent search`` (textual mode). Projects without this diff --git a/tests/test_merge_request_client.py b/tests/test_merge_request_client.py new file mode 100644 index 00000000..adb9e877 --- /dev/null +++ b/tests/test_merge_request_client.py @@ -0,0 +1,515 @@ +"""Tests for the merge-request client family (client/merge_requests.py + configs.py). + +Pins the wire contract: path construction (MR paths never branch-prefixed, +diff/rebase always), JSON encoding (the surrounding configs.py teaches form +encoding -- the opposite), presence detection, the ``diff`` envelope, the +empty-object delete resolution, implicit merge-job waiting, +``include=activityLog``, and the namespace-never-touches-the-client seam. +""" + +import inspect +import json +from typing import Any + +import httpx +import pytest + +from keboola_agent_cli.client import KeboolaClient +from keboola_agent_cli.client.merge_requests import MergeRequests, _optional_mr_fields +from keboola_agent_cli.constants import MERGE_JOB_MAX_WAIT, STORAGE_JOB_MAX_WAIT +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError + +STACK_URL = "https://connection.keboola.com" +TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" +MR_BASE = f"{STACK_URL}/v2/storage/merge-request" + +SAMPLE_MR = { + "id": 42, + "title": "Promote revenue pipeline", + "state": "development", + "branches": {"branchFromId": 123, "branchIntoId": 1}, +} + + +@pytest.fixture +def client(): + c = KeboolaClient(stack_url=STACK_URL, token=TOKEN) + yield c + c.close() + + +def _sent_json(request: httpx.Request) -> Any: + """Decode a captured request body, asserting it is JSON-encoded.""" + assert request.headers["Content-Type"] == "application/json" + return json.loads(request.content) + + +class TestMergeRequestPaths: + """MR endpoints are project-level -- never branch-prefixed.""" + + def test_list_hits_bare_path(self, client, httpx_mock) -> None: + """list() hits /v2/storage/merge-request with no branch prefix and no params.""" + httpx_mock.add_response(url=MR_BASE, json=[SAMPLE_MR]) + + result = client.merge_requests.list() + + assert result == [SAMPLE_MR] + request = httpx_mock.get_requests()[0] + assert request.url.path == "/v2/storage/merge-request" + assert request.url.query == b"" + + def test_get_hits_id_path(self, client, httpx_mock) -> None: + """get() hits /merge-request/{id} without include by default.""" + httpx_mock.add_response(url=f"{MR_BASE}/42", json=SAMPLE_MR) + + result = client.merge_requests.get(42) + + assert result == SAMPLE_MR + request = httpx_mock.get_requests()[0] + assert request.url.path == "/v2/storage/merge-request/42" + assert request.url.query == b"" + + def test_get_include_activity_log_only_when_asked(self, client, httpx_mock) -> None: + """include=activityLog is present exactly when include_activity_log=True.""" + httpx_mock.add_response(url=f"{MR_BASE}/42?include=activityLog", json=SAMPLE_MR) + + client.merge_requests.get(42, include_activity_log=True) + + request = httpx_mock.get_requests()[0] + assert request.url.params["include"] == "activityLog" + + def test_conflicts_path(self, client, httpx_mock) -> None: + """conflicts() hits /merge-request/{id}/conflicts.""" + httpx_mock.add_response(url=f"{MR_BASE}/42/conflicts", json=[]) + + result = client.merge_requests.conflicts(42) + + assert result == [] + assert httpx_mock.get_requests()[0].url.path == "/v2/storage/merge-request/42/conflicts" + + +class TestCreateAndUpdateBodies: + """JSON encoding + presence detection on the two body-carrying writes.""" + + def test_create_sends_real_json_types(self, client, httpx_mock) -> None: + """Branch ids go as JSON numbers, reviewerIds as an int array, not strings.""" + httpx_mock.add_response(url=MR_BASE, json=SAMPLE_MR, status_code=201) + + client.merge_requests.create( + branch_from_id=123, + branch_into_id=1, + title="Promote revenue pipeline", + description="Q3 changes", + reviewer_ids=[7, 9], + ) + + body = _sent_json(httpx_mock.get_requests()[0]) + assert body["branchFromId"] == 123 + assert body["branchIntoId"] == 1 + assert isinstance(body["branchFromId"], int) + assert isinstance(body["branchIntoId"], int) + assert body["title"] == "Promote revenue pipeline" + assert body["description"] == "Q3 changes" + assert body["reviewerIds"] == [7, 9] + + def test_create_omits_unset_optionals(self, client, httpx_mock) -> None: + """Unset optionals are absent from the body, not sent as null.""" + httpx_mock.add_response(url=MR_BASE, json=SAMPLE_MR, status_code=201) + + client.merge_requests.create(branch_from_id=123, branch_into_id=1, title="T") + + body = _sent_json(httpx_mock.get_requests()[0]) + assert set(body) == {"branchFromId", "branchIntoId", "title"} + + def test_create_sends_auto_merge_and_external_id(self, client, httpx_mock) -> None: + """autoMergeStrategy / autoMergeAt / externalId pass through verbatim.""" + httpx_mock.add_response(url=MR_BASE, json=SAMPLE_MR, status_code=201) + + client.merge_requests.create( + branch_from_id=123, + branch_into_id=1, + title="T", + auto_merge_strategy="scheduled", + auto_merge_at="2026-09-01T06:00:00+00:00", + external_id="DMD-1701", + ) + + body = _sent_json(httpx_mock.get_requests()[0]) + assert body["autoMergeStrategy"] == "scheduled" + assert body["autoMergeAt"] == "2026-09-01T06:00:00+00:00" + assert body["externalId"] == "DMD-1701" + + def test_update_sends_only_provided_fields(self, client, httpx_mock) -> None: + """update() omits unset fields; provided ones go as real JSON types.""" + httpx_mock.add_response(url=f"{MR_BASE}/42", json=SAMPLE_MR) + + client.merge_requests.update(42, title="New title", reviewer_ids=[7]) + + request = httpx_mock.get_requests()[0] + assert request.method == "PUT" + body = _sent_json(request) + assert body == {"title": "New title", "reviewerIds": [7]} + + def test_update_sends_every_optional_field(self, client, httpx_mock) -> None: + """All optionals reach the wire under their camelCase names (no create/update drift).""" + httpx_mock.add_response(url=f"{MR_BASE}/42", json=SAMPLE_MR) + + client.merge_requests.update( + 42, + title="T", + description="D", + reviewer_ids=[7, 9], + auto_merge_strategy="scheduled", + auto_merge_at="2026-09-01T06:00:00+00:00", + external_id="DMD-1701", + ) + + assert _sent_json(httpx_mock.get_requests()[0]) == { + "title": "T", + "description": "D", + "reviewerIds": [7, 9], + "autoMergeStrategy": "scheduled", + "autoMergeAt": "2026-09-01T06:00:00+00:00", + "externalId": "DMD-1701", + } + + +class TestStateTransitions: + """request-review / approve / request-changes.""" + + def test_request_review(self, client, httpx_mock) -> None: + """request_review() PUTs the request-review path with no body.""" + httpx_mock.add_response(url=f"{MR_BASE}/42/request-review", json=SAMPLE_MR) + + result = client.merge_requests.request_review(42) + + assert result == SAMPLE_MR + request = httpx_mock.get_requests()[0] + assert request.method == "PUT" + assert request.content == b"" + + def test_approve(self, client, httpx_mock) -> None: + """approve() PUTs the approve path with no body.""" + httpx_mock.add_response(url=f"{MR_BASE}/42/approve", json=SAMPLE_MR) + + result = client.merge_requests.approve(42) + + assert result == SAMPLE_MR + assert httpx_mock.get_requests()[0].method == "PUT" + + def test_request_changes_with_reason(self, client, httpx_mock) -> None: + """request_changes() sends {"reason": ...} as JSON when given.""" + httpx_mock.add_response(url=f"{MR_BASE}/42/request-changes", json=SAMPLE_MR) + + client.merge_requests.request_changes(42, reason="Please split the flow") + + body = _sent_json(httpx_mock.get_requests()[0]) + assert body == {"reason": "Please split the flow"} + + def test_request_changes_without_reason_sends_empty_object(self, client, httpx_mock) -> None: + """request_changes() without a reason sends {} (reason omitted, not null).""" + httpx_mock.add_response(url=f"{MR_BASE}/42/request-changes", json=SAMPLE_MR) + + client.merge_requests.request_changes(42) + + assert _sent_json(httpx_mock.get_requests()[0]) == {} + + +class TestMerge: + """merge() awaits the Storage job implicitly, like every job-backed client method.""" + + def test_merge_waits_for_job_and_returns_completed_job( + self, client, httpx_mock, monkeypatch + ) -> None: + """202's job is polled to success; the completed job dict is returned.""" + monkeypatch.setattr("keboola_agent_cli.client._core.time.sleep", lambda _: None) + httpx_mock.add_response( + url=f"{MR_BASE}/42/merge", + json={"id": 555, "status": "waiting"}, + status_code=202, + ) + completed = {"id": 555, "status": "success", "results": SAMPLE_MR} + httpx_mock.add_response(url=f"{STACK_URL}/v2/storage/jobs/555", json=completed) + + result = client.merge_requests.merge(42) + + assert result == completed + assert result["results"] == SAMPLE_MR + merge_request = httpx_mock.get_requests()[0] + assert merge_request.method == "PUT" + assert merge_request.url.path == "/v2/storage/merge-request/42/merge" + + def test_merge_failed_job_raises_storage_job_failed( + self, client, httpx_mock, monkeypatch + ) -> None: + """A failed merge job surfaces STORAGE_JOB_FAILED from the shared helper.""" + monkeypatch.setattr("keboola_agent_cli.client._core.time.sleep", lambda _: None) + httpx_mock.add_response( + url=f"{MR_BASE}/42/merge", + json={"id": 555, "status": "waiting"}, + status_code=202, + ) + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/jobs/555", + json={"id": 555, "status": "error", "error": {"message": "merge conflict"}}, + ) + + with pytest.raises(KeboolaApiError) as exc_info: + client.merge_requests.merge(42) + + assert exc_info.value.error_code == ErrorCode.STORAGE_JOB_FAILED + assert "merge conflict" in str(exc_info.value) + + def test_merge_waits_with_the_merge_budget_not_the_default(self) -> None: + """merge() passes max_wait=MERGE_JOB_MAX_WAIT to the poller. + + The kwarg is the whole point of the 600 s budget: if it is + dropped, every wire test stays green and merge silently falls back to + the 60 s STORAGE_JOB_MAX_WAIT -- a mid-merge STORAGE_JOB_TIMEOUT with + retryable=True on any many-config branch. Asserted through a stub + requester because httpx mocks never see the kwarg. + """ + waits: list[float] = [] + completed = {"id": 555, "status": "success", "results": SAMPLE_MR} + + class StubRequester: + def request(self, method: str, path: str, **kwargs: Any) -> httpx.Response: + return httpx.Response( + 202, + json={"id": 555, "status": "waiting"}, + request=httpx.Request(method, path), + ) + + def wait_for_storage_job( + self, job: dict[str, Any], max_wait: float = STORAGE_JOB_MAX_WAIT + ) -> dict[str, Any]: + waits.append(max_wait) + return completed + + result = MergeRequests(StubRequester()).merge(42) + + assert result == completed + assert waits == [MERGE_JOB_MAX_WAIT] + assert MERGE_JOB_MAX_WAIT > STORAGE_JOB_MAX_WAIT, ( + "the dedicated budget must actually exceed the default it exists to replace" + ) + + +class TestConfigDiff: + """get_config_diff -- branch-prefixed, branch_id required (no production fallback).""" + + def test_diff_path_is_branch_prefixed(self, client, httpx_mock) -> None: + """diff always hits /v2/storage/branch/{id}/... -- no production fallback.""" + diff = {"base": None, "ours": {"version": 3}, "theirs": {"version": 7}} + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/branch/123/components/keboola.ex-http/configs/cfg-1/diff", + json=diff, + ) + + result = client.get_config_diff("keboola.ex-http", "cfg-1", branch_id=123) + + assert result == diff + + def test_diff_quotes_component_and_config_ids(self, client, httpx_mock) -> None: + """Component/config ids are percent-encoded like everywhere in configs.py.""" + httpx_mock.add_response( + url=f"{STACK_URL}/v2/storage/branch/123/components/vendor%2Fapp/configs/c%2F1/diff", + json={}, + ) + + client.get_config_diff("vendor/app", "c/1", branch_id=123) + + assert len(httpx_mock.get_requests()) == 1 + + +class TestRebaseEnvelope: + """The diff envelope -- version at the top level, content inside diff.""" + + REBASE_URL = ( + f"{STACK_URL}/v2/storage/branch/123/components/keboola.ex-http/configs/cfg-1/rebase" + ) + + def test_keep_rebase_builds_the_envelope(self, client, httpx_mock) -> None: + """version stays top-level; name/rows/optionals live inside diff; JSON types.""" + httpx_mock.add_response(url=self.REBASE_URL, json={"id": "cfg-1", "version": 8}) + + client.rebase_config( + "keboola.ex-http", + "cfg-1", + branch_id=123, + version=7, + 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", + ) + + body = _sent_json(httpx_mock.get_requests()[0]) + assert body["version"] == 7 + assert isinstance(body["version"], int) + assert set(body) == {"version", "diff"}, "nothing content-like at the top level" + diff = body["diff"] + assert diff["name"] == "My config" + assert diff["rows"] == [{"id": "r1", "name": "Row 1"}] + assert diff["configuration"] == {"parameters": {"baseUrl": "https://example.com"}} + assert isinstance(diff["configuration"], dict), "real nested JSON, not a dumped string" + assert diff["description"] == "resolved" + assert diff["changeDescription"] == "rebase onto v7" + assert diff["isDisabled"] is False, "is_disabled=False is sent, as a JSON boolean" + + def test_keep_rebase_omits_unset_optionals_and_sends_empty_rows( + self, client, httpx_mock + ) -> None: + """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=[], + configuration={}, + is_disabled=False, + description=None, + ) + + body = _sent_json(httpx_mock.get_requests()[0]) + 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.""" + httpx_mock.add_response(url=self.REBASE_URL, json={}) + + client.rebase_config_delete("keboola.ex-http", "cfg-1", branch_id=123, version=7) + + # Dict equality distinguishes {} from null / "" / [] -- any of those + # would be a malformed-diff 400 (or a delete misread as keep) server-side. + assert _sent_json(httpx_mock.get_requests()[0]) == {"version": 7, "diff": {}} + + +class TestRequesterSeam: + """The namespace depends on the StorageRequester Protocol, not the client.""" + + def test_namespace_works_against_a_stub_requester(self) -> None: + """A stub Protocol implementation is all MergeRequests needs -- no HTTP client.""" + calls: list[tuple[str, str]] = [] + + class StubRequester: + def request(self, method: str, path: str, **kwargs: Any) -> httpx.Response: + calls.append((method, path)) + return httpx.Response(200, json=[SAMPLE_MR], request=httpx.Request(method, path)) + + # Present only to satisfy the Protocol -- this test exercises + # list(). Returning the job unconditionally would violate the + # documented raise-on-failure contract; not a reference + # implementation to copy. + def wait_for_storage_job( + self, job: dict[str, Any], max_wait: float = 60.0 + ) -> dict[str, Any]: + return job + + namespace = MergeRequests(StubRequester()) + + 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) + try: + assert client.merge_requests is client.merge_requests + finally: + client.close()