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

Manually building KueryNode for Fleet's routes #75693

Merged
merged 19 commits into from
Sep 1, 2020
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
filter?: string;
filter?: string | KueryNode;
```
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface SavedObjectsFindOptions
| --- | --- | --- |
| [defaultSearchOperator](./kibana-plugin-core-public.savedobjectsfindoptions.defaultsearchoperator.md) | <code>'AND' &#124; 'OR'</code> | |
| [fields](./kibana-plugin-core-public.savedobjectsfindoptions.fields.md) | <code>string[]</code> | An array of fields to include in the results |
| [filter](./kibana-plugin-core-public.savedobjectsfindoptions.filter.md) | <code>string</code> | |
| [filter](./kibana-plugin-core-public.savedobjectsfindoptions.filter.md) | <code>string &#124; KueryNode</code> | |
| [hasReference](./kibana-plugin-core-public.savedobjectsfindoptions.hasreference.md) | <code>{</code><br/><code> type: string;</code><br/><code> id: string;</code><br/><code> }</code> | |
| [namespaces](./kibana-plugin-core-public.savedobjectsfindoptions.namespaces.md) | <code>string[]</code> | |
| [page](./kibana-plugin-core-public.savedobjectsfindoptions.page.md) | <code>number</code> | |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
filter?: string;
filter?: string | KueryNode;
```
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface SavedObjectsFindOptions
| --- | --- | --- |
| [defaultSearchOperator](./kibana-plugin-core-server.savedobjectsfindoptions.defaultsearchoperator.md) | <code>'AND' &#124; 'OR'</code> | |
| [fields](./kibana-plugin-core-server.savedobjectsfindoptions.fields.md) | <code>string[]</code> | An array of fields to include in the results |
| [filter](./kibana-plugin-core-server.savedobjectsfindoptions.filter.md) | <code>string</code> | |
| [filter](./kibana-plugin-core-server.savedobjectsfindoptions.filter.md) | <code>string &#124; KueryNode</code> | |
| [hasReference](./kibana-plugin-core-server.savedobjectsfindoptions.hasreference.md) | <code>{</code><br/><code> type: string;</code><br/><code> id: string;</code><br/><code> }</code> | |
| [namespaces](./kibana-plugin-core-server.savedobjectsfindoptions.namespaces.md) | <code>string[]</code> | |
| [page](./kibana-plugin-core-server.savedobjectsfindoptions.page.md) | <code>number</code> | |
Expand Down
4 changes: 3 additions & 1 deletion src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1188,8 +1188,10 @@ export interface SavedObjectsFindOptions {
// (undocumented)
defaultSearchOperator?: 'AND' | 'OR';
fields?: string[];
// Warning: (ae-forgotten-export) The symbol "KueryNode" needs to be exported by the entry point index.d.ts
//
// (undocumented)
filter?: string;
filter?: string | KueryNode;
// (undocumented)
hasReference?: {
type: string;
Expand Down
14 changes: 13 additions & 1 deletion src/core/server/saved_objects/service/lib/filter_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,19 @@ const mockMappings = {

describe('Filter Utils', () => {
describe('#validateConvertFilterToKueryNode', () => {
test('Validate a simple filter', () => {
test('Empty string filters are ignored', () => {
expect(validateConvertFilterToKueryNode(['foo'], '', mockMappings)).toBeUndefined();
});
test('Validate a simple KQL KueryNode filter', () => {
expect(
validateConvertFilterToKueryNode(
['foo'],
esKuery.nodeTypes.function.buildNode('is', `foo.attributes.title`, 'best', true),
mockMappings
)
).toEqual(esKuery.fromKueryExpression('foo.title: "best"'));
});
test('Validate a simple KQL expression filter', () => {
expect(
validateConvertFilterToKueryNode(['foo'], 'foo.attributes.title: "best"', mockMappings)
).toEqual(esKuery.fromKueryExpression('foo.title: "best"'));
Expand Down
7 changes: 4 additions & 3 deletions src/core/server/saved_objects/service/lib/filter_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ const astFunctionType = ['is', 'range', 'nested'];

export const validateConvertFilterToKueryNode = (
allowedTypes: string[],
filter: string,
filter: string | KueryNode,
indexMapping: IndexMapping
): KueryNode | undefined => {
if (filter && filter.length > 0 && indexMapping) {
const filterKueryNode = esKuery.fromKueryExpression(filter);
if (filter && indexMapping) {
const filterKueryNode =
typeof filter === 'string' ? esKuery.fromKueryExpression(filter) : filter;
Comment on lines -34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: we are no longer ignoring empty string filters, but I guess this has no impact?

Copy link
Contributor Author

@kobelb kobelb Aug 31, 2020

Choose a reason for hiding this comment

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

Empty strings are still ignored, I added 195a5a1#diff-c5ab35755205b8adbf6cfc858d69ea3bR86-R88 to double-check this. JavaScript treats an empty-string as being falsy, so checking filter and filter.length > 0 is redundant, we only need to check filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait what? Kibana is not Java code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no... Maybe in the future though! New-new platform??

Copy link
Contributor

Choose a reason for hiding this comment

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

We would need to find a more suitable codename if we switch to java. NewPlatformBijectiveAdapterFactory maybe.


const validationFilterKuery = validateFilterKueryNode({
astFilter: filterKueryNode,
Expand Down
45 changes: 44 additions & 1 deletion src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { encodeHitVersion } from '../../version';
import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry';
import { DocumentMigrator } from '../../migrations/core/document_migrator';
import { elasticsearchClientMock } from '../../../elasticsearch/client/mocks';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { nodeTypes } from '../../../../../plugins/data/common/es_query';

jest.mock('./search_dsl/search_dsl', () => ({ getSearchDsl: jest.fn() }));

Expand Down Expand Up @@ -2529,7 +2531,7 @@ describe('SavedObjectsRepository', () => {
expect(getSearchDslNS.getSearchDsl).toHaveBeenCalledWith(mappings, registry, relevantOpts);
});

it(`accepts KQL filter and passes kueryNode to getSearchDsl`, async () => {
it(`accepts KQL expression filter and passes KueryNode to getSearchDsl`, async () => {
const findOpts = {
namespace,
search: 'foo*',
Expand Down Expand Up @@ -2570,6 +2572,47 @@ describe('SavedObjectsRepository', () => {
`);
});

it(`accepts KQL KueryNode filter and passes KueryNode to getSearchDsl`, async () => {
const findOpts = {
namespace,
search: 'foo*',
searchFields: ['foo'],
type: ['dashboard'],
sortField: 'name',
sortOrder: 'desc',
defaultSearchOperator: 'AND',
hasReference: {
type: 'foo',
id: '1',
},
indexPattern: undefined,
filter: nodeTypes.function.buildNode('is', `dashboard.attributes.otherField`, '*'),
};

await findSuccess(findOpts, namespace);
const { kueryNode } = getSearchDslNS.getSearchDsl.mock.calls[0][2];
expect(kueryNode).toMatchInlineSnapshot(`
Object {
"arguments": Array [
Object {
"type": "literal",
"value": "dashboard.otherField",
},
Object {
"type": "wildcard",
"value": "@kuery-wildcard@",
},
Object {
"type": "literal",
"value": false,
},
],
"function": "is",
"type": "function",
}
`);
});

it(`supports multiple types`, async () => {
const types = ['config', 'index-pattern'];
await findSuccess({ type: types });
Expand Down
5 changes: 4 additions & 1 deletion src/core/server/saved_objects/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ import { SavedObjectUnsanitizedDoc } from './serialization';
import { SavedObjectsMigrationLogger } from './migrations/core/migration_logger';
import { SavedObject } from '../../types';

// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { KueryNode } from '../../../plugins/data/common';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring this code to remove the circular reference became quite time-consuming. Since a circular reference already exists between core and the data plugin, I'd like to postpone fixing the circular reference to another PR to minimize the likelihood of this change not making 7.10.


export {
SavedObjectAttributes,
SavedObjectAttribute,
Expand Down Expand Up @@ -89,7 +92,7 @@ export interface SavedObjectsFindOptions {
rootSearchFields?: string[];
hasReference?: { type: string; id: string };
defaultSearchOperator?: 'AND' | 'OR';
filter?: string;
filter?: string | KueryNode;
namespaces?: string[];
/** An optional ES preference value to be used for the query **/
preference?: string;
Expand Down
4 changes: 3 additions & 1 deletion src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2320,8 +2320,10 @@ export interface SavedObjectsFindOptions {
// (undocumented)
defaultSearchOperator?: 'AND' | 'OR';
fields?: string[];
// Warning: (ae-forgotten-export) The symbol "KueryNode" needs to be exported by the entry point index.d.ts
//
// (undocumented)
filter?: string;
filter?: string | KueryNode;
// (undocumented)
hasReference?: {
type: string;
Expand Down
37 changes: 35 additions & 2 deletions x-pack/plugins/ingest_manager/server/services/agents/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { Agent, AgentAction, AgentActionSOAttributes } from '../../../common/typ
import { AGENT_ACTION_SAVED_OBJECT_TYPE } from '../../../common/constants';
import { savedObjectToAgentAction } from './saved_objects';
import { appContextService } from '../app_context';
import { nodeTypes } from '../../../../../../src/plugins/data/common';

export async function createAgentAction(
soClient: SavedObjectsClientContract,
Expand All @@ -29,9 +30,24 @@ export async function getAgentActionsForCheckin(
soClient: SavedObjectsClientContract,
agentId: string
): Promise<AgentAction[]> {
const filter = nodeTypes.function.buildNode('and', [
kobelb marked this conversation as resolved.
Show resolved Hide resolved
nodeTypes.function.buildNode(
'not',
nodeTypes.function.buildNode(
'is',
`${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.sent_at`,
'*'
)
),
nodeTypes.function.buildNode(
'is',
`${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.agent_id`,
agentId
),
]);
const res = await soClient.find<AgentActionSOAttributes>({
type: AGENT_ACTION_SAVED_OBJECT_TYPE,
filter: `not ${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.sent_at: * and ${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.agent_id:${agentId}`,
filter,
});

return Promise.all(
Expand Down Expand Up @@ -78,9 +94,26 @@ export async function getAgentActionByIds(
}

export async function getNewActionsSince(soClient: SavedObjectsClientContract, timestamp: string) {
const filter = nodeTypes.function.buildNode('and', [
nodeTypes.function.buildNode(
'not',
nodeTypes.function.buildNode(
'is',
`${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.sent_at`,
'*'
)
),
nodeTypes.function.buildNode(
'range',
`${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.created_at`,
{
gte: timestamp,
}
),
]);
const res = await soClient.find<AgentActionSOAttributes>({
type: AGENT_ACTION_SAVED_OBJECT_TYPE,
filter: `not ${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.sent_at: * AND ${AGENT_ACTION_SAVED_OBJECT_TYPE}.attributes.created_at >= "${timestamp}"`,
filter,
});

return res.saved_objects.map(savedObjectToAgentAction);
Expand Down