diff --git a/CHANGELOG.md b/CHANGELOG.md index db55384..8c295ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- Every command except `tfscli auth login`, when no API version is configured, now first asks the server with an `OPTIONS` request which version its resource supports and sends that version, so `wit work-items get-batch` and `wit wiql query-by-wiql` no longer fail with "No api-version was supplied" on servers that require one. +- A request sent without an API version because the server did not answer that question is now noted under `--verbose`, and if the server refuses it for the missing version, the `config` error names `apiVersion`, `TFSCLI_API_VERSION`, and `--api-version`. + ## [0.1.0] - 2026-10-06 ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index 658b0f6..31d6b0a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. - **Stable error contract.** Error categories (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`) are a contract: existing categories are never removed or renamed. New ones may be added. Format: `Error [category]: message (HTTP status)` to stderr, non-zero exit. - **Config precedence:** CLI flag > env var (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) > config file (optional) > built-in default. The credential is outside this chain (see above). -- **Target is on-prem TFS / Azure DevOps Server; no `api-version` by default.** Requests carry `api-version` only when one is configured, and `auth login` never sends one. Cloud Azure DevOps Services may work incidentally but is not tested or supported. +- **Target is on-prem TFS / Azure DevOps Server; the API version is negotiated.** Without a configured version, each request is preceded by `OPTIONS {url}/{collection}/_apis` and carries the version the server lists for its resource; when the negotiation fails, the request goes out without a version, as before. A configured version is sent as is. `auth login` never sends one. Cloud Azure DevOps Services may work incidentally but is not tested or supported. - **Do not narrow server compatibility.** A change must not stop tfscli from working with a TFS / Azure DevOps Server version it worked with before, unless that is an explicitly discussed and recorded decision (see "Server compatibility is preserved" in `project-brief.md`). - **Works out of the box; errors lead the way.** Assume the user has read nothing and configured nothing. Do not add a required setting where a working default exists, and do not guess where the answer is ambiguous — ask the user instead. Every error a first-time user can hit names the concrete next step, and says when only a person can take it (see "Works out of the box; errors lead the way" in `project-brief.md`). - **Minimal dependencies.** Total external deps are intentionally three: `cobra`, `html-to-markdown/v2`, and `golang.org/x/term` (hidden PAT input for `auth login`). HTTP via `net/http`, JSON via `encoding/json`, config via `encoding/json`. Don't pull in `viper`, `resty`, retry libraries, or YAML/TOML parsers without a strong reason and explicit discussion. diff --git a/README.md b/README.md index c265ca3..85fd932 100644 --- a/README.md +++ b/README.md @@ -159,7 +159,7 @@ Every setting apart from the credential: | Setting | Config key | Environment variable | Flag | Default | Required | |---|---|---|---|---|---| | Team project | `project` | `TFSCLI_PROJECT` | `-p`, `--project` | — | per command | -| REST API version | `apiVersion` | `TFSCLI_API_VERSION` | `--api-version` | none | no | +| REST API version | `apiVersion` | `TFSCLI_API_VERSION` | `--api-version` | negotiated with the server | no | | Request logging | — | `TFSCLI_VERBOSE=1` | `--verbose` | off | no | | CA bundle | `caBundle` | — | — | none | no | | Skip certificate verification | `insecureSkipVerify` | — | — | `false` | no | @@ -183,9 +183,11 @@ The config file is optional: every setting it holds, except the TLS ones, can be ### REST API version -By default no `api-version` is sent, and the server answers at the version it chooses. The commands that send a GET request, `wit work-items get` and `wit work-items list`, need no setting to work with any server release. +Without a configured version, tfscli asks the server for one. Each command first sends `OPTIONS` to `{url}/{collection}/_apis`, which lists every resource of the server with its versions, and the request then carries the released version of its resource. That is the version the server would choose for a request without one, so no setting is needed on any server release. The extra request shows under `--verbose`. -`wit work-items get-batch` and `wit wiql query-by-wiql` send a POST request, which a server may refuse without a version: it then reports a `config` error saying that no `api-version` was supplied, and a version has to be set with any of the three settings above. Otherwise set a version only to pin the shape of the response. `auth login` does not use this setting. +If the server does not answer `OPTIONS` with that list, the request is sent without a version, and `--verbose` says why. A server may refuse a POST request without a version — `wit work-items get-batch` and `wit wiql query-by-wiql` send one — and the `config` error then names the settings above. A token the server does not accept, or a server that cannot be reached, is reported at once. + +A configured version is sent unchanged, without `OPTIONS`. Set one only for a server that refuses requests without a version, or to pin the shape of the response. `auth login` does not use this setting. The version must not exceed the highest one the server supports: @@ -317,11 +319,18 @@ An empty result reads `Work items: none` or `Relations: none`. The time of the r ### Request logging -`--verbose` writes one line per request to stderr — method, URL, status, duration — leaving stdout clean for the markdown. Headers and bodies are never logged, so the PAT cannot leak through it. A failed round trip is logged with status `0`. +`--verbose` writes one line per request to stderr — method, URL, status, duration — leaving stdout clean for the markdown. Headers and bodies are never logged, so the PAT cannot leak through it. A failed round trip is logged with status `0`. The `OPTIONS` request that negotiates the API version is logged like any other: ``` $ tfscli wit work-items get -p MyProject 12345 --verbose -GET https://tfs.company.com:8080/tfs/DefaultCollection/MyProject/_apis/wit/workitems/12345 200 86.4512ms +OPTIONS https://tfs.company.com:8080/tfs/DefaultCollection/_apis 200 41.2087ms +GET https://tfs.company.com:8080/tfs/DefaultCollection/MyProject/_apis/wit/workitems/12345?api-version=7.1 200 86.4512ms +``` + +Lines starting with `[warn]` report a condition worth knowing, such as disabled certificate verification; lines starting with `[info]` report what tfscli did on its own, such as sending a request without a version: + +``` +[info] api-version not negotiated (OPTIONS returned HTTP 405); sending the request without it ``` ## Errors @@ -340,7 +349,7 @@ The `(HTTP status)` part is omitted for errors that did not come from an HTTP re | `forbidden` | Authenticated, but access was denied (HTTP 403). | | `not_found` | The work item, project, or team does not exist (HTTP 404). During `auth login`, the server URL or the collection leads nowhere. | | `server` | TFS failed or returned an unparseable response (HTTP 5xx). | -| `config` | Missing or invalid configuration, a bad argument, or a request TFS rejected. When TFS refuses a configured API version, the message names the setting it came from. | +| `config` | Missing or invalid configuration, a bad argument, or a request TFS rejected. When TFS refuses a configured API version, the message names the setting it came from; when it refuses a request for carrying no version, after the version could not be negotiated, the message names the settings that supply one. | | `network` | The server could not be reached, or the request timed out or was canceled. | Examples: diff --git a/docs/2026-10-04-tasks-first-run-errors.md b/docs/2026-10-04-tasks-first-run-errors.md index 0defd80..50ce972 100644 --- a/docs/2026-10-04-tasks-first-run-errors.md +++ b/docs/2026-10-04-tasks-first-run-errors.md @@ -109,25 +109,3 @@ optional for `wit work-items get`, taking server compatibility into account. the Context section, and implementation tasks for the decision are added to this file. **Status:** Pending - -### TASK-08 `missing-api-version` -**Description:** A server may refuse a POST request that carries no -`api-version` — `wit work-items get-batch` and `wit wiql query-by-wiql` -send one only when it is configured — with "No api-version was supplied for -the \"POST\" request". `classify` in `internal/apiclient/apiclient.go` adds -a next step to a version error only when a version is configured, so this -refusal reaches the user as the server's message alone, without the settings -that supply a version or how to choose its value. Add the next step for this -case: name `apiVersion` in the config file with the path of the file, -`TFSCLI_API_VERSION`, and `--api-version`, and say that the value must not -exceed the highest version the server supports, as listed in the -"Configuration" section of `README.md`. How the refusal is recognised is -unverified: the `typeKey` of this error is not in `versionErrorTypeKeys` -and has to be established from documentation or a sample response, falling -back to the message text only if no `typeKey` is available. -**Definition of done:** A refusal of a request that carries no `api-version` -produces a message naming the three settings, the config file path, and how -to choose the value; a refusal of a configured version keeps its current -message. Tests cover both. `README.md` and the representative messages in -`skills/tfscli/SKILL.md` match. `make check` passes. -**Status:** Pending diff --git a/docs/architecture.md b/docs/architecture.md index 37cbb21..502aed5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -147,8 +147,9 @@ This section describes the conceptual module structure, their responsibilities, ▼ ┌──────────────────┐ ┌─────────────────────┐ │ apiclient │◄─────►│ TFS REST API │ - │ net/http + PAT │ TLS │ (no default ver.) │ - │ + TLS + logger │ └─────────────────────┘ + │ net/http + PAT │ TLS │ (version: config, │ + │ + TLS + logger │ │ else OPTIONS) │ + │ │ └─────────────────────┘ └────────┬─────────┘ │ raw JSON / *tfserr.Error ▼ @@ -174,26 +175,29 @@ This section describes the conceptual module structure, their responsibilities, - **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. -- **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path`, adding `api-version` only when `APIVersion` is set; without it the server answers at the version it chooses. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). +- **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. - **wiql** — domain logic for Wiql: Query By Wiql. Owns the `Request` and `Result` types. Builds the path at collection level, under a project, or under a project and a team; passes the query text through unchanged and sends `$top` and `timePrecision` only when they are given. Reads the query type, `asOf`, the reference names of the columns, and either the work item ids of a flat query or the relations of a link query, keeping the server's order. Does not read the work items themselves: that is a separate call through `workitem`, made by the caller. - **htmlmd** — thin wrapper over `github.com/JohannesKaufmann/html-to-markdown/v2`. One method: `Convert(html string) (string, error)`. v1 uses library defaults only; the wrapper is the extension point where TFS-specific rules will be registered once real HTML samples have been collected (see the `html-quirks` entry in `docs/backlog.md`). - **tfserr** — typed error `Error{Category, Message, HTTPStatus, Cause}` with the stable category set from the brief. Provides `Print(err, w)` writing `Error [category]: message (HTTP status)` and `ExitCode(err)` mapping to a non-zero process exit code (v1: 1 for every category; the API leaves room for per-category exit codes later without breaking the contract). -- **log** — `Logger` interface with two implementations: `noop` (default) and `stderr` (selected by `--verbose` / `TFSCLI_VERBOSE=1`). Methods: `LogRequest(method, url, status, dur)` and `Warn(msg)`. The logger interface accepts only these four request fields — PAT and the `Authorization` header are never passed in, so they cannot leak through the logger by construction. +- **log** — `Logger` interface with two implementations: `noop` (default) and `stderr` (selected by `--verbose` / `TFSCLI_VERBOSE=1`). Methods: `LogRequest(method, url, status, dur)`, `Warn(msg)` for a condition worth reporting, and `Info(msg)` for what tfscli did on its own, such as sending a request without a version after a failed negotiation. The logger interface accepts only these four request fields — PAT and the `Authorization` header are never passed in, so they cannot leak through the logger by construction. ### Key dependency interfaces These are the boundaries between modules. Final signatures may evolve during Step 4. ```go +// location is the id under which the server lists the resource of the +// request in its response to OPTIONS on _apis; empty for no negotiation. type APIClient interface { - Get(ctx context.Context, path string, query url.Values) ([]byte, error) - Post(ctx context.Context, path string, query url.Values, body any) ([]byte, error) + Get(ctx context.Context, location, path string, query url.Values) ([]byte, error) + Post(ctx context.Context, location, path string, query url.Values, body any) ([]byte, error) } type Logger interface { LogRequest(method, url string, status int, dur time.Duration) Warn(msg string) + Info(msg string) } type Converter interface { @@ -271,8 +275,8 @@ type Result struct { 2. `config.Load` takes `URL`, `Collection`, and `PAT` from `TFSCLI_AUTH` or `auth.json`, which must carry all three; reads the config file if present, overlays env vars, overlays bound flags. The command then checks that `Project` is supplied either by `-p`, env, or config default. 3. cli builds a `Logger` (noop or stderr depending on `--verbose`) and an `apiclient.APIClient`. The TLS config is derived from `CABundle` and `InsecureSkipVerify`. If verification is disabled, the logger emits one `[warn] TLS verification disabled` line. 4. cli calls `workitem.Get(ctx, client, project, id, fields)`. -5. `workitem.Get` constructs the path `{Project}/_apis/wit/workitems/{id}` with the `fields` query parameter and calls `client.Get`, which prefixes the collection and adds `api-version={APIVersion}` only when a version is configured. -6. `apiclient.Get` builds the request, sets the `Authorization` header, and executes it through its `RoundTripper`, which calls `LogRequest` once with method, URL, status, and duration. On non-2xx the response is classified into a `*tfserr.Error`; otherwise the response body is returned. +5. `workitem.Get` constructs the path `{Project}/_apis/wit/workitems/{id}` with the `fields` query parameter and calls `client.Get` with the location id of Work Items, `72c7ddf8-2cdc-4f60-90cd-ab71c14a399b`. +6. Without a configured version, `apiclient.Get` first sends `OPTIONS {Collection}/_apis` and takes the version listed for that location; with one, it uses the configured version. It then builds the request with `api-version`, sets the `Authorization` header, and executes it through its `RoundTripper`, which calls `LogRequest` once with method, URL, status, and duration. On non-2xx the response is classified into a `*tfserr.Error`; otherwise the response body is returned. 7. `workitem.Get` unmarshals the JSON, walks `fields`, sets each `Field.Kind` using the HTML allowlist plus known identity/datetime field types, and returns `*WorkItem`. 8. The cli printer iterates `wi.Fields`. HTML fields go through `htmlmd.Convert`. Identity and datetime fields are formatted by small dedicated helpers. Plain fields print as is. Output is written to `stdout`. 9. On error at any step: `tfserr.Print(err, os.Stderr)`; `os.Exit(tfserr.ExitCode(err))`. diff --git a/docs/archive/2026-10-06-tasks-api-version-negotiation.md b/docs/archive/2026-10-06-tasks-api-version-negotiation.md new file mode 100644 index 0000000..44c4f18 --- /dev/null +++ b/docs/archive/2026-10-06-tasks-api-version-negotiation.md @@ -0,0 +1,172 @@ +# 2026-10-06-tasks-api-version-negotiation.md + +**Status:** Archived + +## Context + +Goal: every command works on any server without the user setting an API +version. Today `wit work-items get-batch` and `wit wiql query-by-wiql` fail +with "No api-version was supplied" on servers that require a version for +POST, and neither the user nor an agent can be expected to know the right +value. + +Microsoft documents `api-version` as required on every request; in practice +GET requests work without it, while POST requests are refused by some +servers. The official SDKs (.NET, Go, Node, Python) discover versions with +`OPTIONS {url}/{collection}/_apis`, which lists every resource with its +`minVersion`, `maxVersion`, `releasedVersion`, and `resourceVersion` +(`getResourceLocationsFromServer` in `azuredevops/client.go` and +`negotiateRequestVersion` in `azuredevops/versionnegotiation.go` of +`microsoft/azure-devops-go-api`; `client.py` of +`microsoft/azure-devops-python-api`). + +Decisions already made: + +- When no version is configured, every request except the verification in + `auth login` is preceded by `OPTIONS {url}/{collection}/_apis` and carries + the `releasedVersion` of its resource — the version the server chooses for a + request without one, so the output does not change. A resource without a + released version (`0.0`) gets `{maxVersion}-preview.{resourceVersion}`. +- The response is not cached: an on-disk cache is excluded by the product + constraints, and an in-process cache gains nothing in a CLI. Each invocation + makes one extra request. +- A configured version (config file, `TFSCLI_API_VERSION`, `--api-version`) is + sent as is, without `OPTIONS`. +- Negotiation never fails a command on its own. A 401 or a transport error on + `OPTIONS` is reported at once in its usual category, since the main request + would fail the same way. Any other failure — 400, 403, 404, 405, 5xx, an + unparseable response, the resource missing from the list — falls back to + sending the request without a version, as before, so that no server that + works today stops working. +- Under `--verbose` the `OPTIONS` request is logged like any other, and a + fallback adds one line saying that the version was not negotiated and why. + Nothing is printed without `--verbose`. +- When a request sent without a version after a fallback is refused for lack + of one, the `config` error says that negotiation failed and why, names + `apiVersion` with the config file path, `TFSCLI_API_VERSION`, and + `--api-version`, and says that the value must not exceed the server's + version. This replaces TASK-08 of + `docs/2026-10-04-tasks-first-run-errors.md`. +- No new error category. +- The constraint "requests carry `api-version` only when one is configured" in + `docs/project-brief.md` and `CLAUDE.md`, and the "REST API version" section + of `README.md`, change together with this work. + +### Findings (TASK-01) + +Sources: `microsoft/azure-devops-go-api`, branch `dev`, files +`azuredevops/v7/client.go` [G1], `azuredevops/v7/models.go` [G2], +`azuredevops/v7/connection.go` [G3], `azuredevops/v7/versionnegotiation.go` +[G4], `azuredevops/v7/workitemtracking/client.go` [G5]; the error response +quoted in https://dexterposh.github.io/posts/002-azdo-tip-api/ [D1]. + +- **Request.** `getResourceLocationsFromServer` sends `OPTIONS` to the client's + base URL joined with `_apis`, with `Accept: application/json` and no + `api-version` [G1]. For an on-premises server the base URL is the + collection URL: `GetClientByResourceAreaId` falls back to the connection + URL because "resourceAreaInfo will be nil for on prem servers" [G3]. tfscli + therefore sends `OPTIONS {url}/{collection}/_apis`. +- **Response.** A collection wrapper `{"count": n, "value": [...]}`, read with + `UnmarshalCollectionBody` [G1]. Each element is an `ApiResourceLocation` + [G2]: `id` (GUID), `area`, `resourceName`, `routeTemplate`, `minVersion`, + `maxVersion`, `releasedVersion` (strings such as `"7.1"`), and + `resourceVersion` (an integer). `releasedVersion` is "the latest version of + this resource location that is in Release (non-preview) mode"; the + negotiation treats `0.0` as "not released" [G4]. +- **Location ids** [G5]: Get Work Item and Work Items - List share + `72c7ddf8-2cdc-4f60-90cd-ab71c14a399b`; Get Work Items Batch is + `908509b6-4248-4475-a1cd-829139ba419f`; Query By Wiql, with and without a + team, is `1a9c53f7-f243-4447-b110-35ef023636e4`. +- **Matching: by location id.** The SDK indexes the response by `id` and + fails with `LocationIdNotRegisteredError` when the id is absent [G1]. The + id is what the server publishes as the identity of a location and is the + same in every release. Matching by area and resource name is rejected: the + pair is not documented as unique, and the work items resource already has + two locations — `72c7ddf8-…` and `62d3d110-0047-428c-ad3c-4fe872c91c74`, + used for Create Work Item and Get Work Item Template [G5] — whose resource + names have not been checked; an ambiguous match would pick a version for + the wrong route. +- **Where the version goes.** The SDK sends it in the `Accept` header + (`application/json;api-version=…`) [G1]; the server error [D1] accepts it + "either as part of the Accept header … or as a query parameter". tfscli + keeps the query parameter, so that the version stays visible in the URL + logged under `--verbose`. +- **Refusal without a version** [D1]: `typeKey` `VssVersionNotSpecifiedException`, + message `No api-version was supplied for the "POST" request. …`. The + report is from Azure DevOps Services; that an on-premises server returns + the same `typeKey` is unverified. +- **TFS 2015 and 2017.** No source was found that shows the `OPTIONS` + response of these releases. Unverified; the fallback to a request without + a version covers a server that does not answer it. + +--- + +### TASK-01 `options-research` +**Description:** From the SDK sources and Microsoft documentation, establish: +the shape of the `OPTIONS {url}/{collection}/_apis` response and the fields +of an `ApiResourceLocation`; the location ids of the resources tfscli calls — +Work Items (Get Work Item, Work Items - List), Work Items Batch, and Wiql; +whether a request is matched to its entry by location id, as the SDKs do, or +by area and resource name, and the reasons; what is known about `OPTIONS` on +TFS 2015 and 2017; and the `typeKey` of the error a server returns for a +request that carries no `api-version`. +**Definition of done:** The findings are recorded in the Context section, +each with its source; anything not confirmed is marked as unverified; the +matching method is decided and recorded with the rejected alternative. +**Status:** Done + +### TASK-02 `version-selection` +**Description:** In `internal/apiclient`, parse the `OPTIONS` response and add +a pure function that chooses the version for a resource from its entry: the +`releasedVersion`, or `{maxVersion}-preview.{resourceVersion}` when the +released version is `0.0`. A response that cannot be parsed, and a resource +missing from it, are reported to the caller as distinct outcomes, so that the +fallback can say why it happened. +**Definition of done:** Unit tests cover a released resource, a +preview-only resource, a resource missing from the list, and an unparseable +response. `make check` passes. +**Status:** Done + +### TASK-03 `client-negotiation` +**Description:** Use the version selection in `Client`. Callers name the +resource of each request, which changes `Get` and `Post` and the `APIClient` +interfaces in `internal/workitem` and `internal/wiql`. A configured version +is sent without `OPTIONS`. A 401 or a transport error on `OPTIONS` is +returned as the error of the command; any other failure falls back to a +request without a version and logs one line under `--verbose` saying why, +through a `Logger` method suited to it rather than `Warn`. The verification +in `auth login` (`verify` in `internal/cli/auth.go`) makes no `OPTIONS` +request. +**Definition of done:** Tests against a stub server cover: a negotiated +version on GET and on POST; a configured version sent without `OPTIONS`; a +401 and a transport failure on `OPTIONS`; each fallback case with its +`--verbose` line; and `auth login` sending no `OPTIONS`. `make check` passes. +**Status:** Done + +### TASK-04 `missing-version-error` +**Description:** When a request sent without a version after a fallback is +refused for lack of one, recognise the refusal by the `typeKey` established +in TASK-01, falling back to the message text only if no `typeKey` is +available, and add the next step to the `config` error: that the version +could not be negotiated and why; `apiVersion` in the config file with the +path of the file, `TFSCLI_API_VERSION`, and `--api-version`; and that the +value must not exceed the highest version the server supports, as listed in +the "REST API version" section of `README.md`. +**Definition of done:** Tests cover the refusal after a fallback; a refusal +of a configured version keeps its current message. `make check` passes. +**Status:** Done + +### TASK-05 `docs` +**Description:** Bring the documents in line with the new behaviour: the +constraint on `api-version` in `docs/project-brief.md` (Constraints, Design +Principles, Configuration) and `CLAUDE.md`; the description of +`internal/apiclient` and the data flow in `docs/architecture.md`; the "REST +API version" and "Request logging" sections of `README.md`, and the +`config` row of its error table; `skills/tfscli/SKILL.md` wherever it speaks +of the API version; and an entry in `CHANGELOG.md` per the `changelog` +skill. +**Definition of done:** No document describes requests as carrying +`api-version` only when one is configured; the README describes the +`OPTIONS` request and the fallback; the changelog entry is in place. `make +check` passes. +**Status:** Done diff --git a/docs/backlog.md b/docs/backlog.md index 032946f..107f9c1 100644 --- a/docs/backlog.md +++ b/docs/backlog.md @@ -2,52 +2,6 @@ Accepted work that has not started yet, in priority order. -### `api-version-negotiation` -**Goal:** Every command works on any server without the user setting an API -version: `wit work-items get-batch` and `wit wiql query-by-wiql` no longer fail -with "No api-version was supplied" on servers that require a version for POST. -**Context:** Microsoft documents `api-version` as required on every request; in -practice GET requests work without it, while POST requests are refused by some -servers. The official SDKs (.NET, Go, Node, Python) discover versions with -`OPTIONS {url}/{collection}/_apis`, which lists every resource with its -`minVersion`, `maxVersion`, `releasedVersion`, and `resourceVersion`. Decisions -already made: -- When no version is configured, every request except the verification in - `auth login` is preceded by `OPTIONS {url}/{collection}/_apis` and carries the - `releasedVersion` of its resource — the version the server chooses for a - request without one, so the output does not change. A resource without a - released version (`0.0`) gets `{maxVersion}-preview.{resourceVersion}`. -- The response is not cached: an on-disk cache is excluded by the product - constraints, and an in-process cache gains nothing in a CLI. Each invocation - makes one extra request. -- A configured version (config file, `TFSCLI_API_VERSION`, `--api-version`) is - sent as is, without `OPTIONS`. -- Negotiation never fails a command on its own. A 401 or a transport error on - `OPTIONS` is reported at once in its usual category, since the main request - would fail the same way. Any other failure — 400, 403, 404, 405, 5xx, an - unparseable response, the resource missing from the list — falls back to - sending the request without a version, as before, so that no server that works - today stops working. -- Under `--verbose` the `OPTIONS` request is logged like any other, and a - fallback adds one line saying that the version was not negotiated and why. - Nothing is printed without `--verbose`. -- When a request sent without a version after a fallback is refused for lack of - one, the `config` error says that negotiation failed and why, names - `apiVersion` with the config file path, `TFSCLI_API_VERSION`, and - `--api-version`, and says that the value must not exceed the server's version. - This supersedes TASK-08 in `docs/2026-10-04-tasks-first-run-errors.md`, which - moves into this work when it starts. -- No new error category. -- The constraint "requests carry `api-version` only when one is configured" in - `docs/project-brief.md` and `CLAUDE.md`, and the "REST API version" section of - `README.md`, change together with this work. - -Open questions: -- Whether `OPTIONS` answers in this form on TFS 2015 and 2017 (unverified). -- How a request is matched to its entry in the list: by location id, as the - SDKs do, or by area and resource name. -**Added:** 2026-10-06, in conversation on API version negotiation - ### `workitem-relations` **Goal:** An agent reading a work item sees the work items linked to it — parent, children, related, duplicates — without composing a WIQL link query. diff --git a/docs/project-brief.md b/docs/project-brief.md index 2286228..ccdb95f 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -32,7 +32,7 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - **Platform:** primary target is Windows (most on-prem TFS environments). Cross-compilation to Linux/macOS is a bonus enabled by Go, not a priority. - **API coverage in MVP:** Work Items → Get Work Item (single ID); Work Items - List and Get Work Items Batch (multiple IDs; the latter, a POST, needs Azure DevOps Server 2019 or later). Fields selectable via `--fields` parameter; all fields returned by default. Wiql → Query By Wiql (find work items with a WIQL query, available since TFS 2015); it returns work item IDs and links only, which are then read with Work Items - List. -- **Target API version:** none by default. Requests carry no `api-version`, so the server answers at the version it chooses, and the same configuration works with older TFS installations and with the latest Azure DevOps Server alike. A version can be pinned in the config file, the environment, or per call, and is then sent unchanged. A server may refuse a POST request, such as Get Work Items Batch or Query By Wiql, that carries no version; tfscli does not supply one on its own, since the API permits the request without it, and reports the server's refusal as it is, in the `config` category. +- **Target API version:** negotiated with the server, not configured. Microsoft documents `api-version` as required on every request, and some servers refuse a POST request without it, such as Get Work Items Batch or Query By Wiql. Without a configured version, each request is therefore preceded by `OPTIONS {url}/{collection}/_apis`, which lists every resource of the server with its versions, as the official SDKs do, and carries the released version of its resource: the version the server chooses for a request without one, so the same configuration works with older TFS installations and with the latest Azure DevOps Server alike. The answer is not kept between calls, since tfscli is stateless. When the negotiation fails for any reason other than a refused token or an unreachable server, the request is sent without a version, so that no server tfscli worked with stops working; if the server then refuses it for the missing version, the `config` error names the settings that supply one. A version can be pinned in the config file, the environment, or per call, and is then sent unchanged, without negotiation. - **Authentication:** PAT only. No SSPI or NTLM. A PAT is issued for one collection, so the collection is part of the credential. The server URL, the collection, and the PAT are stored together and always come from the same source, so the token is never sent to any other server or collection: either `$XDG_DATA_HOME/tfscli/auth.json` (default `~/.local/share/tfscli/auth.json`, on every OS), written only by `tfscli auth login`, or the `TFSCLI_AUTH` environment variable holding the same JSON, which takes precedence and serves CI. `auth.json` holds exactly one credential, is written with mode `0600` (directory `0700`), and stores the token in plaintext. No flag, separate environment variable, or config key supplies the URL, the collection, or the token. See "Authentication" below. - **No OS keychain.** Against code running as the same user a keychain adds no protection; other users are excluded by file permissions, and stolen disks by disk encryption. A keychain is reconsidered only if an organisation requires secrets to be kept in the system store, and would then be a second storage backend behind the same `auth login`. - **Output:** Markdown by default. JSON as optional flag in future versions. @@ -84,7 +84,7 @@ File: `$XDG_CONFIG_HOME/tfscli/config.json`, defaulting to `~/.config/tfscli/con } ``` -All values overridable via environment variables (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) and command-line flags (`-p`, `--api-version`). The API version is set with `apiVersion` in the config file, and has no default: without it no `api-version` is sent. The TLS settings `caBundle` and `insecureSkipVerify` are accepted in the config file only. +All values overridable via environment variables (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) and command-line flags (`-p`, `--api-version`). The API version is set with `apiVersion` in the config file, and has no default: without it the version is negotiated with the server (see "Target API version"). The TLS settings `caBundle` and `insecureSkipVerify` are accepted in the config file only. Priority: CLI flag > environment variable > config file > built-in default. diff --git a/internal/apiclient/apiclient.go b/internal/apiclient/apiclient.go index 5b1e3c7..d2b1bfc 100644 --- a/internal/apiclient/apiclient.go +++ b/internal/apiclient/apiclient.go @@ -41,6 +41,16 @@ var versionErrorTypeKeys = map[string]bool{ "VssVersionOutOfRangeException": true, } +// missingVersionTypeKey is the typeKey of the error refusing a request that +// carries no API version, as Azure DevOps Services reports it; that an +// on-premises server uses the same one is assumed, not observed. +// missingVersionMessage, the start of its English message, identifies the +// error from a server that sends no typeKey. +const ( + missingVersionTypeKey = "VssVersionNotSpecifiedException" + missingVersionMessage = "No api-version was supplied" +) + // Client performs TFS REST API calls. It owns the PAT and is the only module // that sets the Authorization header. type Client struct { @@ -53,6 +63,7 @@ type Client struct { // apiVersionSource names the setting apiVersion came from; see // config.Config.APIVersionSource. apiVersionSource string + logger log.Logger } // New builds a Client from cfg. TLS is configured from CABundle (appended to @@ -93,24 +104,27 @@ func New(cfg *config.Config, logger log.Logger) (*Client, error) { pat: cfg.PAT, apiVersion: cfg.APIVersion, apiVersionSource: cfg.APIVersionSource, + logger: logger, }, nil } // Get performs a GET request against path, which is appended to the base URL // (server URL plus collection). The path is taken as already escaped, so a // caller escapes each user-supplied segment, such as a project name, with -// url.PathEscape. The api-version parameter is added here when -// a version is configured, so callers pass only their own query parameters; -// without one the server answers at the version it chooses. On a non-2xx +// url.PathEscape. Callers pass only their own query parameters: api-version +// is added here (see version). location is the id under which the server +// lists the resource of the request among its API versions; an empty +// location sends the request without negotiating a version. On a non-2xx // response or a transport failure the error is a *tfserr.Error carrying the // matching category. -func (c *Client) Get(ctx context.Context, path string, query url.Values) ([]byte, error) { - return c.do(ctx, http.MethodGet, path, query, nil) +func (c *Client) Get(ctx context.Context, location, path string, query url.Values) ([]byte, error) { + return c.do(ctx, http.MethodGet, location, path, query, nil) } // Post performs a POST request against path with body encoded as JSON. The -// path, the query, api-version, and the errors are handled as in Get. -func (c *Client) Post(ctx context.Context, path string, query url.Values, body any) ([]byte, error) { +// location, the path, the query, api-version, and the errors are handled as +// in Get. +func (c *Client) Post(ctx context.Context, location, path string, query url.Values, body any) ([]byte, error) { payload, err := json.Marshal(body) if err != nil { return nil, &tfserr.Error{ @@ -119,29 +133,80 @@ func (c *Client) Post(ctx context.Context, path string, query url.Values, body a Cause: err, } } - return c.do(ctx, http.MethodPost, path, query, payload) + return c.do(ctx, http.MethodPost, location, path, query, payload) } // do sends one request and returns the body of a 2xx response. A nil payload // sends no body; a non-nil one is sent as JSON. -func (c *Client) do(ctx context.Context, method, path string, query url.Values, payload []byte) ([]byte, error) { +func (c *Client) do(ctx context.Context, method, location, path string, query url.Values, payload []byte) ([]byte, error) { + version, fallback, err := c.version(ctx, location) + if err != nil { + return nil, err + } + u := c.base.JoinPath(path) q := maps.Clone(query) if q == nil { q = url.Values{} } - if c.apiVersion != "" { - q.Set("api-version", c.apiVersion) + if version != "" { + q.Set("api-version", version) } u.RawQuery = q.Encode() + status, body, err := c.send(ctx, method, u, payload) + if err != nil { + return nil, err + } + if status < 200 || status > 299 { + return nil, c.classify(status, body, fallback) + } + return body, nil +} + +// version returns the api-version for a request to location. A configured +// version is used as it is. Otherwise the version is negotiated with an +// OPTIONS request to _apis, which lists every resource location with its +// versions, as the official SDKs do; see chooseVersion. A 401 or a transport +// failure is returned as the error, since the request itself would fail the +// same way. Any other failure leaves the version empty, so that the request +// goes out without one, as it did before negotiation existed; fallback then +// says why, and the same is noted under --verbose. +func (c *Client) version(ctx context.Context, location string) (version, fallback string, err error) { + if c.apiVersion != "" || location == "" { + return c.apiVersion, "", nil + } + + status, body, err := c.send(ctx, http.MethodOptions, c.base.JoinPath("_apis"), nil) + if err != nil { + return "", "", err + } + switch { + case status == http.StatusUnauthorized: + return "", "", c.classify(status, body, "") + case status < 200 || status > 299: + fallback = fmt.Sprintf("OPTIONS returned HTTP %d", status) + default: + version, fallback = chooseVersion(body, location) + } + if fallback != "" { + c.logger.Info("api-version not negotiated (" + fallback + "); sending the request without it") + } + return version, fallback, nil +} + +// send makes one request to u and returns the status and the body of the +// response, whatever the status. A nil payload sends no body; a non-nil one +// is sent as JSON. The error is a *tfserr.Error for a request that could not +// be built, a transport failure, or a body that could not be read. +func (c *Client) send(ctx context.Context, method string, u *url.URL, payload []byte) (int, []byte, error) { var reqBody io.Reader if payload != nil { reqBody = bytes.NewReader(payload) } req, err := http.NewRequestWithContext(ctx, method, u.String(), reqBody) if err != nil { - return nil, &tfserr.Error{ + return 0, nil, &tfserr.Error{ Category: tfserr.Config, Message: fmt.Sprintf("cannot build a request for %s", u.Redacted()), Cause: err, @@ -157,7 +222,7 @@ func (c *Client) do(ctx context.Context, method, path string, query url.Values, resp, err := c.http.Do(req) if err != nil { - return nil, transportError(u, err) + return 0, nil, transportError(u, err) } // The body is read in full below; a failure to close it afterwards says // nothing about the result and has nowhere useful to go. @@ -165,18 +230,14 @@ func (c *Client) do(ctx context.Context, method, path string, query url.Values, body, err := io.ReadAll(resp.Body) if err != nil { - return nil, &tfserr.Error{ + return 0, nil, &tfserr.Error{ Category: tfserr.Server, Message: "cannot read the response from TFS", HTTPStatus: resp.StatusCode, Cause: err, } } - - if resp.StatusCode < 200 || resp.StatusCode > 299 { - return nil, c.classify(resp.StatusCode, body) - } - return body, nil + return resp.StatusCode, body, nil } // loggingTransport logs one line per request and is the single call site of @@ -273,17 +334,22 @@ func transportError(u *url.URL, err error) error { // classify maps a non-2xx response to a stable error category, preferring the // message from the TFS error JSON over the generic wording when the body // carries one. When TFS refused a version the user configured, the message -// goes on to name the setting and what to do with it. -func (c *Client) classify(status int, body []byte) error { +// goes on to name the setting and what to do with it. When it refused a +// request for carrying no version, sent so because negotiation failed for the +// reason in fallback, the message says that and where a version is set. +func (c *Client) classify(status int, body []byte, fallback string) error { category, generic := categoryFor(status) message := generic payload := parseServerError(body) if payload.message != "" { message = payload.message } - if versionErrorTypeKeys[payload.typeKey] && c.apiVersion != "" { + switch { + case versionErrorTypeKeys[payload.typeKey] && c.apiVersion != "": message += fmt.Sprintf(" (api-version %q is set by %s; remove it to let the server choose the version, or set one the server supports)", c.apiVersion, c.apiVersionSource) + case fallback != "" && payload.versionMissing(): + message += " (" + missingVersionStep(fallback) + ")" } return &tfserr.Error{ Category: category, @@ -325,6 +391,29 @@ type serverError struct { typeKey string } +// versionMissing reports whether e refuses a request for carrying no API +// version. The typeKey decides when there is one, since the message depends +// on the language the server is installed in. +func (e serverError) versionMissing() bool { + if e.typeKey != "" { + return e.typeKey == missingVersionTypeKey + } + return strings.HasPrefix(e.message, missingVersionMessage) +} + +// missingVersionStep is the next step for a request refused for carrying no +// API version after negotiation failed for reason: where to set a version and +// how to choose its value. +func missingVersionStep(reason string) string { + file := "the config file" + if path, err := config.DefaultPath(); err == nil { + file = path + } + return "the API version could not be negotiated: " + reason + + `; set one with "apiVersion" in ` + file + ", TFSCLI_API_VERSION, or --api-version, " + + `no higher than the server supports (see "REST API version" in the tfscli README)` +} + // parseServerError extracts the "message" and "typeKey" fields of a TFS // error response. A body that is not TFS error JSON — an HTML error page from // a proxy, say — yields empty fields so that the generic wording is used diff --git a/internal/apiclient/apiclient_test.go b/internal/apiclient/apiclient_test.go index 695e786..d24ddf6 100644 --- a/internal/apiclient/apiclient_test.go +++ b/internal/apiclient/apiclient_test.go @@ -11,6 +11,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "testing" "time" @@ -33,6 +34,7 @@ type requestEntry struct { type recordingLogger struct { requests []requestEntry warnings []string + infos []string } func (l *recordingLogger) LogRequest(method, url string, status int, dur time.Duration) { @@ -41,6 +43,8 @@ func (l *recordingLogger) LogRequest(method, url string, status int, dur time.Du func (l *recordingLogger) Warn(msg string) { l.warnings = append(l.warnings, msg) } +func (l *recordingLogger) Info(msg string) { l.infos = append(l.infos, msg) } + var _ log.Logger = (*recordingLogger)(nil) // newTestClient builds a Client pointed at serverURL, with the recording @@ -79,7 +83,7 @@ func TestGetSetsAuthorizationHeader(t *testing.T) { t.Cleanup(srv.Close) client, _ := newTestClient(t, srv.URL) - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil); err != nil { + if _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil); err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -102,7 +106,7 @@ func TestGetBuildsURL(t *testing.T) { client, _ := newTestClient(t, srv.URL) query := url.Values{"fields": {"System.Title,System.State"}} - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", query); err != nil { + if _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", query); err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -132,7 +136,7 @@ func TestGetEscapesCollectionAndKeepsEscapedPath(t *testing.T) { if err != nil { t.Fatalf("New() error = %v, want nil", err) } - if _, err := client.Get(t.Context(), "100%25%20Done/_apis/wit/workitems/123", nil); err != nil { + if _, err := client.Get(t.Context(), testLocation, "100%25%20Done/_apis/wit/workitems/123", nil); err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -141,9 +145,11 @@ func TestGetEscapesCollectionAndKeepsEscapedPath(t *testing.T) { } } -func TestGetWithoutAPIVersion(t *testing.T) { +func TestGetWithoutLocationSendsNoVersion(t *testing.T) { + var methods []string var gotQuery url.Values srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + methods = append(methods, r.Method) gotQuery = r.URL.Query() })) t.Cleanup(srv.Close) @@ -156,10 +162,13 @@ func TestGetWithoutAPIVersion(t *testing.T) { if err != nil { t.Fatalf("New() error = %v, want nil", err) } - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil); err != nil { + if _, err := client.Get(t.Context(), "", "_apis/connectionData", nil); err != nil { t.Fatalf("Get() error = %v, want nil", err) } + if want := []string{http.MethodGet}; !slices.Equal(methods, want) { + t.Errorf("requests = %v, want %v — no OPTIONS without a location", methods, want) + } if _, ok := gotQuery["api-version"]; ok { t.Errorf("query = %v, want no api-version", gotQuery) } @@ -171,7 +180,7 @@ func TestGetDoesNotMutateCallerQuery(t *testing.T) { client, _ := newTestClient(t, srv.URL) query := url.Values{"fields": {"System.Title"}} - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", query); err != nil { + if _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", query); err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -188,7 +197,7 @@ func TestGetReturnsBody(t *testing.T) { t.Cleanup(srv.Close) client, _ := newTestClient(t, srv.URL) - got, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + got, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) if err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -217,7 +226,7 @@ func TestGetClassifiesResponseStatus(t *testing.T) { srv := errorServer(t, tt.status, "") client, _ := newTestClient(t, srv.URL) - _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tt.want) if te.HTTPStatus != tt.status { t.Errorf("HTTPStatus = %d, want %d", te.HTTPStatus, tt.status) @@ -234,7 +243,7 @@ func TestGetPrefersServerMessage(t *testing.T) { srv := errorServer(t, http.StatusNotFound, body) client, _ := newTestClient(t, srv.URL) - _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/99999", nil) + _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/99999", nil) te := assertTFSError(t, err, tfserr.NotFound) if want := "TF401232: Work item 99999 does not exist."; te.Message != want { t.Errorf("message = %q, want %q", te.Message, want) @@ -256,7 +265,7 @@ func TestGetFallsBackToGenericMessage(t *testing.T) { srv := errorServer(t, http.StatusForbidden, tt.body) client, _ := newTestClient(t, srv.URL) - _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Forbidden) if want := "access denied by TFS"; te.Message != want { t.Errorf("message = %q, want %q", te.Message, want) @@ -282,7 +291,7 @@ func TestGetNamesTheSourceOfARefusedVersion(t *testing.T) { t.Fatalf("New() error = %v, want nil", err) } - _, err = client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err = client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Config) want := outOfRangeMessage + ` (api-version "7.2" is set by TFSCLI_API_VERSION; ` + "remove it to let the server choose the version, or set one the server supports)" @@ -335,7 +344,7 @@ func TestGetLeavesOtherRejectionsAlone(t *testing.T) { t.Fatalf("New() error = %v, want nil", err) } - _, err = client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err = client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Config) if te.Message != tt.want { t.Errorf("message = %q, want %q", te.Message, tt.want) @@ -349,7 +358,7 @@ func TestGetCollapsesAndBoundsServerMessage(t *testing.T) { srv := errorServer(t, http.StatusInternalServerError, body) client, _ := newTestClient(t, srv.URL) - _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Server) if strings.ContainsAny(te.Message, "\n\t") { @@ -369,7 +378,7 @@ func TestGetUnreachableServer(t *testing.T) { srv.Close() // nothing listens on that port any more client, logger := newTestClient(t, serverURL) - _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil) + _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Network) if want := "cannot reach " + serverURL; te.Message != want { @@ -394,7 +403,7 @@ func TestGetCanceledContext(t *testing.T) { cancel() client, _ := newTestClient(t, srv.URL) - _, err := client.Get(ctx, "/MyProject/_apis/wit/workitems/123", nil) + _, err := client.Get(ctx, testLocation, "/MyProject/_apis/wit/workitems/123", nil) te := assertTFSError(t, err, tfserr.Network) if !strings.Contains(te.Message, "canceled") { @@ -418,7 +427,7 @@ func TestLogsEveryRequestExactlyOnce(t *testing.T) { client, logger := newTestClient(t, srv.URL) for range 2 { - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil); err != nil { + if _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil); err != nil { t.Fatalf("Get() error = %v, want nil", err) } } @@ -605,7 +614,7 @@ func TestPostSendsJSONBody(t *testing.T) { IDs []int `json:"ids"` Fields []string `json:"fields,omitempty"` }{IDs: []int{297, 299}} - got, err := client.Post(t.Context(), "/MyProject/_apis/wit/workitemsbatch", nil, body) + got, err := client.Post(t.Context(), testLocation, "/MyProject/_apis/wit/workitemsbatch", nil, body) if err != nil { t.Fatalf("Post() error = %v, want nil", err) } @@ -635,7 +644,7 @@ func TestPostCarriesQueryAndAPIVersion(t *testing.T) { client, _ := newTestClient(t, srv.URL) query := url.Values{"$top": {"50"}} - if _, err := client.Post(t.Context(), "/MyProject/_apis/wit/wiql", query, map[string]string{}); err != nil { + if _, err := client.Post(t.Context(), testLocation, "/MyProject/_apis/wit/wiql", query, map[string]string{}); err != nil { t.Fatalf("Post() error = %v, want nil", err) } @@ -650,31 +659,11 @@ func TestPostCarriesQueryAndAPIVersion(t *testing.T) { } } -func TestPostWithoutAPIVersion(t *testing.T) { - srv, rec := recordingServer(t, "") - client, err := New(&config.Config{ - URL: srv.URL, - Collection: "DefaultCollection", - PAT: testPAT, - }, &recordingLogger{}) - if err != nil { - t.Fatalf("New() error = %v, want nil", err) - } - - if _, err := client.Post(t.Context(), "/MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}); err != nil { - t.Fatalf("Post() error = %v, want nil", err) - } - - if _, ok := rec.query["api-version"]; ok { - t.Errorf("query = %v, want no api-version", rec.query) - } -} - func TestGetSendsNoContentType(t *testing.T) { srv, rec := recordingServer(t, "") client, _ := newTestClient(t, srv.URL) - if _, err := client.Get(t.Context(), "/MyProject/_apis/wit/workitems/123", nil); err != nil { + if _, err := client.Get(t.Context(), testLocation, "/MyProject/_apis/wit/workitems/123", nil); err != nil { t.Fatalf("Get() error = %v, want nil", err) } @@ -688,7 +677,7 @@ func TestPostClassifiesResponseStatus(t *testing.T) { srv := errorServer(t, http.StatusNotFound, body) client, _ := newTestClient(t, srv.URL) - _, err := client.Post(t.Context(), "/MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}) + _, err := client.Post(t.Context(), testLocation, "/MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}) te := assertTFSError(t, err, tfserr.NotFound) if te.HTTPStatus != http.StatusNotFound { t.Errorf("HTTPStatus = %d, want %d", te.HTTPStatus, http.StatusNotFound) @@ -702,7 +691,7 @@ func TestPostRejectsUnencodableBody(t *testing.T) { srv, rec := recordingServer(t, "") client, _ := newTestClient(t, srv.URL) - _, err := client.Post(t.Context(), "/MyProject/_apis/wit/workitemsbatch", nil, make(chan int)) + _, err := client.Post(t.Context(), testLocation, "/MyProject/_apis/wit/workitemsbatch", nil, make(chan int)) _ = assertTFSError(t, err, tfserr.Config) if rec.method != "" { diff --git a/internal/apiclient/negotiate.go b/internal/apiclient/negotiate.go new file mode 100644 index 0000000..e57f239 --- /dev/null +++ b/internal/apiclient/negotiate.go @@ -0,0 +1,63 @@ +package apiclient + +import ( + "bytes" + "encoding/json" + "strconv" + "strings" +) + +// resourceLocation is the part of an ApiResourceLocation, one element of the +// response to OPTIONS on _apis, that version selection reads. +type resourceLocation struct { + ID string `json:"id"` + // MaxVersion is the highest version the server supports for the + // location, such as "7.1". + MaxVersion string `json:"maxVersion"` + // ReleasedVersion is the latest version that is not a preview; "0.0" + // when the location has only preview versions. + ReleasedVersion string `json:"releasedVersion"` + // ResourceVersion is the revision of the preview at MaxVersion. + ResourceVersion int `json:"resourceVersion"` +} + +// chooseVersion picks the api-version for location from body, the response +// to OPTIONS on _apis: the released version of the location, or, when it has +// none, the latest preview. That is the version the server itself chooses for +// a request without one, so sending it does not change the response. On +// failure version is empty and reason says why, in words fit for a log line +// or an error message. +func chooseVersion(body []byte, location string) (version, reason string) { + var payload struct { + Value []resourceLocation `json:"value"` + } + // Some servers prefix JSON with a byte order mark. + body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf")) + if err := json.Unmarshal(body, &payload); err != nil || payload.Value == nil { + return "", "the server's list of API versions could not be read" + } + + for _, loc := range payload.Value { + if !strings.EqualFold(loc.ID, location) { + continue + } + if isReleased(loc.ReleasedVersion) { + return loc.ReleasedVersion, "" + } + if loc.MaxVersion == "" { + return "", "the server lists no version for this resource" + } + version = loc.MaxVersion + "-preview" + if loc.ResourceVersion > 0 { + version += "." + strconv.Itoa(loc.ResourceVersion) + } + return version, "" + } + return "", "the server does not list this resource among its API versions" +} + +// isReleased reports whether v names a released version. The server writes +// "0.0" for a location that has only preview versions. +func isReleased(v string) bool { + return v != "" && strings.Trim(v, "0.") != "" +} diff --git a/internal/apiclient/negotiate_test.go b/internal/apiclient/negotiate_test.go new file mode 100644 index 0000000..d736be9 --- /dev/null +++ b/internal/apiclient/negotiate_test.go @@ -0,0 +1,410 @@ +package apiclient + +import ( + "net/http" + "net/http/httptest" + "net/url" + "path/filepath" + "slices" + "testing" + + "github.com/dpleshakov/tfscli/internal/config" + "github.com/dpleshakov/tfscli/internal/tfserr" +) + +const ( + testLocation = "908509b6-4248-4475-a1cd-829139ba419f" + + // optionsBody is a response to OPTIONS on _apis listing a released + // location, the one under test, and a preview-only one. + optionsBody = `{"count":3,"value":[ + {"id":"72c7ddf8-2cdc-4f60-90cd-ab71c14a399b","area":"wit","resourceName":"workItems","minVersion":"1.0","maxVersion":"7.1","releasedVersion":"7.1","resourceVersion":3}, + {"id":"908509B6-4248-4475-A1CD-829139BA419F","area":"wit","resourceName":"workItemsBatch","minVersion":"5.0","maxVersion":"7.1","releasedVersion":"7.0","resourceVersion":1}, + {"id":"1a9c53f7-f243-4447-b110-35ef023636e4","area":"wit","resourceName":"wiql","minVersion":"1.0","maxVersion":"7.1","releasedVersion":"0.0","resourceVersion":2} + ]}` +) + +func TestChooseVersion(t *testing.T) { + tests := []struct { + name string + body string + location string + want string + }{ + { + name: "released location, id in another case", + body: optionsBody, + location: testLocation, + want: "7.0", + }, + { + name: "preview-only location", + body: optionsBody, + location: "1a9c53f7-f243-4447-b110-35ef023636e4", + want: "7.1-preview.2", + }, + { + name: "preview-only location without a resource version", + body: `{"count":1,"value":[{"id":"` + testLocation + `","maxVersion":"5.0","releasedVersion":"0.0"}]}`, + location: testLocation, + want: "5.0-preview", + }, + { + name: "missing released version", + body: `{"count":1,"value":[{"id":"` + testLocation + `","maxVersion":"5.0","resourceVersion":1}]}`, + location: testLocation, + want: "5.0-preview.1", + }, + { + name: "byte order mark", + body: "\xef\xbb\xbf" + optionsBody, + location: testLocation, + want: "7.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, reason := chooseVersion([]byte(tt.body), tt.location) + if got != tt.want || reason != "" { + t.Errorf("chooseVersion() = %q, %q; want %q, \"\"", got, reason, tt.want) + } + }) + } +} + +func TestChooseVersionFailures(t *testing.T) { + tests := []struct { + name string + body string + reason string + }{ + { + name: "location not listed", + body: `{"count":1,"value":[{"id":"72c7ddf8-2cdc-4f60-90cd-ab71c14a399b","maxVersion":"7.1","releasedVersion":"7.1"}]}`, + reason: "the server does not list this resource among its API versions", + }, + { + name: "no version for the location", + body: `{"count":1,"value":[{"id":"` + testLocation + `","releasedVersion":"0.0"}]}`, + reason: "the server lists no version for this resource", + }, + { + name: "html page", + body: "Sign in", + reason: "the server's list of API versions could not be read", + }, + { + name: "json without a value array", + body: `{"message":"something else"}`, + reason: "the server's list of API versions could not be read", + }, + { + name: "empty body", + body: "", + reason: "the server's list of API versions could not be read", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, reason := chooseVersion([]byte(tt.body), testLocation) + if got != "" || reason != tt.reason { + t.Errorf("chooseVersion() = %q, %q; want \"\", %q", got, reason, tt.reason) + } + }) + } +} + +func TestIsReleased(t *testing.T) { + tests := map[string]bool{ + "": false, + "0.0": false, + "0": false, + "1.0": true, + "7.1": true, + "0.1": true, + } + for v, want := range tests { + if got := isReleased(v); got != want { + t.Errorf("isReleased(%q) = %v, want %v", v, got, want) + } + } +} + +// negotiation is what a negotiationServer saw. +type negotiation struct { + // requests lists each request as method and path, in order. + requests []string + // query is the query of the last request other than OPTIONS. + query url.Values +} + +// negotiationServer answers OPTIONS with optionsStatus and optionsReply, and +// every other request with an empty JSON object. +func negotiationServer(t *testing.T, optionsStatus int, optionsReply string) (*httptest.Server, *negotiation) { + t.Helper() + rec := &negotiation{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rec.requests = append(rec.requests, r.Method+" "+r.URL.Path) + if r.Method == http.MethodOptions { + w.WriteHeader(optionsStatus) + _, _ = w.Write([]byte(optionsReply)) + return + } + rec.query = r.URL.Query() + _, _ = w.Write([]byte("{}")) + })) + t.Cleanup(srv.Close) + return srv, rec +} + +// newUnversionedClient builds a Client with no API version configured, so +// that it negotiates one. +func newUnversionedClient(t *testing.T, serverURL string) (*Client, *recordingLogger) { + t.Helper() + logger := &recordingLogger{} + client, err := New(&config.Config{ + URL: serverURL, + Collection: "DefaultCollection", + PAT: testPAT, + }, logger) + if err != nil { + t.Fatalf("New() error = %v, want nil", err) + } + return client, logger +} + +func TestNegotiatesVersion(t *testing.T) { + tests := []struct { + name string + send func(*Client) error + want []string + }{ + { + name: "GET", + send: func(c *Client) error { + _, err := c.Get(t.Context(), testLocation, "MyProject/_apis/wit/workitems", nil) + return err + }, + want: []string{"OPTIONS /DefaultCollection/_apis", "GET /DefaultCollection/MyProject/_apis/wit/workitems"}, + }, + { + name: "POST", + send: func(c *Client) error { + _, err := c.Post(t.Context(), testLocation, "MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}) + return err + }, + want: []string{"OPTIONS /DefaultCollection/_apis", "POST /DefaultCollection/MyProject/_apis/wit/workitemsbatch"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv, rec := negotiationServer(t, http.StatusOK, optionsBody) + client, logger := newUnversionedClient(t, srv.URL) + + if err := tt.send(client); err != nil { + t.Fatalf("request error = %v, want nil", err) + } + + if !slices.Equal(rec.requests, tt.want) { + t.Errorf("requests = %v, want %v", rec.requests, tt.want) + } + if got := rec.query.Get("api-version"); got != "7.0" { + t.Errorf("api-version = %q, want the negotiated %q", got, "7.0") + } + if len(logger.requests) != 2 { + t.Errorf("logged %d requests, want 2 — OPTIONS is logged like any other", len(logger.requests)) + } + if len(logger.infos) != 0 { + t.Errorf("infos = %v, want none after a successful negotiation", logger.infos) + } + }) + } +} + +func TestConfiguredVersionSkipsNegotiation(t *testing.T) { + srv, rec := negotiationServer(t, http.StatusOK, optionsBody) + client, _ := newTestClient(t, srv.URL) + + if _, err := client.Post(t.Context(), testLocation, "MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}); err != nil { + t.Fatalf("Post() error = %v, want nil", err) + } + + if want := []string{"POST /DefaultCollection/MyProject/_apis/wit/workitemsbatch"}; !slices.Equal(rec.requests, want) { + t.Errorf("requests = %v, want %v", rec.requests, want) + } + if got := rec.query.Get("api-version"); got != "7.2" { + t.Errorf("api-version = %q, want the configured %q", got, "7.2") + } +} + +func TestNegotiationFailsOnUnauthorized(t *testing.T) { + srv, rec := negotiationServer(t, http.StatusUnauthorized, "") + client, _ := newUnversionedClient(t, srv.URL) + + _, err := client.Get(t.Context(), testLocation, "MyProject/_apis/wit/workitems", nil) + + te := assertTFSError(t, err, tfserr.Auth) + if te.HTTPStatus != http.StatusUnauthorized { + t.Errorf("HTTPStatus = %d, want %d", te.HTTPStatus, http.StatusUnauthorized) + } + if want := []string{"OPTIONS /DefaultCollection/_apis"}; !slices.Equal(rec.requests, want) { + t.Errorf("requests = %v, want %v — the request itself is not sent", rec.requests, want) + } +} + +func TestNegotiationFailsOnTransportError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + serverURL := srv.URL + srv.Close() + client, logger := newUnversionedClient(t, serverURL) + + _, err := client.Get(t.Context(), testLocation, "MyProject/_apis/wit/workitems", nil) + + _ = assertTFSError(t, err, tfserr.Network) + if len(logger.requests) != 1 || logger.requests[0].method != http.MethodOptions { + t.Errorf("logged requests = %v, want the OPTIONS request alone", logger.requests) + } +} + +func TestNegotiationFallsBack(t *testing.T) { + tests := []struct { + name string + status int + reply string + info string + }{ + {"bad request", http.StatusBadRequest, "", "api-version not negotiated (OPTIONS returned HTTP 400); sending the request without it"}, + {"forbidden", http.StatusForbidden, "", "api-version not negotiated (OPTIONS returned HTTP 403); sending the request without it"}, + {"not found", http.StatusNotFound, "", "api-version not negotiated (OPTIONS returned HTTP 404); sending the request without it"}, + {"method not allowed", http.StatusMethodNotAllowed, "", "api-version not negotiated (OPTIONS returned HTTP 405); sending the request without it"}, + {"server error", http.StatusInternalServerError, "", "api-version not negotiated (OPTIONS returned HTTP 500); sending the request without it"}, + {"unparseable", http.StatusOK, "Sign in", "api-version not negotiated (the server's list of API versions could not be read); sending the request without it"}, + {"location missing", http.StatusOK, `{"count":0,"value":[]}`, "api-version not negotiated (the server does not list this resource among its API versions); sending the request without it"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv, rec := negotiationServer(t, tt.status, tt.reply) + client, logger := newUnversionedClient(t, srv.URL) + + if _, err := client.Post(t.Context(), testLocation, "MyProject/_apis/wit/workitemsbatch", nil, map[string]string{}); err != nil { + t.Fatalf("Post() error = %v, want nil", err) + } + + if len(rec.requests) != 2 { + t.Fatalf("requests = %v, want OPTIONS and then the request", rec.requests) + } + if _, ok := rec.query["api-version"]; ok { + t.Errorf("query = %v, want no api-version after a fallback", rec.query) + } + if want := []string{tt.info}; !slices.Equal(logger.infos, want) { + t.Errorf("infos = %q, want %q", logger.infos, want) + } + }) + } +} + +const missingVersionReply = `{"$id":"1","message":"No api-version was supplied for the \"POST\" request.","typeKey":"VssVersionNotSpecifiedException"}` + +// refusingServer answers OPTIONS with optionsStatus and every other request +// with HTTP 400 and reply. +func refusingServer(t *testing.T, optionsStatus int, reply string) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodOptions { + w.WriteHeader(optionsStatus) + return + } + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(reply)) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestMissingVersionAfterFallbackNamesTheSettings(t *testing.T) { + cfgHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", cfgHome) + cfgPath := filepath.Join(cfgHome, "tfscli", "config.json") + step := ` (the API version could not be negotiated: OPTIONS returned HTTP 405; ` + + `set one with "apiVersion" in ` + cfgPath + `, TFSCLI_API_VERSION, or --api-version, ` + + `no higher than the server supports (see "REST API version" in the tfscli README))` + + tests := []struct { + name string + reply string + want string + }{ + { + name: "typeKey", + reply: missingVersionReply, + want: `No api-version was supplied for the "POST" request.` + step, + }, + { + name: "message without a typeKey", + reply: `{"message":"No api-version was supplied for the \"POST\" request."}`, + want: `No api-version was supplied for the "POST" request.` + step, + }, + { + name: "another refusal", + reply: `{"message":"TF51005: The query references a field that does not exist.","typeKey":"QueryException"}`, + want: "TF51005: The query references a field that does not exist.", + }, + { + name: "message of another error without a typeKey", + reply: `{"message":"Some other problem."}`, + want: "Some other problem.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := refusingServer(t, http.StatusMethodNotAllowed, tt.reply) + client, _ := newUnversionedClient(t, srv.URL) + + _, err := client.Post(t.Context(), testLocation, "_apis/wit/wiql", nil, map[string]string{}) + + te := assertTFSError(t, err, tfserr.Config) + if te.Message != tt.want { + t.Errorf("message = %q, want %q", te.Message, tt.want) + } + }) + } +} + +func TestMissingVersionWithoutFallbackKeepsTheMessage(t *testing.T) { + tests := []struct { + name string + location string + }{ + // No location: the request was meant to go without a version. + {"no location", ""}, + // A wrong typeKey on a negotiated request is not a missing version. + {"negotiated", testLocation}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodOptions { + _, _ = w.Write([]byte(optionsBody)) + return + } + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(missingVersionReply)) + })) + t.Cleanup(srv.Close) + client, _ := newUnversionedClient(t, srv.URL) + + _, err := client.Post(t.Context(), tt.location, "_apis/wit/wiql", nil, map[string]string{}) + + te := assertTFSError(t, err, tfserr.Config) + if want := `No api-version was supplied for the "POST" request.`; te.Message != want { + t.Errorf("message = %q, want %q", te.Message, want) + } + }) + } +} diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 499940c..36c9eeb 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -247,16 +247,17 @@ func inputError(what string, err error) error { // live server answered 401 at the server level to a PAT it accepted at the // collection level. HTTP failures come back in their usual categories. func (g *globals) verify(ctx context.Context, cfg *config.Config) (string, error) { - // The request carries no api-version, whatever the config file says: - // connectionData has no released version, so a version pinned for the - // other commands would be refused for it, and without one the server - // answers at its latest preview version. + // The request carries no api-version, whatever the config file says, and + // none is negotiated, which the empty location ensures: connectionData has + // no released version, so a version pinned for the other commands would be + // refused for it, and without one the server answers at its latest preview + // version. cfg.APIVersion = "" client, err := apiclient.New(cfg, g.logger()) if err != nil { return "", err } - body, err := client.Get(ctx, "_apis/connectionData", nil) + body, err := client.Get(ctx, "", "_apis/connectionData", nil) if err != nil { return "", notFoundAt(err, cfg) } diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index dffa1d3..84eb9d4 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -104,6 +104,9 @@ func TestAuthLoginStoresTheCredential(t *testing.T) { if _, ok := s.query["api-version"]; ok { t.Errorf("query = %v, want no api-version", s.query) } + if s.options != 0 { + t.Errorf("OPTIONS requests = %d, want none: the login check negotiates no version", s.options) + } if want := "Basic " + base64.StdEncoding.EncodeToString([]byte(":secret-token")); s.auth != want { t.Errorf("Authorization = %q, want %q", s.auth, want) } diff --git a/internal/cli/cli.go b/internal/cli/cli.go index abbf1fb..51d6bd5 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -92,7 +92,7 @@ func newRoot(b build, stdin prompter, stdout, stderr io.Writer) *cobra.Command { f := root.PersistentFlags() f.BoolVar(&g.verbose, "verbose", false, "log every request to stderr (also TFSCLI_VERBOSE=1)") - f.StringVar(&g.apiVersion, "api-version", "", "REST API version (TFSCLI_API_VERSION; by default none is sent and the server chooses)") + f.StringVar(&g.apiVersion, "api-version", "", "REST API version (TFSCLI_API_VERSION; by default it is negotiated with the server)") root.AddCommand(newAuthCmd(g)) root.AddCommand(newWitCmd(g)) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 0aa0c6e..3f15b57 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -34,13 +34,27 @@ const workItemResponse = `{ } }` +// optionsResponse is the reply to OPTIONS on _apis: the resource locations +// tfscli calls, each with its versions, as a server lists them. +const optionsResponse = `{"count":3,"value":[ + {"id":"72c7ddf8-2cdc-4f60-90cd-ab71c14a399b","area":"wit","resourceName":"workItems","minVersion":"1.0","maxVersion":"7.1","releasedVersion":"7.1","resourceVersion":3}, + {"id":"908509b6-4248-4475-a1cd-829139ba419f","area":"wit","resourceName":"workItemsBatch","minVersion":"5.0","maxVersion":"7.1","releasedVersion":"7.1","resourceVersion":1}, + {"id":"1a9c53f7-f243-4447-b110-35ef023636e4","area":"wit","resourceName":"wiql","minVersion":"1.0","maxVersion":"7.1","releasedVersion":"7.1","resourceVersion":2} +]}` + // server records the request the CLI made and answers with a canned response. +// OPTIONS, which negotiates the API version, is answered with optionsStatus +// and optionsResponse and is counted only in options, so that the other +// fields describe the request of the command itself. type server struct { *httptest.Server status int body string + optionsStatus int + options int + calls int method string path string @@ -53,8 +67,17 @@ type server struct { func newServer(t *testing.T, status int, body string) *server { t.Helper() - s := &server{status: status, body: body} + s := &server{status: status, body: body, optionsStatus: http.StatusOK} s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodOptions { + s.options++ + w.WriteHeader(s.optionsStatus) + if s.optionsStatus == http.StatusOK { + _, _ = fmt.Fprint(w, optionsResponse) + } + return + } s.calls++ s.method = r.Method s.path = r.URL.Path @@ -64,7 +87,6 @@ func newServer(t *testing.T, status int, body string) *server { sent, _ := io.ReadAll(r.Body) s.sent = string(sent) - w.Header().Set("Content-Type", "application/json") w.WriteHeader(s.status) _, _ = fmt.Fprint(w, s.body) })) @@ -168,8 +190,11 @@ func TestWorkItemGetRequest(t *testing.T) { if got, want := s.query.Get("fields"), "System.Title,System.State"; got != want { t.Errorf("fields = %q, want %q — spaces around the comma are trimmed", got, want) } - if _, ok := s.query["api-version"]; ok { - t.Errorf("query = %v, want no api-version when none is configured", s.query) + if got, want := s.query.Get("api-version"), "7.1"; got != want { + t.Errorf("api-version = %q, want %q, negotiated when none is configured", got, want) + } + if s.options != 1 { + t.Errorf("OPTIONS requests = %d, want 1", s.options) } if want := "Basic " + base64.StdEncoding.EncodeToString([]byte(":secret-token")); s.auth != want { t.Errorf("Authorization = %q, want %q", s.auth, want) @@ -214,6 +239,9 @@ func TestWorkItemGetConfiguredAPIVersion(t *testing.T) { if got := s.query.Get("api-version"); got != tt.want { t.Errorf("api-version = %q, want %q unchanged", got, tt.want) } + if s.options != 0 { + t.Errorf("OPTIONS requests = %d, want none with a configured version", s.options) + } }) } } @@ -603,8 +631,11 @@ func TestVerboseLogsTheRequestToStderr(t *testing.T) { if code != 0 { t.Fatalf("exit code = %d, want 0 (stderr: %s)", code, stderr) } - if !strings.HasPrefix(stderr, "GET "+s.URL) { - t.Errorf("stderr = %q, want a logged GET request", stderr) + lines := strings.Split(strings.TrimSuffix(stderr, "\n"), "\n") + if len(lines) != 2 || + !strings.HasPrefix(lines[0], "OPTIONS "+s.URL+"/DefaultCollection/_apis ") || + !strings.HasPrefix(lines[1], "GET "+s.URL+"/DefaultCollection/MyProject/_apis/wit/workitems/12345?api-version=7.1 ") { + t.Errorf("stderr = %q, want the OPTIONS request and then the GET request with the negotiated version", stderr) } if strings.Contains(stderr, "secret-token") { t.Error("the log line contains the PAT") @@ -745,13 +776,30 @@ func TestWorkItemListRequest(t *testing.T) { if want := "/DefaultCollection/MyProject/_apis/wit/workitems"; s.path != want { t.Errorf("requested path %q, want %q", s.path, want) } - if got, want := s.query.Encode(), tt.want.Encode(); got != want { + if got, want := commandQuery(t, s).Encode(), tt.want.Encode(); got != want { t.Errorf("query = %q, want %q", got, want) } }) } } +// commandQuery returns the query s received without api-version, after +// checking that it carries the version negotiated from optionsResponse: what +// remains are the parameters of the command itself. +func commandQuery(t *testing.T, s *server) url.Values { + t.Helper() + if got, want := s.query.Get("api-version"), "7.1"; got != want { + t.Errorf("api-version = %q, want the negotiated %q", got, want) + } + q := url.Values{} + for k, v := range s.query { + if k != "api-version" { + q[k] = v + } + } + return q +} + func TestWorkItemBatchReportsServerErrors(t *testing.T) { for _, command := range batchCommands { t.Run(command, func(t *testing.T) { @@ -879,8 +927,8 @@ func TestWorkItemGetBatchRequest(t *testing.T) { if want := "/DefaultCollection/MyProject/_apis/wit/workitemsbatch"; s.path != want { t.Errorf("requested path %q, want %q", s.path, want) } - if len(s.query) != 0 { - t.Errorf("query = %v, want none: the parameters belong in the body", s.query) + if q := commandQuery(t, s); len(q) != 0 { + t.Errorf("query = %v, want only api-version: the parameters belong in the body", s.query) } if s.sent != tt.want { t.Errorf("body = %s, want %s", s.sent, tt.want) diff --git a/internal/cli/wiql_test.go b/internal/cli/wiql_test.go index ffb22f9..0df6a44 100644 --- a/internal/cli/wiql_test.go +++ b/internal/cli/wiql_test.go @@ -132,7 +132,7 @@ func TestWiqlQueryByWiqlRequest(t *testing.T) { if s.rawPath != tt.rawPath { t.Errorf("requested path %q, want %q", s.rawPath, tt.rawPath) } - if !reflect.DeepEqual(s.query, tt.query) { + if !reflect.DeepEqual(commandQuery(t, s), tt.query) { t.Errorf("query = %v, want %v", s.query, tt.query) } want, _ := json.Marshal(map[string]string{"query": testWiql}) diff --git a/internal/log/log.go b/internal/log/log.go index 5a63b8e..66d7272 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -12,6 +12,7 @@ import ( type Logger interface { LogRequest(method, url string, status int, dur time.Duration) Warn(msg string) + Info(msg string) } // Noop returns a Logger that discards everything. It is the default when @@ -26,6 +27,9 @@ func (noop) LogRequest(method, url string, status int, dur time.Duration) {} // Warn discards the message. func (noop) Warn(msg string) {} +// Info discards the message. +func (noop) Info(msg string) {} + // New returns a Logger writing one line per event to w. The CLI passes // os.Stderr when --verbose / TFSCLI_VERBOSE=1 is set. func New(w io.Writer) Logger { return &writerLogger{w: w} } @@ -44,3 +48,9 @@ func (l *writerLogger) LogRequest(method, url string, status int, dur time.Durat func (l *writerLogger) Warn(msg string) { _, _ = fmt.Fprintf(l.w, "[warn] %s\n", msg) } + +// Info writes one line describing what tfscli did on its own, such as a +// fallback, that a reader of the request lines would otherwise have to infer. +func (l *writerLogger) Info(msg string) { + _, _ = fmt.Fprintf(l.w, "[info] %s\n", msg) +} diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 56609c9..10a6d71 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -50,6 +50,17 @@ func TestWriterLoggerWarn(t *testing.T) { } } +func TestWriterLoggerInfo(t *testing.T) { + var sb strings.Builder + l := New(&sb) + + l.Info("api-version not negotiated") + + if got, want := sb.String(), "[info] api-version not negotiated\n"; got != want { + t.Errorf("Info() wrote %q, want %q", got, want) + } +} + var ( _ Logger = Noop() _ Logger = New(io.Discard) @@ -63,4 +74,5 @@ func TestNoopDoesNotPanic(t *testing.T) { l.LogRequest("GET", "https://tfs.company.com/coll/_apis/a", 200, time.Second) l.Warn("TLS verification disabled") + l.Info("api-version not negotiated") } diff --git a/internal/wiql/wiql.go b/internal/wiql/wiql.go index 17b7243..5fd4da1 100644 --- a/internal/wiql/wiql.go +++ b/internal/wiql/wiql.go @@ -11,11 +11,16 @@ import ( ) // APIClient is the part of the API client this package uses: Post for Query -// By Wiql. +// By Wiql. location is the id under which the server lists the resource +// among its API versions. type APIClient interface { - Post(ctx context.Context, path string, query url.Values, body any) ([]byte, error) + Post(ctx context.Context, location, path string, query url.Values, body any) ([]byte, error) } +// wiqlLocation is the resource location id of Query By Wiql, with and without +// a team, as the official SDKs give it. +const wiqlLocation = "1a9c53f7-f243-4447-b110-35ef023636e4" + // Request holds the parameters of Query By Wiql. Project and Team are the // optional path segments: an empty Project runs the query at collection // level, and Team is used only together with a Project, which the caller @@ -67,7 +72,7 @@ func QueryByWiql(ctx context.Context, client APIClient, req Request) (*Result, e Query string `json:"query"` }{req.Query} - resp, err := client.Post(ctx, path(req), query, body) + resp, err := client.Post(ctx, wiqlLocation, path(req), query, body) if err != nil { return nil, err } diff --git a/internal/wiql/wiql_test.go b/internal/wiql/wiql_test.go index d67ea63..47af084 100644 --- a/internal/wiql/wiql_test.go +++ b/internal/wiql/wiql_test.go @@ -16,13 +16,14 @@ type fakeClient struct { body []byte err error - path string - query url.Values - sent any + location string + path string + query url.Values + sent any } -func (c *fakeClient) Post(_ context.Context, path string, query url.Values, body any) ([]byte, error) { - c.path, c.query, c.sent = path, query, body +func (c *fakeClient) Post(_ context.Context, location, path string, query url.Values, body any) ([]byte, error) { + c.location, c.path, c.query, c.sent = location, path, query, body if c.err != nil { return nil, c.err } @@ -90,6 +91,9 @@ func TestQueryByWiqlRequestsPath(t *testing.T) { if client.path != tt.want { t.Errorf("requested path %q, want %q", client.path, tt.want) } + if want := "1a9c53f7-f243-4447-b110-35ef023636e4"; client.location != want { + t.Errorf("location = %q, want %q", client.location, want) + } }) } } diff --git a/internal/workitem/workitem.go b/internal/workitem/workitem.go index 3fdb0b3..22db478 100644 --- a/internal/workitem/workitem.go +++ b/internal/workitem/workitem.go @@ -16,12 +16,20 @@ import ( ) // APIClient is the part of the API client this package uses: Get for Get Work -// Item and List, Post for Get Work Items Batch. +// Item and List, Post for Get Work Items Batch. location is the id under +// which the server lists the resource among its API versions. type APIClient interface { - Get(ctx context.Context, path string, query url.Values) ([]byte, error) - Post(ctx context.Context, path string, query url.Values, body any) ([]byte, error) + Get(ctx context.Context, location, path string, query url.Values) ([]byte, error) + Post(ctx context.Context, location, path string, query url.Values, body any) ([]byte, error) } +// The resource location ids of the operations this package calls, as the +// official SDKs give them: Get Work Item and Work Items - List share one. +const ( + workItemsLocation = "72c7ddf8-2cdc-4f60-90cd-ab71c14a399b" + workItemsBatchLocation = "908509b6-4248-4475-a1cd-829139ba419f" +) + // FieldKind tells the printer how to render a field value. type FieldKind string @@ -100,7 +108,7 @@ func Get(ctx context.Context, client APIClient, project string, id int, fields [ query.Set("fields", strings.Join(fields, ",")) } - body, err := client.Get(ctx, fmt.Sprintf("%s/_apis/wit/workitems/%d", url.PathEscape(project), id), query) + body, err := client.Get(ctx, workItemsLocation, fmt.Sprintf("%s/_apis/wit/workitems/%d", url.PathEscape(project), id), query) if err != nil { return nil, err } @@ -122,7 +130,7 @@ func List(ctx context.Context, client APIClient, project string, req BatchReques query.Set("errorPolicy", req.ErrorPolicy) } - body, err := client.Get(ctx, url.PathEscape(project)+"/_apis/wit/workitems", query) + body, err := client.Get(ctx, workItemsLocation, url.PathEscape(project)+"/_apis/wit/workitems", query) if err != nil { return nil, err } @@ -143,7 +151,7 @@ func GetBatch(ctx context.Context, client APIClient, project string, req BatchRe ErrorPolicy string `json:"errorPolicy,omitempty"` }{req.IDs, req.Fields, req.AsOf, req.ErrorPolicy} - resp, err := client.Post(ctx, url.PathEscape(project)+"/_apis/wit/workitemsbatch", nil, body) + resp, err := client.Post(ctx, workItemsBatchLocation, url.PathEscape(project)+"/_apis/wit/workitemsbatch", nil, body) if err != nil { return nil, err } diff --git a/internal/workitem/workitem_test.go b/internal/workitem/workitem_test.go index 7609f03..a78c93b 100644 --- a/internal/workitem/workitem_test.go +++ b/internal/workitem/workitem_test.go @@ -18,22 +18,23 @@ type fakeClient struct { body []byte err error - method string - path string - query url.Values - sent any + method string + location string + path string + query url.Values + sent any } -func (c *fakeClient) Get(_ context.Context, path string, query url.Values) ([]byte, error) { - c.method, c.path, c.query = "GET", path, query +func (c *fakeClient) Get(_ context.Context, location, path string, query url.Values) ([]byte, error) { + c.method, c.location, c.path, c.query = "GET", location, path, query if c.err != nil { return nil, c.err } return c.body, nil } -func (c *fakeClient) Post(_ context.Context, path string, query url.Values, body any) ([]byte, error) { - c.method, c.path, c.query, c.sent = "POST", path, query, body +func (c *fakeClient) Post(_ context.Context, location, path string, query url.Values, body any) ([]byte, error) { + c.method, c.location, c.path, c.query, c.sent = "POST", location, path, query, body if c.err != nil { return nil, c.err } @@ -88,22 +89,23 @@ func TestGetRequestsDocumentedPath(t *testing.T) { func TestEscapesProjectInPath(t *testing.T) { const project = "100% Done" tests := []struct { - name string - read func(*fakeClient) error - want string + name string + read func(*fakeClient) error + want string + location string }{ {"get", func(c *fakeClient) error { _, err := Get(context.Background(), c, project, 12345, nil) return err - }, "100%25%20Done/_apis/wit/workitems/12345"}, + }, "100%25%20Done/_apis/wit/workitems/12345", "72c7ddf8-2cdc-4f60-90cd-ab71c14a399b"}, {"list", func(c *fakeClient) error { _, err := List(context.Background(), c, project, BatchRequest{IDs: []int{297}}) return err - }, "100%25%20Done/_apis/wit/workitems"}, + }, "100%25%20Done/_apis/wit/workitems", "72c7ddf8-2cdc-4f60-90cd-ab71c14a399b"}, {"get-batch", func(c *fakeClient) error { _, err := GetBatch(context.Background(), c, project, BatchRequest{IDs: []int{297}}) return err - }, "100%25%20Done/_apis/wit/workitemsbatch"}, + }, "100%25%20Done/_apis/wit/workitemsbatch", "908509b6-4248-4475-a1cd-829139ba419f"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -114,6 +116,9 @@ func TestEscapesProjectInPath(t *testing.T) { if client.path != tt.want { t.Errorf("requested path %q, want %q", client.path, tt.want) } + if client.location != tt.location { + t.Errorf("location = %q, want %q", client.location, tt.location) + } }) } } diff --git a/skills/tfscli/SKILL.md b/skills/tfscli/SKILL.md index b8cdef0..a8af938 100644 --- a/skills/tfscli/SKILL.md +++ b/skills/tfscli/SKILL.md @@ -7,7 +7,7 @@ description: Read work items from an on-premises TFS / Azure DevOps Server with `tfscli` is a single-binary, read-only client for the TFS / Azure DevOps Server REST API. It is stateless: there is no daemon and no cache, and every -invocation performs one HTTP request against the server. +invocation performs its requests against the server anew. The requests it issues are: @@ -18,8 +18,9 @@ POST ///_apis/wit/workitemsbatch wit work-item POST /[/[/]]/_apis/wit/wiql wit wiql query-by-wiql ``` -with `api-version=` added to the query only when a version is -configured. +with `api-version=` added to the query. Unless a version is +configured, each of them is preceded by `OPTIONS //_apis`, +which tells tfscli the version the server supports for the resource. Authentication is a personal access token sent as HTTP Basic with an empty user name. Output is markdown on stdout; errors are one line on stderr. @@ -101,7 +102,7 @@ OS), environment variables, command-line flags. | Setting | Config key | Environment variable | Flag | Default | Required | |---|---|---|---|---|---| | Team project | `project` | `TFSCLI_PROJECT` | `-p`, `--project` | — | per command | -| REST API version | `apiVersion` | `TFSCLI_API_VERSION` | `--api-version` | none | no | +| REST API version | `apiVersion` | `TFSCLI_API_VERSION` | `--api-version` | negotiated with the server | no | | Request logging | — | `TFSCLI_VERBOSE=1` | `--verbose` | off | no | Two further keys are accepted in the config file only, with no environment @@ -109,17 +110,19 @@ variable and no flag: `caBundle` (path to a PEM bundle appended to the system root pool, for an internal CA) and `insecureSkipVerify` (disables certificate verification entirely). -Without a configured API version no `api-version` is sent, and the server -answers at the version it chooses. A configured version is sent unchanged. Do -not set one unless the user asks for it. +Without a configured API version, tfscli negotiates one with the server for +each request; nothing has to be set. A configured version is sent unchanged. +Do not set one unless the user asks for it. -`wit work-items get-batch` and `wit wiql query-by-wiql` send a POST request, -and a server may refuse a POST without a version: the `config` error then says -that no `api-version` was supplied. Do not pick a version yourself, and do not -take the `1.0` from the server's example. Report the error and let the user set -the version their server supports, with `--api-version`, `TFSCLI_API_VERSION`, -or `apiVersion` in the config file. For `get-batch`, `wit work-items list` -reads the same work items with a GET and needs no version. +When the server does not answer the negotiation, the request is sent without +a version. `wit work-items get-batch` and `wit wiql query-by-wiql` send a POST +request, which a server may then refuse: the `config` error says that no +`api-version` was supplied and that the version could not be negotiated. Do not +pick a version yourself, and do not take the `1.0` from the server's example. +Report the error and let the user set the version their server supports, with +`--api-version`, `TFSCLI_API_VERSION`, or `apiVersion` in the config file. For +`get-batch`, `wit work-items list` reads the same work items with a GET, which +servers accept without a version. The config file is optional. Environment variables and flags alone are enough, so `-p` can carry everything a call needs beyond the credential. @@ -139,7 +142,7 @@ request. |---|---| | `-p`, `--project` | Team project. Required unless `project` is in the config file or `TFSCLI_PROJECT` is set. | | `--fields` | Comma-separated TFS reference names. Without it, every field is printed. | -| `--verbose` | One line per request to stderr: method, URL, status, duration. Never headers or bodies. | +| `--verbose` | One line per request to stderr, the `OPTIONS` request that negotiates the API version included: method, URL, status, duration. Never headers or bodies. A line starting with `[info]` says what tfscli did on its own, such as sending a request without a version. | Narrow the request whenever the needed fields are known — a full work item can be large, and its tokens are paid for on every call: @@ -328,7 +331,7 @@ removed or renamed, though new ones may appear. | `forbidden` | HTTP 403 — authenticated, but access denied. | Do not retry. The PAT lacks the scope, or the project is closed to this user. Report it. | | `not_found` | HTTP 404 — no such work item, project, or team. For `wit work-items list` and `wit work-items get-batch`, one missing id is enough. | Check the id, the project, and the team spelling against what the user gave. Do not scan ids looking for a match. For several ids, retry once with `--error-policy omit` to learn which are missing. | | `server` | HTTP 5xx, or a response that could not be parsed. | One retry is reasonable. If it repeats, report the server as unavailable. | -| `config` | A missing or invalid setting, a missing or malformed credential, a bad argument, or a request TFS rejected — a 4xx other than 401, 403, and 404, such as an unknown field name or an unsupported API version. A refused API version names the setting it came from — `--api-version`, `TFSCLI_API_VERSION`, or `apiVersion` in the config file — and the next step. | Fix the invocation if the fault is in it. If the API version was refused and it came from `--api-version` you passed, drop the flag and retry; if it came from the environment or the config file, report the message to the user rather than changing the setting. If a setting is missing, report what is missing; do not write the config file. If the user is not logged in, tell them to run `tfscli auth login`. | +| `config` | A missing or invalid setting, a missing or malformed credential, a bad argument, or a request TFS rejected — a 4xx other than 401, 403, and 404, such as an unknown field name or an unsupported API version. A refused API version names the setting it came from — `--api-version`, `TFSCLI_API_VERSION`, or `apiVersion` in the config file — and the next step. A request refused for carrying no version, sent so because the version could not be negotiated, names those three settings. | Fix the invocation if the fault is in it. If no version could be negotiated, report the message to the user; do not set a version yourself. If the API version was refused and it came from `--api-version` you passed, drop the flag and retry; if it came from the environment or the config file, report the message to the user rather than changing the setting. If a setting is missing, report what is missing; do not write the config file. If the user is not logged in, tell them to run `tfscli auth login`. | | `network` | The server could not be reached, or the request timed out or was canceled. | Do not repeat the call in a loop. Report the server as unreachable and let the user check the URL and their connection. | Representative messages: