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.

Sign up for GitHub

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] Data frame analytics: Fix source index checks. #44479

Merged
merged 5 commits into from
Sep 2, 2019
Merged

[ML] Data frame analytics: Fix source index checks. #44479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
17a7c73
[ML] Fix source index checks.
walterra Aug 30, 2019
422eedb
Merge branch 'master' of github.com:elastic/kibana into ml-dfa-fix-so…
walterra Aug 30, 2019
154d6fd
[ML] Fix tests.
walterra Sep 2, 2019
9e3926d
Merge branch 'master' into ml-dfa-fix-source-index
walterra Sep 2, 2019
f1ac595
Merge branch 'ml-dfa-fix-source-index' of github.com:walterra/kibana …
walterra Sep 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 17 additions & 33 deletions ...ics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,15 @@ import {
import { i18n } from '@kbn/i18n';

import { metadata } from 'ui/metadata';
import { INDEX_PATTERN_ILLEGAL_CHARACTERS } from 'ui/index_patterns';

import { CreateAnalyticsFormProps } from '../../hooks/use_create_analytics_form';

// based on code used by `ui/index_patterns` internally
// remove the space character from the list of illegal characters
INDEX_PATTERN_ILLEGAL_CHARACTERS.pop();
const characterList = INDEX_PATTERN_ILLEGAL_CHARACTERS.join(', ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to be checking against this list of characters, as the control for selecting the source index is a dropdown which list the Kibana index patterns, so presumably this check was done when the index pattern was created? So really the form just needs to be checking that the source index hasn't been left empty.


export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, state }) => {
const { setFormState } = actions;

Expand All @@ -49,7 +55,6 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
jobIdValid,
sourceIndex,
sourceIndexNameEmpty,
sourceIndexNameExists,
sourceIndexNameValid,
} = form;

Expand Down Expand Up @@ -119,39 +124,18 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
'This index pattern does not contain any numeric type fields. The analytics job may not be able to come up with any outliers.',
})
}
isInvalid={!sourceIndexNameEmpty && (!sourceIndexNameValid || !sourceIndexNameExists)}
isInvalid={!sourceIndexNameEmpty && !sourceIndexNameValid}
error={
(!sourceIndexNameEmpty &&
!sourceIndexNameValid && [
<Fragment>
{i18n.translate('xpack.ml.dataframe.analytics.create.sourceIndexInvalidError', {
defaultMessage: 'Invalid source index name.',
})}
<br />
<EuiLink
href={`https://www.elastic.co/guide/en/elasticsearch/reference/${metadata.branch}/indices-create-index.html#indices-create-index`}
target="_blank"
>
{i18n.translate(
'xpack.ml.dataframe.stepDetailsForm.sourceIndexInvalidErrorLink',
{
defaultMessage: 'Learn more about index name limitations.',
}
)}
</EuiLink>
</Fragment>,
]) ||
(!sourceIndexNameEmpty &&
!sourceIndexNameExists && [
<Fragment>
{i18n.translate(
'xpack.ml.dataframe.analytics.create.sourceIndexDoesNotExistError',
{
defaultMessage: 'An index with this name does not exist.',
}
)}
</Fragment>,
])
!sourceIndexNameEmpty &&
!sourceIndexNameValid && [
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this should never be shown as you can only select a valid KIbana index pattern as the source index.

{i18n.translate('xpack.ml.dataframe.analytics.create.sourceIndexInvalidError', {
defaultMessage:
'Invalid source index name, it cannot contain spaces or the characters: {characterList}',
values: { characterList },
})}
</Fragment>,
]
}
>
<Fragment>
Expand Down
4 changes: 4 additions & 0 deletions ...rame_analytics/pages/analytics_management/hooks/use_create_analytics_form/reducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import { ACTION } from './actions';
import { reducer } from './reducer';
import { getInitialState } from './state';

jest.mock('ui/index_patterns', () => ({
validateIndexPattern: () => true,
}));

describe('useCreateAnalyticsForm', () => {
test('reducer(): provide a minimum required valid job config, then reset.', () => {
const initialState = getInitialState();
Expand Down
13 changes: 5 additions & 8 deletions ...ata_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import { idx } from '@kbn/elastic-idx';
import { i18n } from '@kbn/i18n';

import { validateIndexPattern } from 'ui/index_patterns';

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

import { isAnalyticsIdValid } from '../../../../common';
Expand All @@ -22,7 +24,7 @@ const validateAdvancedEditor = (state: State): State => {

const sourceIndexName = idx(jobConfig, _ => _.source.index) || '';
const sourceIndexNameEmpty = sourceIndexName === '';
const sourceIndexNameValid = isValidIndexName(sourceIndexName);
const sourceIndexNameValid = validateIndexPattern(sourceIndexName);

const destinationIndexName = idx(jobConfig, _ => _.dest.index) || '';
const destinationIndexNameEmpty = destinationIndexName === '';
Expand Down Expand Up @@ -161,11 +163,9 @@ export function reducer(state: State, action: Action): State {
}

if (action.payload.sourceIndex !== undefined) {
newFormState.sourceIndexNameExists = state.indexNames.some(
name => newFormState.sourceIndex === name
);
newFormState.sourceIndexNameEmpty = newFormState.sourceIndex === '';
newFormState.sourceIndexNameValid = isValidIndexName(newFormState.sourceIndex);
const validationMessages = validateIndexPattern(newFormState.sourceIndex);
newFormState.sourceIndexNameValid = Object.keys(validationMessages).length === 0;
}

return state.isAdvancedEditorEnabled
Expand All @@ -177,9 +177,6 @@ export function reducer(state: State, action: Action): State {
newState.form.destinationIndexNameExists = newState.indexNames.some(
name => newState.form.destinationIndex === name
);
newState.form.sourceIndexNameExists = newState.indexNames.some(
name => newState.form.sourceIndex === name
);
return newState;
}

Expand Down
2 changes: 0 additions & 2 deletions .../data_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ export interface State {
jobIdEmpty: boolean;
jobIdValid: boolean;
sourceIndex: EsIndexName;
sourceIndexNameExists: boolean;
sourceIndexNameEmpty: boolean;
sourceIndexNameValid: boolean;
};
Expand Down Expand Up @@ -68,7 +67,6 @@ export const getInitialState = (): State => ({
jobIdEmpty: true,
jobIdValid: false,
sourceIndex: '',
sourceIndexNameExists: false,
sourceIndexNameEmpty: true,
sourceIndexNameValid: false,
},
Expand Down
4 changes: 4 additions & 0 deletions ...s/analytics_management/hooks/use_create_analytics_form/use_create_analytics_form.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import { kibanaContextValueMock } from '../../../../../contexts/kibana/__mocks__

import { getErrorMessage, useCreateAnalyticsForm } from './use_create_analytics_form';

jest.mock('ui/index_patterns', () => ({
validateIndexPattern: () => true,
}));

const getMountedHook = () =>
mountHook(
() => useCreateAnalyticsForm(),
Expand Down