Add `state` query param to GET snapshots API by JeremyDahlgren · Pull Request #128635 · elastic/elasticsearch

JeremyDahlgren · 2025-05-29T19:02:22Z

This PR picks up where Elena left off in #123618, addressing the last round of review comments and adding to the tests.

PR description details copied from #123618:

This PR introduces a new state query parameter for the Get Snapshots API, allowing users to filter snapshots by state.
The parameter accepts comma-separated values for states: SUCCESS, IN_PROGRESS, FAILED, PARTIAL, INCOMPATIBLE (case-insensitive).

🎯 Motivation & Use Case

This is my mini-project for the AppEx team’s On-Week, where I aimed to explore contributing to Elasticsearch while addressing several Kibana issues.

In Kibana's Snapshot Restore page, multiple open issues could be resolved by adding this parameter:

1️⃣ Fixing incorrect selection of last successful managed snapshot

Helps correctly identify the last successful managed snapshot, ensuring delete actions are disabled for it.
Resolves bugs [Snapshot and Restore] Incorrect Last Successful Managed Snapshot when the list of snapshots is filtered kibana#158548 and [Snapshot/Restore] UI marks snapshot as "not deletable" falsely kibana#153107

2️⃣ Enhancing the Snapshots table:

Enables filtering/sorting snapshots by their state, improving usability.
Addresses issues Allow filtering of the snapshot STATE in Snapshot and Restore UI kibana#137432 and [Snapshot and Restore] Sort state in snapshots list kibana#202883

✅ Summary of Changes

Added state as an optional query parameter to GET _snapshot/{repository}/_all
Updated filtering logic in TransportGetSnapshotsAction
Added validation of the parameter value
Added integration tests in GetSnapshotsIT.java

elasticsearchmachine · 2025-05-29T19:02:47Z

Hi @JeremyDahlgren, I've created a changelog YAML for you.

elasticsearchmachine · 2025-05-29T19:56:58Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

…ture

mhl-b · 2025-06-11T17:25:29Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java


+            final var details = repositoryData.getSnapshotDetails(snapshotId);
+
+            if (states.isEmpty() == false


nit: can omit emptiness check

++ we could reasonably reject a request with an empty states set in org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest#validate

Removed isEmpty() check, added a check in GetSnapshotsRequest#validate, 3ed9350.

server/src/main/java/org/elasticsearch/action/ActionModule.java

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

mhl-b · 2025-06-11T21:04:20Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

        getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames()));
+
+        final String stateString = request.param("state");
+        if (Strings.hasLength(stateString) == false) {


~~State param with empty value seems valid in this condition even if feature is not available ("state="). I think having param keyword should be illegal when feature is off.~~

Nvm :) I confused myself with another case. If feature is supported but state param is null, will it throw NPE at stateString.split?

Added separate handling for null vs empty, 64d3729.

DaveCTurner · 2025-06-12T11:42:53Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java


+            final var details = repositoryData.getSnapshotDetails(snapshotId);
+
+            if (states.isEmpty() == false


++ we could reasonably reject a request with an empty states set in org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest#validate

DaveCTurner · 2025-06-12T11:43:05Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

                return false;
            }

+            if (states.isEmpty() == false && snapshotInfo.state() != null && states.contains(snapshotInfo.state()) == false) {


Likewise here, states should never be empty

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

DaveCTurner · 2025-06-12T11:48:07Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

        getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames()));
+
+        final String stateString = request.param("state");
+        if (Strings.hasLength(stateString) == false) {


I think we could reasonably reject requests that include ?state= without a value, in constrast to the parameter being missing (i.e. null) which should mean "all states" as done here.

Added separate handling for null vs empty, 64d3729.

DaveCTurner · 2025-06-12T11:49:19Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

+        // Consume these response parameters used in SnapshotInfo now, to avoid assertion errors in BaseRestHandler for requests where they
+        // may not get used.
+        if (Assertions.ENABLED) {
+            for (final var responseParameter : SUPPORTED_RESPONSE_PARAMETERS) {
+                request.param(responseParameter);
+            }
+        }


Hmm I'm suspicious about this, are you sure it's necessary? If so, it seems like a broader problem than just this one API so maybe we should fix it more fundamentally.

If you comment this if block out you can get an error running this test:

./gradlew :rest-api-spec:yamlRestTest --tests org.elasticsearch.test.rest.ClientYamlTestSuiteIT -Dtests.method='test {yaml=snapshot.get/10_basic/*}'

This will fail with Connection refused errors, but if you rerun with --debug you can see the root cause:

2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] Caused by: java.lang.AssertionError: get_snapshots_action: consumed params [after, error_trace, filter_path, format, from_sort_value, human, ignore_unavailable, index_names, master_timeout, offset, order, pretty, repository, size, slm_policy_filter, snapshot, sort, state, verbose] while supporting [after, error_trace, filter_path, format, from_sort_value, human, ignore_unavailable, include_repository, index_details, index_names, master_timeout, offset, order, pretty, repository, size, slm_policy_filter, snapshot, sort, state, verbose] 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.rest.BaseRestHandler.assertConsumesSupportedParams(BaseRestHandler.java:158) 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.rest.BaseRestHandler.handleRequest(BaseRestHandler.java:101) 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.rest.RestController$1.onResponse(RestController.java:468) 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.rest.RestController$1.onResponse(RestController.java:462) 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.security@9.1.0-SNAPSHOT/org.elasticsearch.xpack.security.rest.SecurityRestFilter.intercept(SecurityRestFilter.java:69) 2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.rest.RestController.dispatchRequest(RestController.java:462)

I removed this if block and added exclusion support for response parameters in BaseRestHandler.assertConsumesSupportedParams(), 627c9cc.

I think it's substantial change in our API design - relaxing unconsumed parameters check. Do we need this?

Right yeah this AssertionError is there to catch discrepancies between the parameters we claim to consume and the ones we actually consume. We definitely shouldn't be working around it like this. But the assertion itself seems buggy:

2025-06-12T14:37:37.881-0400 [DEBUG] [TestEventLogger] Caused by: java.lang.AssertionError: get_snapshots_action: consumed params [after, error_trace, filter_path, format, from_sort_value, human, ignore_unavailable, index_names, master_timeout, offset, order, pretty, repository, size, slm_policy_filter, snapshot, sort, state, verbose] while supporting [after, error_trace, filter_path, format, from_sort_value, human, ignore_unavailable, include_repository, index_details, index_names, master_timeout, offset, order, pretty, repository, size, slm_policy_filter, snapshot, sort, state, verbose]

Note that the missing ones are include_repository and index_details which are response parameters and therefore aren't expected to be consumed in prepareRequest. So I think we should fix the assertion.

We must be very careful with code that branches on Assertions.ENABLED because it means that the tests are not testing the exact production behaviour. Sometimes that's what we want, but not in this case.

@DaveCTurner - In 627c9cc I added the exclusion for the response parameters in BaseRestHandler.assertConsumesSupportedParams() and removed the if block. Are you looking for a different approach for fixing the assertion?

Ah sorry I missed that you'd made this change. Let me look again.

…states is not empty in validate()

DaveCTurner · 2025-06-13T13:28:23Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+            // Add exclusions for response parameters since they may not always be consumed for every request.
+            final var unconsumedResponseParams = new HashSet<>(responseParams(request.getRestApiVersion()));
+            unconsumedResponseParams.removeAll(consumed);
+            supportedAndCommon.removeAll(unconsumedResponseParams);


I think this'll permit a case where we declare a response parameter but don't include it in the supported parameters, which would be a bug. Could we instead count the response parameters in consumed (they're implicitly consumed since they're made available to the response rendering)?

DaveCTurner

LGTM (one optional nit)

DaveCTurner · 2025-06-16T08:46:33Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java

+import java.util.Set;
+
+public class GetSnapshotsFeatures implements FeatureSpecification {
+    public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter");


nit: we don't really have a formal convention for the names of these features but informally we seem to be converging on hierarchical.with.dot.delimiter like with setting names. So I think I'd have used snapshots.get.state_parameter here (also needs adjusting elsewhere in this PR)

Suggested change

public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter");

public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("snapshots.get.state_parameter");

ElenaStoeva and others added 2 commits May 29, 2025 14:50

Add state query param to Get snapshots API

b5b877e

Address code review comments from PR 123618

442b5a6

JeremyDahlgren added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. v9.1.0 labels May 29, 2025

Update docs/changelog/128635.yaml

04fcc55

JeremyDahlgren changed the title ~~Add state query param to Get snapshots API~~ May 29, 2025

JeremyDahlgren requested a review from DaveCTurner May 29, 2025 19:53

JeremyDahlgren marked this pull request as ready for review May 29, 2025 19:56

JeremyDahlgren added 11 commits May 30, 2025 11:38

Merge branch 'main' into fix/es-11525

44f5992

Merge branch 'main' into fix/es-11525

a05e099

Use test_runner_features: capabilities in new yaml test

6a560fa

Merge branch 'main' into fix/es-11525

b78ed72

Merge branch 'main' into fix/es-11525

c2d8c72

Add cluster_features clause to new yaml test to check for new NodeFea…

8bb5b22

…ture

Merge branch 'main' into fix/es-11525

95b0fc5

Merge branch 'main' into fix/es-11525

6883acb

Merge branch 'main' into fix/es-11525

e9d6f25

Merge branch 'main' into fix/es-11525

8450318

Merge branch 'main' into fix/es-11525

419f234

mhl-b reviewed Jun 11, 2025

View reviewed changes

Merge branch 'main' into fix/es-11525

a3039f0

mhl-b reviewed Jun 11, 2025

View reviewed changes

DaveCTurner reviewed Jun 12, 2025

View reviewed changes

JeremyDahlgren added 3 commits June 12, 2025 14:23

Merge branch 'main' into fix/es-11525

25da78e

Add handling for null (missing) and present but empty state param values

64d3729

Use IllegalArgumentException when state is used in mixed cluster

548fe0e

JeremyDahlgren added 2 commits June 12, 2025 17:42

Remove unnecessary check for states.isEmpty() in predicates, enforce …

3ed9350

…states is not empty in validate()

Add exclusions for unconsumed response parameters

627c9cc

DaveCTurner reviewed Jun 13, 2025

View reviewed changes

JeremyDahlgren added 2 commits June 13, 2025 10:58

Add response params to consumed before assertion check, adjust comment

0656d9e

Merge branch 'main' into fix/es-11525

56f4f44

DaveCTurner approved these changes Jun 16, 2025

View reviewed changes

JeremyDahlgren added 3 commits June 16, 2025 12:05

Merge branch 'main' into fix/es-11525

757726b

Change the NodeFeature to snapshots.get.state_parameter

f8627ba

Merge branch 'main' into fix/es-11525

644bbc5

JeremyDahlgren merged commit d43198e into elastic:main Jun 16, 2025
24 of 26 checks passed

ElenaStoeva mentioned this pull request Jun 17, 2025

Add state query param to Get snapshots API #123618

Closed

pquentin mentioned this pull request Jun 17, 2025

Update rest-api-spec main elastic/elasticsearch-specification#4594

Merged

JeremyDahlgren mentioned this pull request Jun 17, 2025

Add GET snapshots state query parameter elastic/elasticsearch-specification#4597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `state` query param to GET snapshots API#128635

Add `state` query param to GET snapshots API#128635
JeremyDahlgren merged 25 commits intoelastic:mainfrom
JeremyDahlgren:fix/es-11525

JeremyDahlgren commented May 29, 2025 •

edited

Loading

elasticsearchmachine commented May 29, 2025

elasticsearchmachine commented May 29, 2025

mhl-b Jun 11, 2025

DaveCTurner Jun 12, 2025

JeremyDahlgren Jun 12, 2025

Uh oh!

Uh oh!

mhl-b Jun 11, 2025 •

edited

Loading

mhl-b Jun 11, 2025 •

edited

Loading

JeremyDahlgren Jun 12, 2025

DaveCTurner Jun 12, 2025

DaveCTurner Jun 12, 2025

Uh oh!

Uh oh!

DaveCTurner Jun 12, 2025

JeremyDahlgren Jun 12, 2025

DaveCTurner Jun 12, 2025

JeremyDahlgren Jun 12, 2025

mhl-b Jun 13, 2025 •

edited

Loading

DaveCTurner Jun 13, 2025

JeremyDahlgren Jun 13, 2025

DaveCTurner Jun 13, 2025

DaveCTurner Jun 13, 2025

DaveCTurner left a comment

DaveCTurner Jun 16, 2025

Uh oh!

Labels

5 participants


		final var details = repositoryData.getSnapshotDetails(snapshotId);

		if (states.isEmpty() == false

	public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter");
	public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("snapshots.get.state_parameter");

Conversation

JeremyDahlgren commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎯 Motivation & Use Case

✅ Summary of Changes

elasticsearchmachine commented May 29, 2025

elasticsearchmachine commented May 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhl-b Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mhl-b Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhl-b Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

5 participants

JeremyDahlgren commented May 29, 2025 •

edited

Loading

mhl-b Jun 11, 2025 •

edited

Loading

mhl-b Jun 11, 2025 •

edited

Loading

mhl-b Jun 13, 2025 •

edited

Loading