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

savedObjects: add score to repository.find results #68894

Merged
merged 11 commits into from
Jun 18, 2020
1 change: 1 addition & 0 deletions docs/development/core/server/kibana-plugin-core-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
| [SavedObjectsExportResultDetails](./kibana-plugin-core-server.savedobjectsexportresultdetails.md) | Structure of the export result details entry |
| [SavedObjectsFindOptions](./kibana-plugin-core-server.savedobjectsfindoptions.md) | |
| [SavedObjectsFindResponse](./kibana-plugin-core-server.savedobjectsfindresponse.md) | Return type of the Saved Objects <code>find()</code> method.<!-- -->\*Note\*: this type is different between the Public and Server Saved Objects clients. |
| [SavedObjectsFindResult](./kibana-plugin-core-server.savedobjectsfindresult.md) | |
| [SavedObjectsImportConflictError](./kibana-plugin-core-server.savedobjectsimportconflicterror.md) | Represents a failure to import due to a conflict. |
| [SavedObjectsImportError](./kibana-plugin-core-server.savedobjectsimporterror.md) | Represents a failure to import. |
| [SavedObjectsImportMissingReferencesError](./kibana-plugin-core-server.savedobjectsimportmissingreferenceserror.md) | Represents a failure to import due to missing references. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ export interface SavedObjectsFindResponse<T = unknown>
| --- | --- | --- |
| [page](./kibana-plugin-core-server.savedobjectsfindresponse.page.md) | <code>number</code> | |
| [per\_page](./kibana-plugin-core-server.savedobjectsfindresponse.per_page.md) | <code>number</code> | |
| [saved\_objects](./kibana-plugin-core-server.savedobjectsfindresponse.saved_objects.md) | <code>Array&lt;SavedObject&lt;T&gt;&gt;</code> | |
| [saved\_objects](./kibana-plugin-core-server.savedobjectsfindresponse.saved_objects.md) | <code>Array&lt;SavedObjectsFindResult&lt;T&gt;&gt;</code> | |
| [total](./kibana-plugin-core-server.savedobjectsfindresponse.total.md) | <code>number</code> | |

Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
saved_objects: Array<SavedObject<T>>;
saved_objects: Array<SavedObjectsFindResult<T>>;
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsFindResult](./kibana-plugin-core-server.savedobjectsfindresult.md)

## SavedObjectsFindResult interface


<b>Signature:</b>

```typescript
export interface SavedObjectsFindResult<T = unknown> extends SavedObject<T>
```

## Properties

| Property | Type | Description |
| --- | --- | --- |
| [score](./kibana-plugin-core-server.savedobjectsfindresult.score.md) | <code>number</code> | The ES search's <code>_score</code> of this result. |

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsFindResult](./kibana-plugin-core-server.savedobjectsfindresult.md) &gt; [score](./kibana-plugin-core-server.savedobjectsfindresult.score.md)

## SavedObjectsFindResult.score property

The ES search's `_score` of this result.

<b>Signature:</b>

```typescript
score: number;
```
6 changes: 4 additions & 2 deletions src/core/public/saved_objects/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ export class SavedObjectsClient {
query,
});
return request.then((resp) => {
resp.saved_objects = resp.saved_objects.map((d) => this.createSavedObject(d));
return renameKeys<
PromiseType<ReturnType<SavedObjectsApi['find']>>,
SavedObjectsFindResponsePublic
Expand All @@ -314,7 +313,10 @@ export class SavedObjectsClient {
per_page: 'perPage',
page: 'page',
},
resp
{
...resp,
saved_objects: resp.saved_objects.map((d) => this.createSavedObject(d)),
}
Comment on lines +316 to +319
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client-side version of the SO client is quite different from the server-side's. Most notable difference is that it returns the objects wrapped into a SimpleSavedObject class. Adding the score property on the client-side find results probably means more significant changes, as we would have to extend this SimpleSavedObject class to add the score property, which is a little more complex and subject to side effects than just extending a type as it's done on the server-side

As our current need for score is on the server-side anyway (at least atm), I'm just ignoring the score on the client-side find.

This decision is still open to discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, hopefully we can change this client-side API sometime in the future.

) as SavedObjectsFindResponsePublic<T>;
});
};
Expand Down
1 change: 1 addition & 0 deletions src/core/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export {
SavedObjectsErrorHelpers,
SavedObjectsExportOptions,
SavedObjectsExportResultDetails,
SavedObjectsFindResult,
SavedObjectsFindResponse,
SavedObjectsImportConflictError,
SavedObjectsImportError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('getSortedObjectsForExport()', () => {
id: '2',
type: 'search',
attributes: {},
score: 1,
references: [
{
name: 'name',
Expand All @@ -59,6 +60,7 @@ describe('getSortedObjectsForExport()', () => {
id: '1',
type: 'index-pattern',
attributes: {},
score: 1,
references: [],
},
],
Expand Down Expand Up @@ -133,6 +135,7 @@ describe('getSortedObjectsForExport()', () => {
id: '2',
type: 'search',
attributes: {},
score: 1,
references: [
{
name: 'name',
Expand All @@ -145,6 +148,7 @@ describe('getSortedObjectsForExport()', () => {
id: '1',
type: 'index-pattern',
attributes: {},
score: 1,
references: [],
},
],
Expand Down Expand Up @@ -192,6 +196,7 @@ describe('getSortedObjectsForExport()', () => {
id: '2',
type: 'search',
attributes: {},
score: 1,
references: [
{
name: 'name',
Expand All @@ -204,6 +209,7 @@ describe('getSortedObjectsForExport()', () => {
id: '1',
type: 'index-pattern',
attributes: {},
score: 1,
references: [],
},
],
Expand Down Expand Up @@ -279,6 +285,7 @@ describe('getSortedObjectsForExport()', () => {
id: '2',
type: 'search',
attributes: {},
score: 1,
references: [
{
name: 'name',
Expand All @@ -291,6 +298,7 @@ describe('getSortedObjectsForExport()', () => {
id: '1',
type: 'index-pattern',
attributes: {},
score: 1,
references: [],
},
],
Expand Down Expand Up @@ -366,6 +374,7 @@ describe('getSortedObjectsForExport()', () => {
id: '2',
type: 'search',
attributes: {},
score: 1,
references: [
{
type: 'index-pattern',
Expand All @@ -378,6 +387,7 @@ describe('getSortedObjectsForExport()', () => {
id: '1',
type: 'index-pattern',
attributes: {},
score: 1,
references: [],
},
],
Expand Down Expand Up @@ -405,6 +415,7 @@ describe('getSortedObjectsForExport()', () => {
attributes: {
name: 'baz',
},
score: 1,
references: [],
},
{
Expand All @@ -413,6 +424,7 @@ describe('getSortedObjectsForExport()', () => {
attributes: {
name: 'foo',
},
score: 1,
references: [],
},
{
Expand All @@ -421,6 +433,7 @@ describe('getSortedObjectsForExport()', () => {
attributes: {
name: 'bar',
},
score: 1,
references: [],
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,11 @@ async function fetchObjectsToExport({
}

// sorts server-side by _id, since it's only available in fielddata
return findResponse.saved_objects.sort((a: SavedObject, b: SavedObject) =>
a.id > b.id ? 1 : -1
return (
findResponse.saved_objects
// exclude the find-specific `score` property from the exported objects
.map(({ score, ...obj }) => obj)
.sort((a: SavedObject, b: SavedObject) => (a.id > b.id ? 1 : -1))
Comment on lines +121 to +123
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 think this was the only significant API where we need to remove score after performing a find request.

);
} else {
throw Boom.badRequest('Either `type` or `objects` are required.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ describe('GET /api/saved_objects/_find', () => {
timeFieldName: '@timestamp',
notExpandable: true,
attributes: {},
score: 1,
references: [],
},
{
Expand All @@ -88,6 +89,7 @@ describe('GET /api/saved_objects/_find', () => {
timeFieldName: '@timestamp',
notExpandable: true,
attributes: {},
score: 1,
references: [],
},
],
Expand Down
8 changes: 5 additions & 3 deletions src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1939,7 +1939,7 @@ describe('SavedObjectsRepository', () => {
{
_index: '.kibana',
_id: `${namespace ? `${namespace}:` : ''}config:6.0.0-alpha1`,
_score: 1,
_score: 2,
...mockVersionProps,
_source: {
namespace,
Expand All @@ -1954,7 +1954,7 @@ describe('SavedObjectsRepository', () => {
{
_index: '.kibana',
_id: `${namespace ? `${namespace}:` : ''}index-pattern:stocks-*`,
_score: 1,
_score: 3,
...mockVersionProps,
_source: {
namespace,
Expand All @@ -1970,7 +1970,7 @@ describe('SavedObjectsRepository', () => {
{
_index: '.kibana',
_id: `${NAMESPACE_AGNOSTIC_TYPE}:something`,
_score: 1,
_score: 4,
...mockVersionProps,
_source: {
type: NAMESPACE_AGNOSTIC_TYPE,
Expand Down Expand Up @@ -2131,6 +2131,7 @@ describe('SavedObjectsRepository', () => {
type: doc._source.type,
...mockTimestampFields,
version: mockVersion,
score: doc._score,
attributes: doc._source[doc._source.type],
references: [],
});
Expand All @@ -2153,6 +2154,7 @@ describe('SavedObjectsRepository', () => {
type: doc._source.type,
...mockTimestampFields,
version: mockVersion,
score: doc._score,
attributes: doc._source[doc._source.type],
references: [],
});
Expand Down
8 changes: 6 additions & 2 deletions src/core/server/saved_objects/service/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
SavedObjectsBulkUpdateResponse,
SavedObjectsCreateOptions,
SavedObjectsFindResponse,
SavedObjectsFindResult,
SavedObjectsUpdateOptions,
SavedObjectsUpdateResponse,
SavedObjectsBulkUpdateObject,
Expand Down Expand Up @@ -674,8 +675,11 @@ export class SavedObjectsRepository {
page,
per_page: perPage,
total: response.hits.total,
saved_objects: response.hits.hits.map((hit: SavedObjectsRawDoc) =>
this._rawToSavedObject(hit)
saved_objects: response.hits.hits.map(
(hit: SavedObjectsRawDoc): SavedObjectsFindResult => ({
...this._rawToSavedObject(hit),
score: (hit as any)._score,
})
),
};
}
Expand Down
13 changes: 12 additions & 1 deletion src/core/server/saved_objects/service/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ export interface SavedObjectsBulkResponse<T = unknown> {
saved_objects: Array<SavedObject<T>>;
}

/**
*
* @public
*/
export interface SavedObjectsFindResult<T = unknown> extends SavedObject<T> {
/**
* The ES search's `_score` of this result.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The ES search's `_score` of this result.
* The Elasticsearch `_score` for this result.

I think it's better to rather use the full name for Elasticsearch

*/
score: number;
rudolf marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Return type of the Saved Objects `find()` method.
*
Expand All @@ -88,7 +99,7 @@ export interface SavedObjectsBulkResponse<T = unknown> {
* @public
*/
export interface SavedObjectsFindResponse<T = unknown> {
saved_objects: Array<SavedObject<T>>;
saved_objects: Array<SavedObjectsFindResult<T>>;
total: number;
per_page: number;
page: number;
Expand Down
7 changes: 6 additions & 1 deletion src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2037,11 +2037,16 @@ export interface SavedObjectsFindResponse<T = unknown> {
// (undocumented)
per_page: number;
// (undocumented)
saved_objects: Array<SavedObject<T>>;
saved_objects: Array<SavedObjectsFindResult<T>>;
// (undocumented)
total: number;
}

// @public (undocumented)
export interface SavedObjectsFindResult<T = unknown> extends SavedObject<T> {
score: number;
}

// @public
export interface SavedObjectsImportConflictError {
// (undocumented)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,18 @@
*/

import { times } from 'lodash';
import { SavedObjectsFindOptions, SavedObject } from 'src/core/server';
import { SavedObjectsFindOptions, SavedObjectsFindResult } from 'src/core/server';
import { savedObjectsClientMock } from '../../../../core/server/mocks';
import { findAll } from './find_all';

describe('findAll', () => {
let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;

const createObj = (id: number): SavedObject => ({
const createObj = (id: number): SavedObjectsFindResult => ({
type: 'type',
id: `id-${id}`,
attributes: {},
score: 1,
references: [],
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ describe('findRelationships', () => {
type: 'parent-type',
id: 'parent-id',
attributes: {},
score: 1,
references: [],
},
],
Expand Down
2 changes: 2 additions & 0 deletions test/api_integration/apis/saved_objects/find.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export default function ({ getService }) {
attributes: {
title: 'Count of requests',
},
score: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this is a little scary on the relevance of this PR to be honest. I had to add the score property on the find API FTRs, and it seems that every call we are performing in the suite returns a score of 0...

When testing from the saved object management table page, I did have some non-zero score value however, so I guess we might want/need to do some scoring tuning at some point. I think this is complex enough to be done as a follow-up though.

@rudolf any thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we don't have any integration tests that actually specify a search query in the search query param. Would be great if we could add that (could be a separate PR).

migrationVersion: resp.body.saved_objects[0].migrationVersion,
references: [
{
Expand Down Expand Up @@ -134,6 +135,7 @@ export default function ({ getService }) {
.searchSourceJSON,
},
},
score: 0,
references: [
{
name: 'kibanaSavedObjectMeta.searchSourceJSON.index',
Expand Down
1 change: 1 addition & 0 deletions test/api_integration/apis/saved_objects_management/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export default function ({ getService }: FtrProviderContext) {
type: 'index-pattern',
},
],
score: 0,
updated_at: '2017-09-21T18:51:23.794Z',
meta: {
editUrl:
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/actions/server/actions_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ describe('getAll()', () => {
foo: 'bar',
},
},
score: 1,
references: [],
},
],
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerts/server/alerts_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1667,6 +1667,7 @@ describe('find()', () => {
},
],
},
score: 1,
references: [
{
name: 'action_0',
Expand Down
Loading