Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Enhance job id error message #45349

Merged
merged 11 commits into from
Sep 17, 2019
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/ml/common/constants/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ export enum VALIDATION_STATUS {
export const SKIP_BUCKET_SPAN_ESTIMATION = true;

export const ALLOWED_DATA_UNITS = ['B', 'KB', 'MB', 'GB', 'TB', 'PB'];

export const JOB_ID_MAX_LENGTH = 64;
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/ml/common/util/job_utils.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,5 @@ export function basicJobValidation(
export const ML_MEDIAN_PERCENTS: number;

export const ML_DATA_PREVIEW_COUNT: number;

export function isJobIdValid(jobId: string): boolean;
32 changes: 14 additions & 18 deletions x-pack/legacy/plugins/ml/common/util/job_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import _ from 'lodash';
import semver from 'semver';
import numeral from '@elastic/numeral';

import { ALLOWED_DATA_UNITS } from '../constants/validation';
import { ALLOWED_DATA_UNITS, JOB_ID_MAX_LENGTH } from '../constants/validation';
import { parseInterval } from './parse_interval';
import { maxLengthValidator } from './validators';

// work out the default frequency based on the bucket_span in seconds
export function calculateDatafeedFrequencyDefaultSeconds(bucketSpanSeconds) {
Expand Down Expand Up @@ -223,7 +224,7 @@ export function mlFunctionToESAggregation(functionName) {
// Job name must contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores;
// it must also start and end with an alphanumeric character'
export function isJobIdValid(jobId) {
return (jobId.match(/^[a-z0-9\-\_]{1,64}$/g) && !jobId.match(/^([_-].*)?(.*[_-])?$/g)) ? true : false;
return (jobId.match(/^[a-z0-9\-\_]+$/g) && !jobId.match(/^([_-].*)?(.*[_-])?$/g)) ? true : false;
}

// To get median data for jobs and charts we need to use Elasticsearch's
Expand Down Expand Up @@ -278,6 +279,9 @@ export function basicJobValidation(job, fields, limits, skipMmlChecks = false) {
} else if (isJobIdValid(job.job_id) === false) {
messages.push({ id: 'job_id_invalid' });
valid = false;
} else if (maxLengthValidator(JOB_ID_MAX_LENGTH)(job.job_id)) {
messages.push({ id: 'job_id_invalid_max_length', maxLength: JOB_ID_MAX_LENGTH });
valid = false;
} else {
messages.push({ id: 'job_id_valid' });
}
Expand Down Expand Up @@ -487,22 +491,14 @@ export function validateModelMemoryLimitUnits(job) {
}

export function validateGroupNames(job) {
const messages = [];
let valid = true;
if (job.groups !== undefined) {
let groupIdValid = true;
job.groups.forEach(group => {
if (isJobIdValid(group) === false) {
groupIdValid = false;
valid = false;
}
});
if (job.groups.length > 0 && groupIdValid) {
messages.push({ id: 'job_group_id_valid' });
} else if (job.groups.length > 0 && !groupIdValid) {
messages.push({ id: 'job_group_id_invalid' });
}
}
const { groups = [] } = job;
const errorMessages = [
...groups.some(group => !isJobIdValid(group)) ? [{ id: 'job_group_id_invalid' }] : [],
...groups.some(group => maxLengthValidator(JOB_ID_MAX_LENGTH)(group)) ? [{ id: 'job_group_id_invalid_max_length' }] : [],
];
const valid = errorMessages.length === 0;
const messages = valid ? [{ id: 'job_group_id_valid' }] : errorMessages;

return {
valid,
messages,
Expand Down
22 changes: 22 additions & 0 deletions x-pack/legacy/plugins/ml/common/util/validators.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* 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 { maxLengthValidator } from './validators';

describe('maxLengthValidator', () => {
test('should allow a valid input', () => {
expect(maxLengthValidator(2)('xx')).toBe(null);
});

test('should describe an invalid input', () => {
expect(maxLengthValidator(3)('example')).toEqual({
maxLength: {
requiredLength: 3,
actualLength: 7,
},
});
});
});
23 changes: 23 additions & 0 deletions x-pack/legacy/plugins/ml/common/util/validators.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* 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.
*/

/**
* Provides a validator function for maximum allowed input length.
* @param maxLength Maximum length allowed.
*/
export function maxLengthValidator(
Copy link
Member

@jgowdyelastic jgowdyelastic Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is now used as falsy check, the contents of the returned object are ignored.
Maybe it should be changed to just return a boolean.

Copy link
Member

@jgowdyelastic jgowdyelastic Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised that there is a string_utils.js file along side this file. IMO, this function is worth moving inside there, as it is validating the length of a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it as falsy check because of the current code structure. Validator function provides useful info to render error messages as well as error key which lets generate an object with all errors for specific field/model.

maxLength: number
): (value: string) => { maxLength: { requiredLength: number; actualLength: number } } | null {
return value =>
value && value.length > maxLength
? {
maxLength: {
requiredLength: maxLength,
actualLength: value.length,
},
}
: null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ import { BehaviorSubject } from 'rxjs';
import { filter, distinctUntilChanged } from 'rxjs/operators';
import { Subscription } from 'rxjs';

// @ts-ignore
import { isJobIdValid } from '../../../common/util/job_utils';

export const isAnalyticsIdValid = isJobIdValid;

export type IndexName = string;
export type IndexPattern = string;
export type DataFrameAnalyticsId = string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

export {
getAnalysisType,
isAnalyticsIdValid,
isOutlierAnalysis,
refreshAnalyticsList$,
useRefreshAnalyticsList,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { metadata } from 'ui/metadata';
import { INDEX_PATTERN_ILLEGAL_CHARACTERS } from 'ui/index_patterns';

import { CreateAnalyticsFormProps } from '../../hooks/use_create_analytics_form';
import { JOB_ID_MAX_LENGTH } from '../../../../../../common/constants/validation';

// based on code used by `ui/index_patterns` internally
// remove the space character from the list of illegal characters
Expand Down Expand Up @@ -53,6 +54,7 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
jobIdEmpty,
jobIdExists,
jobIdValid,
jobIdInvalidMaxLength,
sourceIndex,
sourceIndexNameEmpty,
sourceIndexNameValid,
Expand Down Expand Up @@ -106,7 +108,7 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
label={i18n.translate('xpack.ml.dataframe.analytics.create.jobIdLabel', {
defaultMessage: 'Job ID',
})}
isInvalid={(!jobIdEmpty && !jobIdValid) || jobIdExists}
isInvalid={(!jobIdEmpty && !jobIdValid) || jobIdExists || jobIdInvalidMaxLength}
error={[
...(!jobIdEmpty && !jobIdValid
? [
Expand All @@ -123,6 +125,20 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
}),
]
: []),
...(jobIdInvalidMaxLength
? [
i18n.translate(
'xpack.ml.dataframe.analytics.create.jobIdInvalidMaxLengthErrorMessage',
{
defaultMessage:
'Job ID must be no more than {maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}
),
]
: []),
]}
>
<EuiFieldText
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ import { validateIndexPattern } from 'ui/index_patterns';

import { isValidIndexName } from '../../../../../../common/util/es_utils';

import { isAnalyticsIdValid } from '../../../../common';

import { Action, ACTION } from './actions';
import { getInitialState, getJobConfigFromFormState, State } from './state';
import { isJobIdValid } from '../../../../../../common/util/job_utils';
import { maxLengthValidator } from '../../../../../../common/util/validators';
import { JOB_ID_MAX_LENGTH } from '../../../../../../common/constants/validation';

const getSourceIndexString = (state: State) => {
const { jobConfig } = state;
Expand Down Expand Up @@ -189,7 +190,10 @@ export function reducer(state: State, action: Action): State {
if (action.payload.jobId !== undefined) {
newFormState.jobIdExists = state.jobIds.some(id => newFormState.jobId === id);
newFormState.jobIdEmpty = newFormState.jobId === '';
newFormState.jobIdValid = isAnalyticsIdValid(newFormState.jobId);
newFormState.jobIdValid = isJobIdValid(newFormState.jobId);
newFormState.jobIdInvalidMaxLength = !!maxLengthValidator(JOB_ID_MAX_LENGTH)(
newFormState.jobId
);
}

if (action.payload.sourceIndex !== undefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface State {
jobId: DataFrameAnalyticsId;
jobIdExists: boolean;
jobIdEmpty: boolean;
jobIdInvalidMaxLength?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have this in line with the other attributes which are not optional. To fix it I think you just need to add a default jobIdInvalidMaxLength: false; to getInitialState().

jobIdValid: boolean;
sourceIndex: EsIndexName;
sourceIndexNameEmpty: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { i18n } from '@kbn/i18n';
import { BasicValidations } from './job_validator';
import { Job } from '../job_creator/configs';
import { ALLOWED_DATA_UNITS } from '../../../../../common/constants/validation';
import { ALLOWED_DATA_UNITS, JOB_ID_MAX_LENGTH } from '../../../../../common/constants/validation';
import { newJobLimits } from '../../../new_job/utils/new_job_defaults';
import { ValidationResults, ValidationMessage } from '../../../../../common/util/job_utils';
import { ExistingJobsAndGroups } from '../../../../services/job_service';
Expand All @@ -27,11 +27,23 @@ export function populateValidationMessages(
'xpack.ml.newJob.wizard.validateJob.jobNameAllowedCharactersDescription',
{
defaultMessage:
'Job name can contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores; ' +
'Job ID can contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores; ' +
'must start and end with an alphanumeric character',
}
);
basicValidations.jobId.message = msg;
} else if (validationResults.contains('job_id_invalid_max_length')) {
basicValidations.jobId.valid = false;
basicValidations.jobId.message = i18n.translate(
'xpack.ml.newJob.wizard.validateJob.jobIdInvalidMaxLengthErrorMessage',
{
defaultMessage:
'Job ID must be no more than {maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}
);
} else if (validationResults.contains('job_id_already_exists')) {
basicValidations.jobId.valid = false;
const msg = i18n.translate('xpack.ml.newJob.wizard.validateJob.jobNameAlreadyExists', {
Expand All @@ -52,6 +64,18 @@ export function populateValidationMessages(
}
);
basicValidations.groupIds.message = msg;
} else if (validationResults.contains('job_group_id_invalid_max_length')) {
basicValidations.groupIds.valid = false;
basicValidations.groupIds.message = i18n.translate(
'xpack.ml.newJob.wizard.validateJob.jobGroupMaxLengthDescription',
{
defaultMessage:
'Job group name must be no more than {maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}
);
} else if (validationResults.contains('job_group_id_already_exists')) {
basicValidations.groupIds.valid = false;
const msg = i18n.translate('xpack.ml.newJob.wizard.validateJob.groupNameAlreadyExists', {
Expand Down
39 changes: 34 additions & 5 deletions x-pack/legacy/plugins/ml/server/models/job_validation/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@


import { i18n } from '@kbn/i18n';
import { JOB_ID_MAX_LENGTH } from '../../../common/constants/validation';

let messages;

Expand Down Expand Up @@ -240,26 +241,40 @@ export const getMessages = () => {
job_id_empty: {
status: 'ERROR',
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdEmptyMessage', {
defaultMessage: 'The job name field must not be empty.',
defaultMessage: 'Job ID field must not be empty.',
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
job_id_invalid: {
status: 'ERROR',
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdInvalidMessage', {
defaultMessage: 'The job name is invalid. It can contain lowercase alphanumeric (a-z and 0-9) characters, ' +
defaultMessage: 'Job ID is invalid. It can contain lowercase alphanumeric (a-z and 0-9) characters, ' +
'hyphens or underscores and must start and end with an alphanumeric character.',
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
job_id_invalid_max_length: {
status: 'ERROR',
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdInvalidMaxLengthErrorMessage', {
defaultMessage: 'Job ID must be no more than {maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
job_id_valid: {
status: 'SUCCESS',
heading: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdValidHeading', {
defaultMessage: 'Job id format is valid',
defaultMessage: 'Job ID format is valid',
}),
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdValidMessage', {
defaultMessage: 'Lowercase alphanumeric (a-z and 0-9) characters, hyphens or underscores, ' +
'starts and ends with an alphanumeric character.',
'starts and ends with an alphanumeric character, and is no more than ' +
'{maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
Expand All @@ -271,14 +286,28 @@ export const getMessages = () => {
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
job_group_id_invalid_max_length: {
status: 'ERROR',
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobGroupIdInvalidMaxLengthErrorMessage', {
defaultMessage: 'Job group name must be more than {maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
job_group_id_valid: {
status: 'SUCCESS',
heading: i18n.translate('xpack.ml.models.jobValidation.messages.jobGroupIdValidHeading', {
defaultMessage: 'Job group id formats are valid',
}),
text: i18n.translate('xpack.ml.models.jobValidation.messages.jobGroupIdValidMessage', {
defaultMessage: 'Lowercase alphanumeric (a-z and 0-9) characters, hyphens or underscores, ' +
'starts and ends with an alphanumeric character.',
'starts and ends with an alphanumeric character, and is no more than ' +
'{maxLength, plural, one {# character} other {# characters}} long.',
values: {
maxLength: JOB_ID_MAX_LENGTH,
},
}),
url: 'https://www.elastic.co/guide/en/elasticsearch/reference/{{version}}/ml-job-resource.html#ml-job-resource'
},
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -6423,11 +6423,9 @@
"xpack.ml.models.jobValidation.messages.influencerLowSuggestionsMessage": "影響因子が構成されていません。{influencerSuggestion} を 1 つまたは複数使用することをお勧めします。",
"xpack.ml.models.jobValidation.messages.jobGroupIdInvalidMessage": "ジョブグループ名の 1 つが無効です。アルファベットの小文字 (a-z と 0-9)、ハイフンまたはアンダーラインが使用でき、最初と最後を英数字にする必要があります。",
"xpack.ml.models.jobValidation.messages.jobGroupIdValidHeading": "ジョブグループ ID のフォーマットは有効です。",
"xpack.ml.models.jobValidation.messages.jobGroupIdValidMessage": "アルファベットの小文字 (a-z と 0-9)、ハイフンまたはアンダーライン、最初と最後を英数字にする必要があります。",
"xpack.ml.models.jobValidation.messages.jobIdEmptyMessage": "ジョブ名フィールドは未入力のままにできません。",
"xpack.ml.models.jobValidation.messages.jobIdInvalidMessage": "ジョブ名が無効です。アルファベットの小文字 (a-z と 0-9)、ハイフンまたはアンダーラインが使用でき、最初と最後を英数字にする必要があります。",
"xpack.ml.models.jobValidation.messages.jobIdValidHeading": "ジョブ ID のフォーマットは有効です。",
"xpack.ml.models.jobValidation.messages.jobIdValidMessage": "アルファベットの小文字 (a-z と 0-9)、ハイフンまたはアンダーライン、最初と最後を英数字にする必要があります。",
"xpack.ml.models.jobValidation.messages.mmlGreaterThanMaxMmlMessage": "モデルメモリー制限が、このクラスターに構成された最大モデルメモリー制限を超えています。",
"xpack.ml.models.jobValidation.messages.mmlValueInvalidMessage": "{mml} はモデルメモリー制限の有効な値ではありません。この値は最低 1MB で、バイト (例: 10MB) で指定する必要があります。",
"xpack.ml.models.jobValidation.messages.skippedExtendedTestsMessage": "ジョブの構成の基本要件が満たされていないため、他のチェックをスキップしました。",
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -6566,11 +6566,9 @@
"xpack.ml.models.jobValidation.messages.influencerLowSuggestionsMessage": "尚未配置任何影响因素。考虑使用一个或多个 {influencerSuggestion}。",
"xpack.ml.models.jobValidation.messages.jobGroupIdInvalidMessage": "有一个作业组名称无效。它们可以包含小写字母数字(a-z 和 0-9)字符、连字符或下划线,必须以字母数字字符开头和结尾",
"xpack.ml.models.jobValidation.messages.jobGroupIdValidHeading": "作业 ID 格式有效。",
"xpack.ml.models.jobValidation.messages.jobGroupIdValidMessage": "小写字母数字(a-z 和 0-9)字符、连字符或下划线,以字母数字字符开头和结尾。",
"xpack.ml.models.jobValidation.messages.jobIdEmptyMessage": "作业名称字段不得为空。",
"xpack.ml.models.jobValidation.messages.jobIdInvalidMessage": "作业名称无效。其可以包含小写字母数字(a-z 和 0-9)字符、连字符或下划线,必须以字母数字字符开头和结尾",
"xpack.ml.models.jobValidation.messages.jobIdValidHeading": "作业 ID 格式有效。",
"xpack.ml.models.jobValidation.messages.jobIdValidMessage": "小写字母数字(a-z 和 0-9)字符、连字符或下划线,以字母数字字符开头和结尾。",
"xpack.ml.models.jobValidation.messages.mmlGreaterThanMaxMmlMessage": "模型内存限制大于为此集群配置的最大模型内存限制。",
"xpack.ml.models.jobValidation.messages.mmlValueInvalidMessage": "{mml} 不是有效的模型内存限制值。该值需要至少 1MB,且应以字节为单位(例如 10MB)指定。",
"xpack.ml.models.jobValidation.messages.skippedExtendedTestsMessage": "已跳过其他检查,因为未满足作业配置的基本要求。",
Expand Down