-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
cleanup server-log action #53326
cleanup server-log action #53326
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* 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 { withoutControlCharacters } from './string_utils'; | ||
|
||
describe('ensureSingleLineString()', () => { | ||
test('works with plain ole strings', () => { | ||
expect(withoutControlCharacters('')).toEqual(''); | ||
expect(withoutControlCharacters(' a b c ')).toEqual(' a b c '); | ||
}); | ||
|
||
test('works with multiple control characters', () => { | ||
expect(withoutControlCharacters(' \r \n ')).toEqual(' ; ; '); | ||
expect(withoutControlCharacters('\r \n ')).toEqual('; ; '); | ||
expect(withoutControlCharacters(' \r \n')).toEqual(' ; ;'); | ||
expect(withoutControlCharacters('\r \n')).toEqual('; ;'); | ||
}); | ||
|
||
test('works with /00-/1F', () => { | ||
for (let c = 0; c <= 0x1f; c++) { | ||
expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); | ||
} | ||
expect(withoutControlCharacters(String.fromCharCode(0x20))).toEqual(' '); | ||
}); | ||
|
||
test('works with /7F-/9F', () => { | ||
expect(withoutControlCharacters(String.fromCharCode(0x7e))).toEqual('~'); | ||
for (let c = 0x7f; c <= 0x9f; c++) { | ||
expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); | ||
} | ||
const nbsp = String.fromCharCode(0xa0); | ||
expect(withoutControlCharacters(nbsp)).toEqual(nbsp); | ||
}); | ||
|
||
test('works with UCS newlines', () => { | ||
expect(withoutControlCharacters('\u2027')).toEqual('\u2027'); | ||
expect(withoutControlCharacters('\u2028')).toEqual(';'); | ||
expect(withoutControlCharacters('\u2029')).toEqual(';'); | ||
expect(withoutControlCharacters('\u202A')).toEqual('\u202A'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
// see: https://en.wikipedia.org/wiki/Unicode_control_characters | ||
const CONTROL_CHAR_PATTERN = /[\x00-\x1F]|[\x7F-\x9F]|[\u2028-\u2029]/g; | ||
|
||
// replaces control characters in string with ; | ||
export function withoutControlCharacters(s: string): string { | ||
return s.replace(CONTROL_CHAR_PATTERN, ';'); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,21 +10,25 @@ import { schema, TypeOf } from '@kbn/config-schema'; | |||||||||||
|
||||||||||||
import { Logger } from '../../../../../../src/core/server'; | ||||||||||||
import { ActionType, ActionTypeExecutorOptions, ActionTypeExecutorResult } from '../types'; | ||||||||||||
import { withoutControlCharacters } from './lib/string_utils'; | ||||||||||||
|
||||||||||||
// params definition | ||||||||||||
|
||||||||||||
export type ActionParamsType = TypeOf<typeof ParamsSchema>; | ||||||||||||
|
||||||||||||
const ParamsSchema = schema.object({ | ||||||||||||
message: schema.string(), | ||||||||||||
level: schema.oneOf([ | ||||||||||||
schema.literal('trace'), | ||||||||||||
schema.literal('debug'), | ||||||||||||
schema.literal('info'), | ||||||||||||
schema.literal('warn'), | ||||||||||||
schema.literal('error'), | ||||||||||||
schema.literal('fatal'), | ||||||||||||
]), | ||||||||||||
level: schema.oneOf( | ||||||||||||
[ | ||||||||||||
schema.literal('trace'), | ||||||||||||
schema.literal('debug'), | ||||||||||||
schema.literal('info'), | ||||||||||||
schema.literal('warn'), | ||||||||||||
schema.literal('error'), | ||||||||||||
schema.literal('fatal'), | ||||||||||||
], | ||||||||||||
{ defaultValue: 'info' } | ||||||||||||
), | ||||||||||||
}); | ||||||||||||
|
||||||||||||
// action type definition | ||||||||||||
|
@@ -48,8 +52,9 @@ async function executor( | |||||||||||
const actionId = execOptions.actionId; | ||||||||||||
const params = execOptions.params as ActionParamsType; | ||||||||||||
|
||||||||||||
const sanitizedMessage = withoutControlCharacters(params.message); | ||||||||||||
try { | ||||||||||||
logger[params.level](params.message); | ||||||||||||
logger[params.level](`server-log: ${sanitizedMessage}`); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the prefix seems to be causing a test failure in: kibana/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts Lines 97 to 101 in 65de3c5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ya, just pushed a commit with the fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe 'server-log' should be pulled out as a symbol, as it is also used in the action type name and id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense |
||||||||||||
} catch (err) { | ||||||||||||
const message = i18n.translate('xpack.actions.builtin.serverLog.errorLoggingErrorMessage', { | ||||||||||||
defaultMessage: 'error logging message', | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will remove tabs as well? I think that is probably OK, but some might expect that to be valid whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true; it's likely to confuse customers, so I think we should probably just let that one pass through untranslated