From a01ee36c59742ecbec8b9bb08580153bd1b95851 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 19 Aug 2026 13:17:34 +0200 Subject: [PATCH] feat(client): merge-request endpoints -- Layer 3 (DMD-1701) client/merge_requests.py -- the nine MR endpoints as a namespace, client.merge_requests.{list,get,conflicts,create,update,request_review, approve,request_changes,merge}. The namespace depends on a StorageRequester Protocol, not on the client; a temporary _ClientRequester adapter satisfies it until the client-split work (draft #595) builds a real transport under the seam. Two invariants deliberately break the surrounding idioms and are called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint is project-level), and bodies are JSON with real types (the backend asserts branchFromId as int; form-encoded values stay strings and fail validation). _optional_mr_fields keeps create/update from drifting and is keyword-only: four of its five parameters are str | None, so a positional transposition would type-check cleanly and surface only as a backend 422. merge() awaits the Storage job implicitly like every job-backed method in client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a many-config branch can outlive the default 60 s. It does NOT re-check the returned job: raising on a failed job (fast-fail included) is the poller's contract since #603, stated as a requirement on the Protocol so a future transport cannot reintroduce the blind spot. The await covers the merge outcome only; the source-branch deletion runs as a second, unhandled job. client/configs.py -- get_config_diff + rebase_config/rebase_config_delete. branch_id is required with no production fallback (the endpoints 400 on the default branch). Keep and delete rebases are separate methods so no illegal combination is expressible. The keep rebase requires the FULL replaced body (name, rows, configuration, is_disabled, description): /rebase replaces rather than patches, so an omitted key takes the server-side default -- a caller sending only name+rows would wipe the configuration and re-enable a disabled config, then merge that into production. constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS. Layer 3 does no feature check itself (a missing feature is a 403 identical to a role denial); Part 2's service pre-flights with the constant. tests/test_merge_request_client.py pins the wire contract: bare vs branch-prefixed paths, JSON types, presence detection, the diff envelope and the {} delete resolution, merge-job waiting and its 600 s budget, the replaced-body requirement, include=activityLog, and the stub-requester seam. Part 2 (service + commands) follows separately; no CLI command is added here, so no E2E / docs surfaces change yet. Co-Authored-By: Claude Fable 5 --- src/keboola_agent_cli/client/_client.py | 13 +- src/keboola_agent_cli/client/configs.py | 165 ++++++ .../client/merge_requests.py | 334 ++++++++++++ src/keboola_agent_cli/constants.py | 11 + tests/test_merge_request_client.py | 515 ++++++++++++++++++ 5 files changed, 1033 insertions(+), 5 deletions(-) create mode 100644 src/keboola_agent_cli/client/merge_requests.py create mode 100644 tests/test_merge_request_client.py 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()