This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit 82f00f105a36f51ab37d3c9254fb225d7e7bc25f Author: Eric Pugh <[email protected]> AuthorDate: Fri Sep 25 10:20:15 2026 -0400 Documented V2 API Conventions (#4826) (cherry picked from commit bc428ba81bc0ce1dad04a8032dfc35966166136d) --- AGENTS.md | 2 +- ...i-conventions.adoc => apis-v2-conventions.adoc} | 39 ++++++++++- dev-docs/apis.adoc | 81 +++++++++++++++++++++- 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7c0bd0d1b74..64ad2dadb2e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,7 +63,7 @@ Internals: - `dev-docs/shard-split/shard-split.adoc` — SPLITSHARD: shard/replica states, tlog buffering during split - `dev-docs/distributed-update-internals.adoc` — SolrCloud update path: routing, `_version_`/optimistic concurrency, tlog durability, replication acks, shard terms (user-facing consistency model: ref-guide page `solrcloud-update-consistency.adoc`) - `dev-docs/plugins-modules-packages.adoc` — plugin/module/package concepts -- `dev-docs/apis.adoc`, `dev-docs/v2-api-conventions.adoc` — API design and v2 conventions +- `dev-docs/apis.adoc`, `dev-docs/apis-v2-conventions.adoc` — API design and v2 conventions - `dev-docs/ui/` — new Admin UI architecture, component development, testing Process & tooling: diff --git a/dev-docs/v2-api-conventions.adoc b/dev-docs/apis-v2-conventions.adoc similarity index 59% rename from dev-docs/v2-api-conventions.adoc rename to dev-docs/apis-v2-conventions.adoc index 0dc61247bea..945c94afe72 100644 --- a/dev-docs/v2-api-conventions.adoc +++ b/dev-docs/apis-v2-conventions.adoc @@ -65,7 +65,7 @@ Solr's "modify collection" functionality uses partial update semantics, but the For use within the v2 API, the four "popular" HTTP methods have the following semantics and implications: -* `GET` - used for non-mutating (i.e. "read only") requests. Most often used to list elements of a particular resource type, or fetch information about a specific named resource. +* `GET` - used for non-mutating (i.e. "read only") requests. Most often used to list elements of a particular resource type, or fetch information about a specific named resource. Do not use for requests like queries that change state. * `POST` - used for non-idempotent resource modifications. * `PUT` - used for idempotent resource modifications. * `DELETE` - Used to delete or cleanup resource @@ -142,3 +142,40 @@ In these cases, developers may: AddFieldOperation requestBody) throws Exception; ``` + +== Response POJOs + +Every v2 response body extends `SolrJerseyResponse`, which provides `responseHeader` and `error` - except for the raw/streaming APIs described under "Streaming" above, whose interface methods return `StreamingOutput` instead. +Beyond that base, pick the narrowest existing response type that fits before writing a new one: + +* `SolrJerseyResponse` - plain success/error response with no extra data. The default for simple mutations. +* `AsyncJerseyResponse extends SolrJerseyResponse` - adds a `requestId` field. Use this (or a subclass of it) for any API that supports the `async` request-body parameter, since `submitRemoteMessageAndHandleAsync`/`handlePotentiallyAsynchronousTask` populate `requestId` automatically when an async id is present. +* `SubResponseAccumulatingJerseyResponse extends AsyncJerseyResponse` - adds `successfulSubResponsesByNodeName`, `failedSubResponsesByNodeName`, and `warning`. Use this for Overseer-driven APIs that fan out to multiple nodes/replicas (e.g. `CreateShard`, `DeleteShard`) - `AdminAPIBase.submitRemoteMessageAndHandleResponse` populates these fields from the Overseer's `success`/`failure`/`warning` NamedList entries automatically. +* `FlexibleSolrJerseyResponse extends SolrJerseyResponse` - adds `@JsonAnyGetter`/`@JsonAnySetter`-backed dynamic *top-level* properties, for APIs whose entire response shape is genuinely open-ended (e.g. `SchemaDesigner`, the `Select` query API). This is different from the dynamic-request-POJO pattern above: it's for responses, and the dynamism applies to the whole top level rather than one field. + +If an API returns specific, known additional data beyond these bases (e.g. timing information, computed ranges, a resource's status fields), extend the appropriate base with typed `@JsonProperty` fields rather than reaching for `FlexibleSolrJerseyResponse` - see `SplitShardResponse.timing`, `SplitCoreResponse.ranges`, or `CollectionStatusResponse` for examples. +Don't settle for a bare `SolrJerseyResponse` if the underlying operation actually produces more than a bare success/error - a v2 JSON caller has no other way to get that data, since (unlike v1) nothing else in the response pipeline will surface it (see the async caveat below for why this matters in practice). + +== Async Task Handling + +Solr has two distinct, non-interchangeable mechanisms for handling the `async` request parameter, depending on which base class the API extends: + +* **Core-level (`CoreAdminAPIBase`)**: `handlePotentiallyAsynchronousTask(response, coreName, taskId, actionName, supplier)`. If `taskId` is null, the supplier runs inline and its result is returned directly. If non-null, the supplier is wrapped in a `CoreAdminAsyncTracker.TaskObject` and submitted to `coreAdminAsyncTracker`, which tracks status for later `REQUESTSTATUS` polling. Because the supplier is a `Supplier<T>`, it cannot throw checked exceptions directly - wrap them in `CoreAdmi [...] +* **Collection-level (`AdminAPIBase`)**: `submitRemoteMessageAndHandleAsync`/`submitRemoteMessageAndHandleResponse(response, action, remoteMessage, asyncId[, timeoutMs])`. The `asyncId` is baked into the submitted `ZkNodeProps` message and handled by the Overseer's own async-tracking machinery; `response.requestId` is populated automatically when `asyncId` is non-null. A timeout-aware overload exists for APIs (like shard split) that legitimately need longer than `CollectionsHandler.DEFAU [...] + +Only wrap the branches of an API that are actually meant to support async execution. +A synchronous-by-design sub-operation (e.g. a "dry run" or "compute recommendations" branch that happens to share a method with the real mutating operation) should bypass async handling entirely rather than being routed through it - see the `async` caveat under "Relationship Between V1 and V2 Implementations" in link:apis.adoc[apis.adoc] for a concrete failure mode this avoids. + +== Testing V2 APIs + +Newer v2 API tests favor exercising the real HTTP stack over mocking it: + +1. Start a real embedded Solr instance with `SolrJettyTestRule` (a `@ClassRule`) rather than the older `initCore()`/mock-`SolrQueryRequest` style. +2. Issue requests through the *generated* SolrJ client (e.g. `CoresApi.SplitCore`, `ShardsApi.SplitShard`) rather than hand-built `GenericV2SolrRequest`s, so the test also exercises the client codegen. +3. Give each API its own dedicated test class under `handler.admin.api` (e.g. `RenameCoreAPITest`, `SplitCoreAPITest`, `SplitShardAPITest`), rather than adding cases to a large shared mapping test. +4. Assert error responses via `expectThrows(RemoteSolrException.class, () -> request.process(client))`, then check `.code()` and `.getMessage()` - a non-2xx v2 response surfaces as a thrown exception on the client side, not as a populated `.error` field on a normally-returned response object. + +This has superseded the older `V2ApiMappingTest`-based style (e.g. `V2CoreAPIMappingTest`), which mocks the v1 handler and asserts on the `SolrParams` the v2-to-v1 conversion produces. +That style still exists for APIs that haven't been migrated to the "logic lives in v2" pattern yet, but shouldn't be extended for newly-migrated APIs - once an API's logic moves into v2, its mapping test loses its reason to exist and should be removed (see "Relationship Between V1 and V2 Implementations" in link:apis.adoc[apis.adoc]) in favor of a real-client test. + +One exception: white-box tests that need access to package-private helper methods (e.g. `SplitHandlerTest`, which tests `SplitCoreAPI`'s histogram/range-recommendation internals) should live in the *same package* as the class under test, and move with it if that class's package ever changes. diff --git a/dev-docs/apis.adoc b/dev-docs/apis.adoc index fa0565a14d9..b394b16d452 100644 --- a/dev-docs/apis.adoc +++ b/dev-docs/apis.adoc @@ -51,7 +51,7 @@ Separating the API "definition" and "implementation" in this way allows us to on Writing a new v2 API may appear daunting, but additions in reality are actually pretty simple: -. *Agree on Endpoint Syntax*: Before implementing, developers should generate consensus around what the new API will look like. General guidelines and conventions for the v2 API syntax are described <<v2-api-conventions.adoc,here>>. +. *Agree on Endpoint Syntax*: Before implementing, developers should generate consensus around what the new API will look like. General guidelines and conventions for the v2 API syntax are described <<apis-v2-conventions.adoc,here>>. . *Create POJO ("Plain Old Java Object") classes as needed to represent the API's request and response*: ** POJOs are used to represent both the body of the API request (for some `POST` and `PUT` endpoints), as well as the response from the API. ** Re-use of existing classes here is preferred where possible. A library of available POJOs can be found in the `org.apache.solr.client.api.model` package of the `api` gradle project. @@ -76,6 +76,85 @@ Writing a new v2 API may appear daunting, but additions in reality are actually A good example for each of these steps can be seen in Solr's v2 "add-replica-property" API, which has a defining interface https://github.com/apache/solr/blob/9426902acb7081a2e9a1fa29699c5286459e1365/solr/api/src/java/org/apache/solr/client/api/endpoint/AddReplicaPropertyApi.java[AddReplicaPropertyApi], an implementing class https://github.com/apache/solr/blob/9426902acb7081a2e9a1fa29699c5286459e1365/solr/core/src/java/org/apache/solr/handler/admin/api/AddReplicaProperty.java[AddReplicaP [...] +=== Relationship Between V1 and V2 Implementations + +Most v2 APIs have a corresponding legacy v1 API (e.g. `/admin/cores?action=RELOAD` backs `POST /api/cores/coreName/reload`). +Where both exist, **the actual business logic should live in the v2 implementation class, and the v1 handler should delegate to it** - not the other way around. +This convention exists for a few reasons: + +1. It makes it easier to delete the v1 code down the road. +2. It prevents the v2 code from "falling behind" or being forgotten when query params or API functionality changes, since v1 traffic continuously exercises the same v2 code path. +3. It gives the v2 endpoint test coverage "for free" - most of Solr's existing test suite targets v1 APIs, so having v1 call v2 gives confidence that the v2 endpoint works correctly even in the absence of direct v2-specific tests. + +How this delegation is implemented differs depending on whether the API executes synchronously or is processed by the Overseer. + +==== Synchronous (CoreAdminAPIBase) APIs + +Note: this is distinct from "per-core" APIs generally (see the "API Types" section above), which are registered on individual `SolrCore` objects (e.g. `/select`, `/update`, `/admin/ping`). `CoreAdminAPIBase`-derived APIs are instead a "cluster" API registered on the `CoreContainer`, corresponding to the legacy `/admin/cores` handler. + +For `CoreAdminAPIBase`-derived APIs (i.e. `/api/cores/...`), the v1 `CoreAdminHandler.CoreAdminOp` implementation is reduced to a thin adapter: parse the v1 `SolrParams` into the v2 request-body POJO, construct the v2 API class, call it, and squash the response back into the v1 `NamedList` via `V2ApiUtils.squashIntoSolrResponseWithoutHeader`. + +```java +class MergeIndexesOp implements CoreAdminHandler.CoreAdminOp { + @Override + public void execute(CoreAdminHandler.CallInfo it) throws Exception { + SolrParams params = it.req.getParams(); + final var requestBody = new MergeIndexesRequestBody(); + // ... populate requestBody from params ... + final var mergeIndexesApi = + new MergeIndexes(it.handler.coreContainer, it.handler.coreAdminAsyncTracker, it.req, it.rsp); + final var response = mergeIndexesApi.mergeIndexes(cname, requestBody); + V2ApiUtils.squashIntoSolrResponseWithoutHeader(it.rsp, response); + } +} +``` + +See `RenameCore`/`CoreAdminOperation.RENAME_OP`, `MergeIndexes`/`MergeIndexesOp`, and `SplitCoreAPI`/`SplitOp` for examples of this pattern. + +==== Overseer-driven (AdminAPIBase) APIs + +Note: not all collection-level APIs are Overseer-driven, and that's expected to become even less true over time. This section covers only the subset of `AdminAPIBase`-derived APIs that are processed asynchronously by the Overseer via a ZK-based command queue. + +For these APIs (i.e. `/api/collections/...`), the underlying `cloud/api/collections/*Cmd.java` executor (e.g. `SplitShardCmd`, `CreateShardCmd`) is *not* moved into v2 - it stays registered in `CollApiCmds.java` and continues to execute on the Overseer thread exactly as before. +"Logic in v2" here means the v2 class is responsible for: + +1. Validating parameters (including any cross-parameter conflict checks previously done by the v1 handler, so the check applies uniformly to v1 and native v2 requests) +2. Building the `ZkNodeProps` "remote message" the Overseer command expects +3. Submitting it via `AdminAPIBase.submitRemoteMessageAndHandleResponse`/`submitRemoteMessageAndHandleAsync` - the same ZK-queue submission machinery v1 always used + +The v1 `CollectionOperation` enum entry is then reduced to a one-line delegation: + +```java +SPLITSHARD_OP( + SPLITSHARD, + (req, rsp, h) -> { + SplitShardAPI.invokeWithV1Params(h.coreContainer, req, rsp); + return null; + }), +``` + +Returning `null` from a `CollectionOperation` lambda tells `CollectionsHandler.invokeAction()` that the request has already been fully handled. +Returning a non-null `Map<String, Object>` instead falls back to the older pattern where `CollectionsHandler` itself builds and submits the `ZkNodeProps` message generically - this is still the state of many not-yet-migrated v1 actions, and is *not* an example to follow for new migrations. + +See `CreateShard`/`CREATESHARD_OP`, `DeleteShard`/`DELETESHARD_OP`, and `SplitShardAPI`/`SPLITSHARD_OP` for examples of the target pattern. + +==== Standard v1-delegation entry points + +By convention, a migrated v2 API class exposes two `public static` methods for v1 to call: + +* `createRequestBodyFromV1Params(SolrParams params)` - parses legacy params into the v2 request-body POJO. +* `invokeWithV1Params(CoreContainer, SolrQueryRequest, SolrQueryResponse)` (sometimes named `invokeFromV1Params` - naming isn't fully consistent yet) - constructs the v2 API class, calls it, and squashes the response into the v1 `SolrQueryResponse`. + +==== Don't forward `async` from v1 + +`CoreAdminHandler.handleRequestBody()` and `CollectionsHandler.invokeAction()` already wrap the *entire* v1 dispatch in their own async-task submission, keyed by the request's `async` parameter, before the `CoreAdminOp`/`CollectionOperation` even runs. +If a v1 adapter also copies that same `async` value into the v2 request body, the v2 class's own async handling (`handlePotentiallyAsynchronousTask`, `submitRemoteMessageAndHandleAsync`) will try to register the identical task id a second time, and fail with `Duplicate request with the same requestid found.` + +*Leave the v2 request body's `async` field unset when delegating from v1.* +`MergeIndexesRequestBody` has no `async` field at all, and `SplitOp`'s v1 adapter deliberately omits setting one - both are correct examples of this. This only applies to genuine v1-delegation adapters; native v2 JSON requests should set `async` normally, and the v2 class's own handling of it is unaffected. + +Watch for this same class of bug in *internal* sub-requests too: some Overseer commands (e.g. `SplitShardCmd`'s `splitByPrefix` sub-request, sent via `ShardRequestTracker.sendShardRequest`) piggyback an internal correlation id on the `async` param purely for response tracking, not because the sub-request is meant to be tracked as a real async task. If a migrated v2 method routes such a sub-request through its own async wrapper, the same "duplicate requestid" failure results. Keep any suc [...] + === Legacy v2 API Framework While we've settled on JAX-RS as our framework for defining v2 APIs going forward, Solr still retains many v2 APIs that were written using an older homegrown framework.
