[ML] Remove get model snapshot by description functionality (elastic/x-pack-elasticsearch#1288)

relates elastic/x-pack-elasticsearch#1284

Original commit: elastic/x-pack-elasticsearch@780feea5c1

docs/en/rest-api/ml/get-snapshot.asciidoc

@@ -27,11 +27,6 @@ The get model snapshots API enables you to retrieve information about model snap
 `desc`::
   (boolean) If true, the results are sorted in descending order.
 
-`description`::
-  (string) Returns snapshots that match this description.
-//TBD: I couldn't get this to work. What description value is it using?
-NOTE: It might be necessary to URL encode the description.
-
 `end`::
   (date) Returns snapshots with timestamps earlier than this time.
 
@@ -43,7 +38,7 @@ NOTE: It might be necessary to URL encode the description.
 
 `sort`::
   (string) Specifies the sort field for the requested snapshots.
-//By default, the snapshots are sorted by the xxx value.
+  By default, the snapshots are sorted by their timestamp.
 
 `start`::
   (string) Returns snapshots with timestamps after this time.

plugin/src/main/java/org/elasticsearch/xpack/ml/action/DeleteModelSnapshotAction.java

@@ -153,7 +153,7 @@ public class DeleteModelSnapshotAction extends Action<DeleteModelSnapshotAction.
         protected void doExecute(Request request, ActionListener<Response> listener) {
             // Verify the snapshot exists
             jobProvider.modelSnapshots(
-                    request.getJobId(), 0, 1, null, null, null, true, request.getSnapshotId(), null,
+                    request.getJobId(), 0, 1, null, null, null, true, request.getSnapshotId(),
                     page -> {
                         List<ModelSnapshot> deleteCandidates = page.results();
                         if (deleteCandidates.size() > 1) {

plugin/src/main/java/org/elasticsearch/xpack/ml/action/GetModelSnapshotsAction.java

@@ -65,7 +65,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
 
         public static final ParseField SNAPSHOT_ID = new ParseField("snapshot_id");
         public static final ParseField SORT = new ParseField("sort");
-        public static final ParseField DESCRIPTION = new ParseField("description");
         public static final ParseField START = new ParseField("start");
         public static final ParseField END = new ParseField("end");
         public static final ParseField DESC = new ParseField("desc");
@@ -75,7 +74,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
         static {
             PARSER.declareString((request, jobId) -> request.jobId = jobId, Job.ID);
             PARSER.declareString((request, snapshotId) -> request.snapshotId = snapshotId, SNAPSHOT_ID);
-            PARSER.declareString(Request::setDescriptionString, DESCRIPTION);
             PARSER.declareString(Request::setStart, START);
             PARSER.declareString(Request::setEnd, END);
             PARSER.declareString(Request::setSort, SORT);
@@ -97,7 +95,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
         private String jobId;
         private String snapshotId;
         private String sort;
-        private String description;
         private String start;
         private String end;
         private boolean desc;
@@ -163,15 +160,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
             this.end = ExceptionsHelper.requireNonNull(end, END.getPreferredName());
         }
 
-        @Nullable
-        public String getDescriptionString() {
-            return description;
-        }
-
-        public void setDescriptionString(String description) {
-            this.description = ExceptionsHelper.requireNonNull(description, DESCRIPTION.getPreferredName());
-        }
-
         @Override
         public ActionRequestValidationException validate() {
             return null;
@@ -183,7 +171,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
             jobId = in.readString();
             snapshotId = in.readOptionalString();
             sort = in.readOptionalString();
-            description = in.readOptionalString();
             start = in.readOptionalString();
             end = in.readOptionalString();
             desc = in.readBoolean();
@@ -196,7 +183,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
             out.writeString(jobId);
             out.writeOptionalString(snapshotId);
             out.writeOptionalString(sort);
-            out.writeOptionalString(description);
             out.writeOptionalString(start);
             out.writeOptionalString(end);
             out.writeBoolean(desc);
@@ -210,9 +196,6 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
             if (snapshotId != null) {
                 builder.field(SNAPSHOT_ID.getPreferredName(), snapshotId);
             }
-            if (description != null) {
-                builder.field(DESCRIPTION.getPreferredName(), description);
-            }
             if (start != null) {
                 builder.field(START.getPreferredName(), start);
             }
@@ -230,7 +213,7 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
 
         @Override
         public int hashCode() {
-            return Objects.hash(jobId, snapshotId, description, start, end, sort, desc);
+            return Objects.hash(jobId, snapshotId, start, end, sort, desc);
         }
 
         @Override
@@ -242,9 +225,12 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
                 return false;
             }
             Request other = (Request) obj;
-            return Objects.equals(jobId, other.jobId) && Objects.equals(snapshotId, other.snapshotId)
-                    && Objects.equals(description, other.description) && Objects.equals(start, other.start)
-                    && Objects.equals(end, other.end) && Objects.equals(sort, other.sort) && Objects.equals(desc, other.desc);
+            return Objects.equals(jobId, other.jobId)
+                    && Objects.equals(snapshotId, other.snapshotId)
+                    && Objects.equals(start, other.start)
+                    && Objects.equals(end, other.end)
+                    && Objects.equals(sort, other.sort)
+                    && Objects.equals(desc, other.desc);
         }
     }
 
@@ -329,15 +315,14 @@ extends Action<GetModelSnapshotsAction.Request, GetModelSnapshotsAction.Response
         @Override
         protected void doExecute(Request request, ActionListener<Response> listener) {
             logger.debug("Get model snapshots for job {} snapshot ID {}. from = {}, size = {}"
-                    + " start = '{}', end='{}', sort={} descending={}, description filter={}",
+                    + " start = '{}', end='{}', sort={} descending={}",
                     request.getJobId(), request.getSnapshotId(), request.pageParams.getFrom(), request.pageParams.getSize(),
-                    request.getStart(), request.getEnd(), request.getSort(), request.getDescOrder(), request.getDescriptionString());
+                    request.getStart(), request.getEnd(), request.getSort(), request.getDescOrder());
 
             jobManager.getJobOrThrowIfUnknown(request.getJobId());
 
             jobProvider.modelSnapshots(request.getJobId(), request.pageParams.getFrom(), request.pageParams.getSize(),
                     request.getStart(), request.getEnd(), request.getSort(), request.getDescOrder(), request.getSnapshotId(),
-                    request.getDescriptionString(),
                     page -> {
                         listener.onResponse(new Response(clearQuantiles(page)));
                     }, listener::onFailure);

plugin/src/main/java/org/elasticsearch/xpack/ml/action/UpdateModelSnapshotAction.java

@@ -38,9 +38,7 @@ import org.elasticsearch.xpack.ml.job.process.autodetect.state.ModelSnapshot;
 import org.elasticsearch.xpack.ml.utils.ExceptionsHelper;
 
 import java.io.IOException;
-import java.util.List;
 import java.util.Objects;
-import java.util.function.Consumer;
 
 public class UpdateModelSnapshotAction extends Action<UpdateModelSnapshotAction.Request,
         UpdateModelSnapshotAction.Response, UpdateModelSnapshotAction.RequestBuilder> {
@@ -272,54 +270,19 @@ public class UpdateModelSnapshotAction extends Action<UpdateModelSnapshotAction.
         @Override
         protected void doExecute(Request request, ActionListener<Response> listener) {
             logger.debug("Received request to update model snapshot [{}] for job [{}]", request.getSnapshotId(), request.getJobId());
-            getChangeCandidates(request, changeCandidates -> {
-                checkForClashes(request, aVoid -> {
-                    if (changeCandidates.size() > 1) {
-                        logger.warn("More than one model found for [{}: {}, {}: {}] tuple.", Job.ID.getPreferredName(), request.getJobId(),
-                                ModelSnapshot.SNAPSHOT_ID.getPreferredName(), request.getSnapshotId());
-                    }
-                    ModelSnapshot updatedSnapshot = applyUpdate(request, changeCandidates.get(0));
+            jobProvider.getModelSnapshot(request.getJobId(), request.getSnapshotId(), modelSnapshot -> {
+                if (modelSnapshot == null) {
+                    listener.onFailure(new ResourceNotFoundException(Messages.getMessage(
+                            Messages.REST_NO_SUCH_MODEL_SNAPSHOT, request.getSnapshotId(), request.getJobId())));
+                } else {
+                    ModelSnapshot updatedSnapshot = applyUpdate(request, modelSnapshot);
                     jobManager.updateModelSnapshot(updatedSnapshot, b -> {
                         // The quantiles can be large, and totally dominate the output -
                         // it's clearer to remove them
                         listener.onResponse(new Response(new ModelSnapshot.Builder(updatedSnapshot).setQuantiles(null).build()));
                     }, listener::onFailure);
-                }, listener::onFailure);
-            }, listener::onFailure);
-        }
-
-        private void getChangeCandidates(Request request, Consumer<List<ModelSnapshot>> handler, Consumer<Exception> errorHandler) {
-            getModelSnapshots(request.getJobId(), request.getSnapshotId(), null,
-                    changeCandidates -> {
-                        if (changeCandidates == null || changeCandidates.isEmpty()) {
-                            errorHandler.accept(new ResourceNotFoundException(Messages.getMessage(Messages.REST_NO_SUCH_MODEL_SNAPSHOT,
-                                    request.getSnapshotId(), request.getJobId())));
-                        } else {
-                            handler.accept(changeCandidates);
-                        }
-                    }, errorHandler);
-        }
-
-        private void checkForClashes(Request request, Consumer<Void> handler, Consumer<Exception> errorHandler) {
-            if (request.getDescription() == null) {
-                handler.accept(null);
-                return;
-            }
-
-            getModelSnapshots(request.getJobId(), null, request.getDescription(), clashCandidates -> {
-                if (clashCandidates != null && !clashCandidates.isEmpty()) {
-                    errorHandler.accept(new IllegalArgumentException(Messages.getMessage(
-                            Messages.REST_DESCRIPTION_ALREADY_USED, request.getDescription(), request.getJobId())));
-                } else {
-                    handler.accept(null);
                 }
-            }, errorHandler);
-        }
-
-        private void getModelSnapshots(String jobId, String snapshotId, String description,
-                                       Consumer<List<ModelSnapshot>> handler, Consumer<Exception> errorHandler) {
-            jobProvider.modelSnapshots(jobId, 0, 1, null, null, null, true, snapshotId, description,
-                    page -> handler.accept(page.results()), errorHandler);
+            }, listener::onFailure);
         }
 
         private static ModelSnapshot applyUpdate(Request request, ModelSnapshot target) {
@@ -332,7 +295,5 @@ public class UpdateModelSnapshotAction extends Action<UpdateModelSnapshotAction.
             }
             return updatedSnapshotBuilder.build();
         }
-
     }
-
 }

plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java

@@ -135,7 +135,6 @@ public final class Messages {
 
     public static final String REST_CANNOT_DELETE_HIGHEST_PRIORITY =
             "Model snapshot ''{0}'' is the active snapshot for job ''{1}'', so cannot be deleted";
-    public static final String REST_DESCRIPTION_ALREADY_USED = "Model snapshot description ''{0}'' has already been used for job ''{1}''";
     public static final String REST_INVALID_DATETIME_PARAMS =
             "Query param ''{0}'' with value ''{1}'' cannot be parsed as a date or converted to a number (epoch).";
     public static final String REST_INVALID_FLUSH_PARAMS_MISSING = "Invalid flush parameters: ''{0}'' has not been specified.";

plugin/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java

@@ -796,7 +796,6 @@ public class JobProvider {
      * @param sortField      optional sort field name (may be null)
      * @param sortDescending Sort in descending order
      * @param snapshotId     optional snapshot ID to match (null for all)
-     * @param description    optional description to match (null for all)
      */
     public void modelSnapshots(String jobId,
                                int from,
@@ -806,24 +805,11 @@ public class JobProvider {
                                String sortField,
                                boolean sortDescending,
                                String snapshotId,
-                               String description,
                                Consumer<QueryPage<ModelSnapshot>> handler,
                                Consumer<Exception> errorHandler) {
-        boolean haveId = snapshotId != null && !snapshotId.isEmpty();
-        boolean haveDescription = description != null && !description.isEmpty();
-        ResultsFilterBuilder fb;
-        if (haveId || haveDescription) {
-            BoolQueryBuilder query = QueryBuilders.boolQuery();
-            if (haveId) {
-                query.filter(QueryBuilders.termQuery(ModelSnapshot.SNAPSHOT_ID.getPreferredName(), snapshotId));
-            }
-            if (haveDescription) {
-                query.filter(QueryBuilders.termQuery(ModelSnapshot.DESCRIPTION.getPreferredName(), description));
-            }
-
-            fb = new ResultsFilterBuilder(query);
-        } else {
-            fb = new ResultsFilterBuilder();
+        ResultsFilterBuilder fb = new ResultsFilterBuilder();
+        if (snapshotId != null && !snapshotId.isEmpty()) {
+            fb.term(ModelSnapshot.SNAPSHOT_ID.getPreferredName(), snapshotId);
         }
 
         QueryBuilder qb = fb.timeRange(Result.TIMESTAMP.getPreferredName(), startEpochMs, endEpochMs).build();

plugin/src/main/java/org/elasticsearch/xpack/ml/rest/modelsnapshots/RestGetModelSnapshotsAction.java

@@ -15,8 +15,8 @@ import org.elasticsearch.rest.action.RestToXContentListener;
 import org.elasticsearch.xpack.ml.MachineLearning;
 import org.elasticsearch.xpack.ml.action.GetModelSnapshotsAction;
 import org.elasticsearch.xpack.ml.action.GetModelSnapshotsAction.Request;
-import org.elasticsearch.xpack.ml.job.config.Job;
 import org.elasticsearch.xpack.ml.action.util.PageParams;
+import org.elasticsearch.xpack.ml.job.config.Job;
 
 import java.io.IOException;
 
@@ -68,9 +68,6 @@ public class RestGetModelSnapshotsAction extends BaseRestHandler {
             if (restRequest.hasParam(Request.END.getPreferredName())) {
                 getModelSnapshots.setEnd(restRequest.param(Request.END.getPreferredName(), DEFAULT_END));
             }
-            if (restRequest.hasParam(Request.DESCRIPTION.getPreferredName())) {
-                getModelSnapshots.setDescriptionString(restRequest.param(Request.DESCRIPTION.getPreferredName(), DEFAULT_DESCRIPTION));
-            }
             getModelSnapshots.setDescOrder(restRequest.paramAsBoolean(Request.DESC.getPreferredName(), DEFAULT_DESC_ORDER));
             getModelSnapshots.setPageParams(new PageParams(
                     restRequest.paramAsInt(PageParams.FROM.getPreferredName(), PageParams.DEFAULT_FROM),

plugin/src/test/java/org/elasticsearch/xpack/ml/action/GetModelSnapshotsActionRequestTests.java

@@ -20,9 +20,6 @@ public class GetModelSnapshotsActionRequestTests extends AbstractStreamableXCont
     @Override
     protected Request createTestInstance() {
         Request request = new Request(randomAlphaOfLengthBetween(1, 20), randomBoolean() ? null : randomAlphaOfLengthBetween(1, 20));
-        if (randomBoolean()) {
-            request.setDescriptionString(randomAlphaOfLengthBetween(1, 20));
-        }
         if (randomBoolean()) {
             request.setStart(randomAlphaOfLengthBetween(1, 20));
         }

plugin/src/test/java/org/elasticsearch/xpack/ml/job/persistence/JobProviderTests.java

@@ -811,60 +811,6 @@ public class JobProviderTests extends ESTestCase {
         assertEquals(6, snapshots.get(1).getSnapshotDocCount());
     }
 
-    public void testModelSnapshots_WithDescription()
-            throws InterruptedException, ExecutionException, IOException {
-        String jobId = "TestJobIdentificationForInfluencers";
-        Date now = new Date();
-        List<Map<String, Object>> source = new ArrayList<>();
-
-        Map<String, Object> recordMap1 = new HashMap<>();
-        recordMap1.put("job_id", "foo");
-        recordMap1.put("description", "snapshot1");
-        recordMap1.put("timestamp", now.getTime());
-        recordMap1.put("snapshot_doc_count", 5);
-        recordMap1.put("latest_record_time_stamp", now.getTime());
-        recordMap1.put("latest_result_time_stamp", now.getTime());
-        Map<String, Object> recordMap2 = new HashMap<>();
-        recordMap2.put("job_id", "foo");
-        recordMap2.put("description", "snapshot2");
-        recordMap2.put("timestamp", now.getTime());
-        recordMap2.put("snapshot_doc_count", 6);
-        recordMap2.put("latest_record_time_stamp", now.getTime());
-        recordMap2.put("latest_result_time_stamp", now.getTime());
-        source.add(recordMap1);
-        source.add(recordMap2);
-
-        int from = 4;
-        int size = 3;
-        QueryBuilder[] qbHolder = new QueryBuilder[1];
-        SearchResponse response = createSearchResponse(source);
-        Client client = getMockedClient(qb -> qbHolder[0] = qb, response);
-        JobProvider provider = createProvider(client);
-
-        @SuppressWarnings({"unchecked", "rawtypes"})
-        QueryPage<ModelSnapshot>[] hodor = new QueryPage[1];
-        provider.modelSnapshots(jobId, from, size, null, null, "sortfield", true, "snappyId", "description1",
-                p -> hodor[0] = p, RuntimeException::new);
-        QueryPage<ModelSnapshot> page = hodor[0];
-        assertEquals(2L, page.count());
-        List<ModelSnapshot> snapshots = page.results();
-
-        assertEquals(now, snapshots.get(0).getTimestamp());
-        assertEquals(now, snapshots.get(0).getLatestRecordTimeStamp());
-        assertEquals(now, snapshots.get(0).getLatestResultTimeStamp());
-        assertEquals("snapshot1", snapshots.get(0).getDescription());
-        assertEquals(5, snapshots.get(0).getSnapshotDocCount());
-
-        assertEquals(now, snapshots.get(1).getTimestamp());
-        assertEquals(now, snapshots.get(1).getLatestRecordTimeStamp());
-        assertEquals(now, snapshots.get(1).getLatestResultTimeStamp());
-        assertEquals("snapshot2", snapshots.get(1).getDescription());
-        assertEquals(6, snapshots.get(1).getSnapshotDocCount());
-
-        String queryString = qbHolder[0].toString();
-        assertTrue(queryString.matches("(?s).*snapshot_id.*value. : .snappyId.*description.*value. : .description1.*"));
-    }
-
     public void testRestoreStateToStream() throws Exception {
         Map<String, Object> categorizerState = new HashMap<>();
         categorizerState.put("catName", "catVal");

plugin/src/test/resources/rest-api-spec/api/xpack.ml.get_model_snapshots.json

@@ -42,10 +42,6 @@
         "desc": {
           "type": "boolean",
           "description": "True if the results should be sorted in descending order"
-        },
-        "description": {
-          "type": "string",
-          "description": "Description to filter on"
         }
       }
     },

plugin/src/test/resources/rest-api-spec/test/ml/update_model_snapshot.yaml

@@ -17,7 +17,7 @@ setup:
       index:
         index:  .ml-anomalies-update-model-snapshot
         type:   model_snapshot
-        id:     "1"
+        id:     "update-model-snapshot-snapshot-1"
         body: >
           {
             "job_id" : "update-model-snapshot",
@@ -30,7 +30,7 @@ setup:
       index:
         index:  .ml-anomalies-update-model-snapshot
         type:   model_snapshot
-        id:     "2"
+        id:     "update-model-snapshot-snapshot-2"
         body: >
           {
             "job_id": "update-model-snapshot",
@@ -49,10 +49,10 @@ setup:
   - do:
       xpack.ml.get_model_snapshots:
         job_id: "update-model-snapshot"
-        description: "new_description"
+        snapshot_id: "snapshot-1"
 
-  - match: { count: 0 }
-  - length: { model_snapshots: 0 }
+  - match: { count: 1 }
+  - is_false: model_snapshots.0.description
 
   - do:
       xpack.ml.update_model_snapshot:
@@ -74,34 +74,36 @@ setup:
   - do:
       xpack.ml.get_model_snapshots:
         job_id: "update-model-snapshot"
-        description: "new_description"
+        snapshot_id: "snapshot-1"
 
   - match: { count: 1 }
   - match: { model_snapshots.0.snapshot_id: "snapshot-1" }
   - match: { model_snapshots.0.timestamp: 1464825600000 }
 
 ---
-"Test with conflict against existing description":
+"Test duplicate descriptions are allowed":
   - do:
-      xpack.ml.get_model_snapshots:
-        job_id: "update-model-snapshot"
-        description: "2"
-
-  - match: { count: 1 }
-  - length: { model_snapshots: 1 }
-  - match: { model_snapshots.0.job_id: "update-model-snapshot" }
-  - match: { model_snapshots.0.description: "snapshot 2 description" }
-
-  - do:
-      catch: request
       xpack.ml.update_model_snapshot:
         job_id: "update-model-snapshot"
         snapshot_id: "snapshot-1"
         body: >
           {
-            "description": "snapshot"
+            "description": "snapshot 2 description"
           }
 
+  - do:
+      indices.refresh:
+        index: .ml-anomalies-update-model-snapshot
+
+  - do:
+      xpack.ml.get_model_snapshots:
+        job_id: "update-model-snapshot"
+
+  - match: { count: 2 }
+  - match: { model_snapshots.0.snapshot_id: "snapshot-1" }
+  - match: { model_snapshots.0.description: "snapshot 2 description" }
+  - match: { model_snapshots.1.snapshot_id: "snapshot-2" }
+  - match: { model_snapshots.1.description: "snapshot 2 description" }
 ---
 "Test with retain":
   - do:
@@ -157,3 +159,15 @@ setup:
             "description": "new foo",
             "retain": true
           }
+
+---
+"Test with unknown snapshot id":
+  - do:
+      catch: missing
+      xpack.ml.update_model_snapshot:
+        job_id: "update-model-snapshot"
+        snapshot_id: "snapshot-9999"
+        body: >
+          {
+            "description": "new description for snapshot 9999"
+          }