SavedObjectClient.find multiple types #19231

kobelb · 2018-05-18T20:48:35Z

#17426 introduced an includeTypes parameter to the SavedObjects.find, but it was used inconsistently through the SavedObjectClient. It was affecting the score of the documents, wasn't affecting the search query itself and it wasn't being used for sorting.

This PR modifies the SavedObjectsClient.find so that it accepts null, a string, or an array of strings for the type property. Additionally, we're now only searching across known saved object types where we used to be including an incorrect query when searching without specifying a type.

It should be noted, we still don't support sorting without specifying at least one type. Additionally, we only support sorting across multiple types if the field is a "root property". So, currently sorting by "type" or "updatedBy" are the only supported fields. Sorting by "nested" properties led to inconsistent results because we'd create a sort similar to the following:

sort: [
  {'visualize.title': {...} },
  {'savedSearch.title': {...} }
]

where first Visualizations would be returned sorted by title, and then savedSearches would be returned sorted by title.

kobelb · 2018-05-18T20:49:00Z

/cc @chrisronline

chrisronline · 2018-05-18T21:00:19Z

src/server/saved_objects/client/saved_objects_client.js

@@ -272,7 +272,6 @@ export class SavedObjectsClient {
      sortField,
      sortOrder,
      fields,
-      includeTypes,


To prevent breaking the saved objects UI upon merging, can we leave this as an optional parameter and merge it with the type parameter? I'll remove it once I update the saved objects UI to stop using it

I should've updated all usages of includeTypes to type

elasticmachine · 2018-05-18T21:45:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T13:03:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T15:27:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T17:34:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T22:26:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-22T13:05:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb · 2018-05-22T14:17:55Z

@tylersmalley @archanid this is ready for review when you get the chance.

rhoboat · 2018-05-22T20:35:08Z

src/server/mappings/lib/get_root_properties_objects.js

+  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


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

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"

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?

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"

rhoboat · 2018-05-22T20:39:28Z

src/server/saved_objects/client/lib/search_dsl/sorting_params.js

+  if (Array.isArray(type)) {
+    const rootField = getProperty(mappings, sortField);
+    if (!rootField) {
+      throw Boom.badRequest(`Unable to sort multiple types by field ${sortField}, not a root field`);


The error message.. Will people know what a "root field" is? If it's not something we say anywhere else, i'd use language that is consistent...

Up until this point, we haven't had to leak the way that these objects are mapped to the internal indices to consumers of the SavedObjectsClient yet, so I'm not aware of any existing language besides what we refer to them within the mappings "module" which is as "root properties", perhaps "root properties" would be more clear?

Yeah, I do think root properties is clearer. You think of fields and properties as the same thing. I can see that.. but I sort of feel like fields are just anything under the "properties" key that is not an object datatype. But even this usage of "field" isn't totally fool-proof because there's also multi-fields fields, which actually use the key field (https://www.elastic.co/guide/en/elasticsearch/reference/current/multi-fields.html), so we're really at a loss for a word to describe a non-multi-field, non object datatype thing that lives in properties.

After thinking more, I'm really not sure if root field or root property is better here.

++ for root property instead of root field

rhoboat · 2018-05-22T20:41:08Z

src/server/saved_objects/client/lib/search_dsl/sorting_params.test.js

@@ -119,5 +192,25 @@ describe('searchDsl/getSortParams', () => {
          });
      });
    });
+    describe('sortField is root multi-field with multiple types', () => {


For these descriptions, maybe say "data type" whenever it's talking about a data type, instead of just "type", which can refer to more things, like mapping types.

When using "type" in these descriptions, it's referring to the function parameter which is currently named type, so I'm not sure if referring to them as "data type" would provide any more clarity.

Fair enough.

rhoboat

LGTM, the comments about naming are your call.

elasticmachine · 2018-05-23T13:34:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-23T17:05:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tylersmalley

LGTM!

tylersmalley · 2018-05-23T18:17:49Z

As a note, we will probably need to figure out how to sort by multiple nested fields without having them grouped. but this is not something which we have a need for yet.

kobelb · 2018-05-24T11:43:31Z

As a note, we will probably need to figure out how to sort by multiple nested fields without having them grouped. but this is not something which we have a need for yet.

Agreed, there's potential for us to use Script Based Sorting to accomplish this, but there also might be a more performant way.

* Making the SavedObjectsClient.find accept a string or string[] for type * Searching is now filtered to actual types * Adding multi-type sort * Changing another use of includeTypes * Fixing test's reliance on type * Changing the find route to take type as an array * Can't sort on type... it's not a property on the object * Only allowing sorting on multiple types if it's a root property * Expanding indicator of root property object * Sorting by type, now that it's allowed and one of the few sort columns * Adjusting comment * Throwing better error message

elasticmachine · 2018-05-24T12:50:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

* SavedObjectClient.find multiple types (#19231) * Making the SavedObjectsClient.find accept a string or string[] for type * Searching is now filtered to actual types * Adding multi-type sort * Changing another use of includeTypes * Fixing test's reliance on type * Changing the find route to take type as an array * Can't sort on type... it's not a property on the object * Only allowing sorting on multiple types if it's a root property * Expanding indicator of root property object * Sorting by type, now that it's allowed and one of the few sort columns * Adjusting comment * Throwing better error message * Updating search_dsl error snapshots after changing error message (#19394)

kobelb added 3 commits May 18, 2018 08:13

Making the SavedObjectsClient.find accept a string or string[] for type

e259046

Searching is now filtered to actual types

ac21609

Adding multi-type sort

ec799fc

kobelb requested review from tylersmalley and rhoboat May 18, 2018 20:48

chrisronline reviewed May 18, 2018

View reviewed changes

kobelb added 2 commits May 21, 2018 08:04

Merge remote-tracking branch 'upstream/master' into saved-object-types

7ac9f12

Changing another use of includeTypes

8572a90

kobelb force-pushed the saved-object-types branch from 10c1149 to 8572a90 Compare May 21, 2018 12:06

Fixing test's reliance on type

a44879e

kobelb added 2 commits May 21, 2018 12:03

Changing the find route to take type as an array

3f1893e

Can't sort on type... it's not a property on the object

27423c2

kobelb added 2 commits May 21, 2018 17:20

Only allowing sorting on multiple types if it's a root property

d888756

Expanding indicator of root property object

447783a

Sorting by type, now that it's allowed and one of the few sort columns

60867c1

rhoboat reviewed May 22, 2018

View reviewed changes

rhoboat approved these changes May 22, 2018

View reviewed changes

Adjusting comment

2d7bfdb

Merge remote-tracking branch 'upstream/master' into saved-object-types

babe758

tylersmalley approved these changes May 23, 2018

View reviewed changes

Throwing better error message

8fafe43

kobelb merged commit a47348d into elastic:master May 24, 2018

kobelb deleted the saved-object-types branch May 24, 2018 11:45

kobelb mentioned this pull request May 24, 2018

[6.x] SavedObjectClient.find multiple types (#19231) #19390

Merged

kobelb added v7.0.0 v6.4.0 non-issue Indicates to automation that a pull request should not appear in the release notes labels May 24, 2018

chrisronline mentioned this pull request Jul 30, 2018

Fix "Export Everything" to properly constrain types #21402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SavedObjectClient.find multiple types #19231

SavedObjectClient.find multiple types #19231

kobelb commented May 18, 2018 •

edited by rhoboat

Loading

kobelb commented May 18, 2018

chrisronline May 18, 2018

kobelb May 21, 2018

elasticmachine commented May 18, 2018

elasticmachine commented May 21, 2018

elasticmachine commented May 21, 2018

elasticmachine commented May 21, 2018

elasticmachine commented May 21, 2018

elasticmachine commented May 22, 2018

kobelb commented May 22, 2018

rhoboat May 22, 2018

rhoboat May 22, 2018 •

edited

Loading

kobelb May 22, 2018

rhoboat May 22, 2018

rhoboat May 22, 2018

kobelb May 22, 2018

rhoboat May 22, 2018 •

edited

Loading

tylersmalley May 23, 2018

rhoboat May 22, 2018

kobelb May 22, 2018

rhoboat May 22, 2018

rhoboat left a comment

elasticmachine commented May 23, 2018

elasticmachine commented May 23, 2018

tylersmalley left a comment

tylersmalley commented May 23, 2018

kobelb commented May 24, 2018

elasticmachine commented May 24, 2018

SavedObjectClient.find multiple types #19231

SavedObjectClient.find multiple types #19231

Conversation

kobelb commented May 18, 2018 • edited by rhoboat Loading

kobelb commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 18, 2018

💔 Build Failed

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 21, 2018

💚 Build Succeeded

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 22, 2018

💚 Build Succeeded

kobelb commented May 22, 2018

Choose a reason for hiding this comment

rhoboat May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat left a comment

Choose a reason for hiding this comment

elasticmachine commented May 23, 2018

💚 Build Succeeded

elasticmachine commented May 23, 2018

💚 Build Succeeded

tylersmalley left a comment

Choose a reason for hiding this comment

tylersmalley commented May 23, 2018

kobelb commented May 24, 2018

elasticmachine commented May 24, 2018

💔 Build Failed

kobelb commented May 18, 2018 •

edited by rhoboat

Loading

rhoboat May 22, 2018 •

edited

Loading

rhoboat May 22, 2018 •

edited

Loading