[ML] Improving existing job check in anomaly detection wizard (#87674) (

#88457) * [ML] Improving existing job check in anomaly detection wizard * fixing job id validation * allow group ids to be reused * updating module exists endpoint * fixing issuse with job without group list * fixing test and translation ids * fixing validator when model plot is disabled * changes based on review * adding group id check to edit job flyout * small refactor and fixing edit job issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
elastic · Jan 15, 2021 · 1d4d3f3 · 1d4d3f3
1 parent 341e103
commit 1d4d3f3
Show file tree

Hide file tree

Showing 15 changed files with 252 additions and 129 deletions.
diff --git a/x-pack/plugins/ml/common/types/job_service.ts b/x-pack/plugins/ml/common/types/job_service.ts
@@ -0,0 +1,24 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { Job, JobStats } from './anomaly_detection_jobs';
+
+export interface MlJobsResponse {
+  jobs: Job[];
+  count: number;
+}
+
+export interface MlJobsStatsResponse {
+  jobs: JobStats[];
+  count: number;
+}
+
+export interface JobsExistResponse {
+  [jobId: string]: {
+    exists: boolean;
+    isGroup: boolean;
+  };
+}
diff --git a/...lugins/ml/public/application/jobs/jobs_list/components/edit_job_flyout/edit_job_flyout.js b/...lugins/ml/public/application/jobs/jobs_list/components/edit_job_flyout/edit_job_flyout.js
@@ -29,6 +29,7 @@ import { saveJob } from './edit_utils';
 import { loadFullJob } from '../utils';
 import { validateModelMemoryLimit, validateGroupNames, isValidCustomUrls } from '../validate_job';
 import { toastNotificationServiceProvider } from '../../../../services/toast_notification_service';
+import { ml } from '../../../../services/ml_api_service';
 import { withKibana } from '../../../../../../../../../src/plugins/kibana_react/public';
 import { collapseLiteralStrings } from '../../../../../../shared_imports';
 import { DATAFEED_STATE } from '../../../../../../common/constants/states';
@@ -195,16 +196,24 @@ export class EditJobFlyoutUI extends Component {
     }
 
     if (jobDetails.jobGroups !== undefined) {
-      if (jobDetails.jobGroups.some((j) => this.props.allJobIds.includes(j))) {
-        jobGroupsValidationError = i18n.translate(
-          'xpack.ml.jobsList.editJobFlyout.groupsAndJobsHasSameIdErrorMessage',
-          {
-            defaultMessage:
-              'A job with this ID already exists. Groups and jobs cannot use the same ID.',
+      jobGroupsValidationError = validateGroupNames(jobDetails.jobGroups).message;
+      if (jobGroupsValidationError === '') {
+        ml.jobs.jobsExist(jobDetails.jobGroups, true).then((resp) => {
+          const groups = Object.values(resp);
+          const valid = groups.some((g) => g.exists === true && g.isGroup === false) === false;
+          if (valid === false) {
+            this.setState({
+              jobGroupsValidationError: i18n.translate(
+                'xpack.ml.jobsList.editJobFlyout.groupsAndJobsHasSameIdErrorMessage',
+                {
+                  defaultMessage:
+                    'A job with this ID already exists. Groups and jobs cannot use the same ID.',
+                }
+              ),
+              isValidJobDetails: false,
+            });
           }
-        );
-      } else {
-        jobGroupsValidationError = validateGroupNames(jobDetails.jobGroups).message;
+        });
       }
     }
 

diff --git a/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/job_validator.ts b/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/job_validator.ts
@@ -14,9 +14,15 @@ import {
 } from '../../../../../../common/util/job_utils';
 import { getNewJobLimits } from '../../../../services/ml_server_info';
 import { JobCreator, JobCreatorType, isCategorizationJobCreator } from '../job_creator';
-import { populateValidationMessages, checkForExistingJobAndGroupIds } from './util';
-import { ExistingJobsAndGroups } from '../../../../services/job_service';
-import { cardinalityValidator, CardinalityValidatorResult } from './validators';
+import { populateValidationMessages } from './util';
+import {
+  cardinalityValidator,
+  CardinalityValidatorResult,
+  jobIdValidator,
+  groupIdsValidator,
+  JobExistsResult,
+  GroupsExistResult,
+} from './validators';
 import { CATEGORY_EXAMPLES_VALIDATION_STATUS } from '../../../../../../common/constants/categorization_job';
 import { JOB_TYPE } from '../../../../../../common/constants/new_job';
 
@@ -25,7 +31,9 @@ import { JOB_TYPE } from '../../../../../../common/constants/new_job';
 // after every keystroke
 export const VALIDATION_DELAY_MS = 500;
 
-type AsyncValidatorsResult = Partial<CardinalityValidatorResult>;
+type AsyncValidatorsResult = Partial<
+  CardinalityValidatorResult & JobExistsResult & GroupsExistResult
+>;
 
 /**
  * Union of possible validation results.
@@ -69,7 +77,6 @@ export class JobValidator {
   private _validateTimeout: ReturnType<typeof setTimeout> | null = null;
   private _asyncValidators$: Array<Observable<AsyncValidatorsResult>> = [];
   private _asyncValidatorsResult$: Observable<AsyncValidatorsResult>;
-  private _existingJobsAndGroups: ExistingJobsAndGroups;
   private _basicValidations: BasicValidations = {
     jobId: { valid: true },
     groupIds: { valid: true },
@@ -97,17 +104,20 @@ export class JobValidator {
    */
   public validationResult$: Observable<JobValidationResult>;
 
-  constructor(jobCreator: JobCreatorType, existingJobsAndGroups: ExistingJobsAndGroups) {
+  constructor(jobCreator: JobCreatorType) {
     this._jobCreator = jobCreator;
     this._lastJobConfig = this._jobCreator.formattedJobJson;
     this._lastDatafeedConfig = this._jobCreator.formattedDatafeedJson;
     this._validationSummary = {
       basic: false,
       advanced: false,
     };
-    this._existingJobsAndGroups = existingJobsAndGroups;
 
-    this._asyncValidators$ = [cardinalityValidator(this._jobCreatorSubject$)];
+    this._asyncValidators$ = [
+      cardinalityValidator(this._jobCreatorSubject$),
+      jobIdValidator(this._jobCreatorSubject$),
+      groupIdsValidator(this._jobCreatorSubject$),
+    ];
 
     this._asyncValidatorsResult$ = combineLatest(this._asyncValidators$).pipe(
       map((res) => {
@@ -208,14 +218,6 @@ export class JobValidator {
       datafeedConfig
     );
 
-    // run addition job and group id validation
-    const idResults = checkForExistingJobAndGroupIds(
-      this._jobCreator.jobId,
-      this._jobCreator.groups,
-      this._existingJobsAndGroups
-    );
-    populateValidationMessages(idResults, this._basicValidations, jobConfig, datafeedConfig);
-
     this._validationSummary.basic = this._isOverallBasicValid();
     // Update validation results subject
     this._basicValidationResult$.next(this._basicValidations);

diff --git a/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/util.ts b/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/util.ts
@@ -13,8 +13,6 @@ import {
 } from '../../../../../../common/constants/validation';
 import { getNewJobLimits } from '../../../../services/ml_server_info';
 import { ValidationResults } from '../../../../../../common/util/job_utils';
-import { ExistingJobsAndGroups } from '../../../../services/job_service';
-import { JobValidationMessage } from '../../../../../../common/constants/messages';
 
 export function populateValidationMessages(
   validationResults: ValidationResults,
@@ -204,36 +202,6 @@ export function populateValidationMessages(
   }
 }
 
-export function checkForExistingJobAndGroupIds(
-  jobId: string,
-  groupIds: string[],
-  existingJobsAndGroups: ExistingJobsAndGroups
-): ValidationResults {
-  const messages: JobValidationMessage[] = [];
-
-  // check that job id does not already exist as a job or group or a newly created group
-  if (
-    existingJobsAndGroups.jobIds.includes(jobId) ||
-    existingJobsAndGroups.groupIds.includes(jobId) ||
-    groupIds.includes(jobId)
-  ) {
-    messages.push({ id: 'job_id_already_exists' });
-  }
-
-  // check that groups that have been newly added in this job do not already exist as job ids
-  const newGroups = groupIds.filter((g) => !existingJobsAndGroups.groupIds.includes(g));
-  if (existingJobsAndGroups.jobIds.some((g) => newGroups.includes(g))) {
-    messages.push({ id: 'job_group_id_already_exists' });
-  }
-
-  return {
-    messages,
-    valid: messages.length === 0,
-    contains: (id: string) => messages.some((m) => id === m.id),
-    find: (id: string) => messages.find((m) => id === m.id),
-  };
-}
-
 function invalidTimeIntervalMessage(value: string | undefined) {
   return i18n.translate(
     'xpack.ml.newJob.wizard.validateJob.frequencyInvalidTimeIntervalFormatErrorMessage',

diff --git a/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/validators.ts b/x-pack/plugins/ml/public/application/jobs/new_job/common/job_validator/validators.ts
@@ -4,15 +4,17 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { distinctUntilChanged, filter, map, switchMap } from 'rxjs/operators';
-import { Observable, Subject } from 'rxjs';
+import { i18n } from '@kbn/i18n';
+import { distinctUntilChanged, filter, map, pluck, switchMap, startWith } from 'rxjs/operators';
+import { combineLatest, Observable, Subject } from 'rxjs';
 import {
   CardinalityModelPlotHigh,
   CardinalityValidationResult,
   ml,
 } from '../../../../services/ml_api_service';
 import { JobCreator } from '../job_creator';
 import { CombinedJob } from '../../../../../../common/types/anomaly_detection_jobs';
+import { BasicValidations } from './job_validator';
 
 export enum VALIDATOR_SEVERITY {
   ERROR,
@@ -26,8 +28,30 @@ export interface CardinalityValidatorError {
   };
 }
 
+const jobExistsErrorMessage = i18n.translate(
+  'xpack.ml.newJob.wizard.validateJob.asyncJobNameAlreadyExists',
+  {
+    defaultMessage:
+      'Job ID already exists. A job ID cannot be the same as an existing job or group.',
+  }
+);
+const groupExistsErrorMessage = i18n.translate(
+  'xpack.ml.newJob.wizard.validateJob.asyncGroupNameAlreadyExists',
+  {
+    defaultMessage:
+      'Group ID already exists. A group ID cannot be the same as an existing group or job.',
+  }
+);
+
 export type CardinalityValidatorResult = CardinalityValidatorError | null;
 
+export type JobExistsResult = {
+  jobIdExists: BasicValidations['jobId'];
+} | null;
+export type GroupsExistResult = {
+  groupIdsExist: BasicValidations['groupIds'];
+} | null;
+
 export function isCardinalityModelPlotHigh(
   cardinalityValidationResult: CardinalityValidationResult
 ): cardinalityValidationResult is CardinalityModelPlotHigh {
@@ -39,39 +63,95 @@ export function isCardinalityModelPlotHigh(
 export function cardinalityValidator(
   jobCreator$: Subject<JobCreator>
 ): Observable<CardinalityValidatorResult> {
+  return combineLatest([
+    jobCreator$.pipe(pluck('modelPlot')),
+    jobCreator$.pipe(
+      filter((jobCreator) => {
+        return jobCreator?.modelPlot;
+      }),
+      map((jobCreator) => {
+        return {
+          jobCreator,
+          analysisConfigString: JSON.stringify(jobCreator.jobConfig.analysis_config, null, 2),
+        };
+      }),
+      distinctUntilChanged((prev, curr) => {
+        return prev.analysisConfigString === curr.analysisConfigString;
+      }),
+      switchMap(({ jobCreator }) => {
+        // Perform a cardinality check only with enabled model plot.
+        return ml
+          .validateCardinality$({
+            ...jobCreator.jobConfig,
+            datafeed_config: jobCreator.datafeedConfig,
+          } as CombinedJob)
+          .pipe(
+            map((validationResults) => {
+              for (const validationResult of validationResults) {
+                if (isCardinalityModelPlotHigh(validationResult)) {
+                  return {
+                    highCardinality: {
+                      value: validationResult.modelPlotCardinality,
+                      severity: VALIDATOR_SEVERITY.WARNING,
+                    },
+                  };
+                }
+              }
+              return null;
+            })
+          );
+      }),
+      startWith(null)
+    ),
+  ]).pipe(
+    map(([isModelPlotEnabled, cardinalityValidationResult]) => {
+      return isModelPlotEnabled ? cardinalityValidationResult : null;
+    })
+  );
+}
+
+export function jobIdValidator(jobCreator$: Subject<JobCreator>): Observable<JobExistsResult> {
   return jobCreator$.pipe(
-    // Perform a cardinality check only with enabled model plot.
-    filter((jobCreator) => {
-      return jobCreator?.modelPlot;
-    }),
     map((jobCreator) => {
+      return jobCreator.jobId;
+    }),
+    // No need to perform an API call if the analysis configuration hasn't been changed
+    distinctUntilChanged((prevJobId, currJobId) => prevJobId === currJobId),
+    switchMap((jobId) => ml.jobs.jobsExist$([jobId], true)),
+    map((jobExistsResults) => {
+      const jobs = Object.values(jobExistsResults);
+      const valid = jobs?.[0].exists === false;
       return {
-        jobCreator,
-        analysisConfigString: JSON.stringify(jobCreator.jobConfig.analysis_config),
+        jobIdExists: {
+          valid,
+          ...(valid ? {} : { message: jobExistsErrorMessage }),
+        },
       };
-    }),
+    })
+  );
+}
+
+export function groupIdsValidator(jobCreator$: Subject<JobCreator>): Observable<GroupsExistResult> {
+  return jobCreator$.pipe(
+    map((jobCreator) => jobCreator.groups),
     // No need to perform an API call if the analysis configuration hasn't been changed
-    distinctUntilChanged((prev, curr) => {
-      return prev.analysisConfigString === curr.analysisConfigString;
-    }),
-    switchMap(({ jobCreator }) => {
-      return ml.validateCardinality$({
-        ...jobCreator.jobConfig,
-        datafeed_config: jobCreator.datafeedConfig,
-      } as CombinedJob);
+    distinctUntilChanged(
+      (prevGroups, currGroups) => JSON.stringify(prevGroups) === JSON.stringify(currGroups)
+    ),
+    switchMap((groups) => {
+      return ml.jobs.jobsExist$(groups, true);
     }),
-    map((validationResults) => {
-      for (const validationResult of validationResults) {
-        if (isCardinalityModelPlotHigh(validationResult)) {
-          return {
-            highCardinality: {
-              value: validationResult.modelPlotCardinality,
-              severity: VALIDATOR_SEVERITY.WARNING,
-            },
-          };
-        }
-      }
-      return null;
+    map((jobExistsResults) => {
+      const groups = Object.values(jobExistsResults);
+      // only match jobs that exist but aren't groups.
+      // as we should allow existing groups to be reused.
+      const valid = groups.some((g) => g.exists === true && g.isGroup === false) === false;
+      return {
+        groupIdsExist: {
+          valid,
+          ...(valid ? {} : { message: groupExistsErrorMessage }),
+        },
+      };
     })
   );
 }