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

SavedObjectClient.find multiple types #19231

Merged
merged 14 commits into from
May 24, 2018
Merged
Show file tree
Hide file tree
Changes from 11 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 @@ -96,8 +96,8 @@ $http.post = jest.fn().mockImplementation(() => ([]));
const defaultProps = {
savedObjectsClient: {
find: jest.fn().mockImplementation(({ type }) => {
// We pass in type when fetching counts
if (type) {
// We pass in a single type when fetching counts
if (type && !Array.isArray(type)) {
return {
total: 1,
savedObjects: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,13 @@ export class ObjectsTable extends Component {

fetchCounts = async () => {
const { queryText, visibleTypes } = parseQuery(this.state.activeQuery);
const includeTypes = INCLUDED_TYPES.filter(
const type = INCLUDED_TYPES.filter(
type => !visibleTypes || visibleTypes.includes(type)
);

const savedObjectCounts = await getSavedObjectCounts(
this.props.$http,
includeTypes,
type,
queryText
);

Expand Down Expand Up @@ -130,7 +130,7 @@ export class ObjectsTable extends Component {
let savedObjects = [];
let totalItemCount = 0;

const includeTypes = INCLUDED_TYPES.filter(
const type = INCLUDED_TYPES.filter(
type => !visibleTypes || visibleTypes.includes(type)
);

Expand All @@ -144,7 +144,7 @@ export class ObjectsTable extends Component {
sortField: 'type',
fields: ['title', 'id'],
searchFields: ['title'],
includeTypes,
type,
});

savedObjects = data.savedObjects.map(savedObject => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function registerScrollForCountRoute(server) {
handler: async (req, reply) => {
const savedObjectsClient = req.getSavedObjectsClient();
const findOptions = {
includeTypes: req.payload.typesToInclude,
type: req.payload.typesToInclude,
perPage: 1000,
};

Expand Down
1 change: 1 addition & 0 deletions src/server/mappings/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export {
getRootType,
getProperty,
getRootProperties,
getRootPropertiesObjects,
} from './lib';
28 changes: 28 additions & 0 deletions src/server/mappings/lib/get_root_properties_objects.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { getRootProperties } from './get_root_properties';

/**
* Get the property mappings for the root type in the EsMappingsDsl
* where the properties are objects
*
* If the mappings don't have a root type, or the root type is not
* an object type (it's a keyword or something) this function will
* throw an error.
*
* This data can be found at `{indexName}.mappings.{typeName}.properties`
* in the es indices.get() response where the properties are objects.
*
* @param {EsMappingsDsl} mappings
* @return {EsPropertyMappings}
*/
export function getRootPropertiesObjects(mappings) {
const rootProperties = getRootProperties(mappings);
return Object.entries(rootProperties).reduce((acc, [key, value]) => {

// we consider the existence of the properties or type of object to designate that this is an object
Copy link

Choose a reason for hiding this comment

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

I think this should say "... to designate that this is a type"

Copy link

@rhoboat rhoboat May 22, 2018

Choose a reason for hiding this comment

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

hm. I guess we are calling these "root properties objects", so maybe type makes sense? But to me personally, the root properties object is the same thing as a top level type. Maybe saying "... to designate that this is a top level mapping type"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the naming is quite weird. I was trying to keep the way we were parsing the mappings separate from their usage within Kibana's saved objects, which prevented me from naming it getSavedObjectTypes. Perhaps going with a long and somewhat clumsy name of getRootPropertiesThatAreObjectDatatypes is better?

Copy link

Choose a reason for hiding this comment

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

My comment wasn't talking about saved objects in particular. I was thinking of the elasticsearch naming, of "top level mapping type" which seems like a common way to describe that thing.

mappings
|-type (_doc)
  |-properties
    |-object datatype (kibana saved object type, sub-type of doc)
    |-field (fields common to all sibling object datatypes)
    |- ...

But ah, there's always the top level _doc type, so everything below it is just some thing, which we call types in Kibana, but which are generically Object Datatypes.

Does that above diagram describe it?

I now think the comment should say ".. that this is an object datatype"

if (value.properties || value.type === 'object') {
acc[key] = value;
}

return acc;
}, {});
}
166 changes: 166 additions & 0 deletions src/server/mappings/lib/get_root_properties_objects.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
import { getRootPropertiesObjects } from './get_root_properties_objects';

test(`returns single object with properties`, () => {
const mappings = {
rootType: {
properties: {
foo: {
properties: {}
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
properties: {}
}
});
});

test(`returns single object with type === 'object'`, () => {
const mappings = {
rootType: {
properties: {
foo: {
type: 'object'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
type: 'object'
}
});
});

test(`returns two objects with properties`, () => {
const mappings = {
rootType: {
properties: {
foo: {
properties: {}
},
bar: {
properties: {}
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
properties: {}
},
bar: {
properties: {}
}
});
});

test(`returns two objects with type === 'object'`, () => {
const mappings = {
rootType: {
properties: {
foo: {
type: 'object'
},
bar: {
type: 'object'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
type: 'object'
},
bar: {
type: 'object'
}
});
});

test(`excludes objects without properties and type of keyword`, () => {
const mappings = {
rootType: {
properties: {
foo: {
type: 'keyword'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({});
});

test(`excludes two objects without properties and type of keyword`, () => {
const mappings = {
rootType: {
properties: {
foo: {
type: 'keyword'
},
bar: {
type: 'keyword'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({});
});

test(`includes one object with properties and excludes one object without properties`, () => {
const mappings = {
rootType: {
properties: {
foo: {
properties: {}
},
bar: {
type: 'keyword'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
properties: {}
}
});
});

test(`includes one object with type === 'object' and excludes one object without properties`, () => {
const mappings = {
rootType: {
properties: {
foo: {
type: 'object'
},
bar: {
type: 'keyword'
}
}
}
};

const result = getRootPropertiesObjects(mappings);
expect(result).toEqual({
foo: {
type: 'object'
}
});
});
1 change: 1 addition & 0 deletions src/server/mappings/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ export { getProperty } from './get_property';
export { getTypes } from './get_types';
export { getRootType } from './get_root_type';
export { getRootProperties } from './get_root_properties';
export { getRootPropertiesObjects } from './get_root_properties_objects';
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`searchDsl/getSortParams sort with direction sortField is non-root multi-field with multiple types returns correct params 1`] = `"Unable to sort multiple types by field title.raw, not a root field"`;

exports[`searchDsl/getSortParams sort with direction sortFields is non-root simple property with multiple types returns correct params 1`] = `"Unable to sort multiple types by field title, not a root field"`;

exports[`searchDsl/getSortParams sortField no direction sortField is not-root multi-field with multiple types returns correct params 1`] = `"Unable to sort multiple types by field title.raw, not a root field"`;

exports[`searchDsl/getSortParams sortField no direction sortField is simple non-root property with multiple types returns correct params 1`] = `"Unable to sort multiple types by field title, not a root field"`;
59 changes: 25 additions & 34 deletions src/server/saved_objects/client/lib/search_dsl/query_params.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,31 @@
import { getRootProperties } from '../../../../mappings';
import { getRootPropertiesObjects } from '../../../../mappings';

/**
* Gets the types based on the type. Uses mappings to support
* null type (all types), a single type string or an array
* @param {EsMapping} mappings
* @param {(string|Array<string>)} type
*/
function getTypes(mappings, type) {
if (!type) {
return Object.keys(getRootPropertiesObjects(mappings));
}

if (Array.isArray(type)) {
return type;
}

return [type];
}

/**
* Get the field params based on the types and searchFields
* @param {Array<string>} searchFields
* @param {Array<string>} types
* @param {string|Array<string>} types
* @return {Object}
*/
function getFieldsForTypes(searchFields, types) {

if (!searchFields || !searchFields.length) {
return {
all_fields: true
Expand All @@ -24,49 +43,21 @@ function getFieldsForTypes(searchFields, types) {
/**
* Get the "query" related keys for the search body
* @param {EsMapping} mapping mappings from Ui
* @param {Object} type
* @param {(string|Array<string>)} type
* @param {String} search
* @param {Array<string>} searchFields
* @return {Object}
*/
export function getQueryParams(mappings, type, search, searchFields, includeTypes) {
export function getQueryParams(mappings, type, search, searchFields) {
if (!type && !search) {
if (includeTypes) {
return {
query: {
bool: {
should: includeTypes.map(includeType => ({
term: {
type: includeType,
}
}))
}
}
};
}

return {};
}

const bool = {};

if (type) {
bool.filter = [
{ term: { type } }
];
}

if (includeTypes) {
bool.must = [
{
bool: {
should: includeTypes.map(includeType => ({
term: {
type: includeType,
}
})),
}
}
{ [Array.isArray(type) ? 'terms' : 'term']: { type } }
];
}

Expand All @@ -78,7 +69,7 @@ export function getQueryParams(mappings, type, search, searchFields, includeType
query: search,
...getFieldsForTypes(
searchFields,
type ? [type] : Object.keys(getRootProperties(mappings)),
getTypes(mappings, type)
)
}
}
Expand Down
Loading