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

Use saved object client for saved_object_loader find function #12083

Merged

Conversation

stacey-gammon
Copy link
Contributor

Still TODO: the rest of the saved_object_loader functionality that is querying es directly.

@trevan
Copy link
Contributor

trevan commented May 30, 2017

Could you also allow plugins to modify the search query sent through the find function (#11876)?

It also looks like you've now restricted the search query to be a GET request and I believe that prevents using more advanced query DSL.

@stacey-gammon
Copy link
Contributor Author

@trevan - I don't believe the saved object loader find function ever supported advanced query DSL. We don't support it in the object listing pages where this is used. Given the time sensitive nature of switching the es direct calls out for the new saved object api, which is a pre-requirement for type removal coming in 6.0, I'd prefer for enhancements to be dealt with separately and keep this PR focused on keeping existing functionality but switching the implementation around. Unless I am breaking something with this that I am unaware of?

@trevan
Copy link
Contributor

trevan commented May 31, 2017

@stacey-gammon, yes and no. It didn't support advanced DSL but a plugin could (with a minor change to the find function) be able to use advanced DSL. I just had to edit the find function to let non-strings through as is and then I could wrap the find functions inside my plugin to send custom DSL. Going to a GET request makes that a lot harder and makes implementing #11876 harder as well. Why the decision to use GET when it was previously using POST?

@stacey-gammon
Copy link
Contributor Author

I don't think the saved object API supports advanced query DSL. @tylersmalley can you confirm? I don't think there are any plans to add it either (at least at this point).

Can you explain more about your use case for using query DSL to retrieve saved objects? We have gotten requests to support wildcards for saved objects (#9767) but not advanced query DSL.

@trevan
Copy link
Contributor

trevan commented May 31, 2017

@stacey-gammon, my use case is for plugins. I would like to be able to restrict which saved objects are loaded for a particular user. I've edited the saved object data in ES (#11877 is a request to make that easier) so that saved object data has an owner attached to it and other things. My plugin then wraps the find function, takes the search query and wraps it in a bool object to add additional restrictions, and then passes that to find and on to ES.

So the query DSL isn't for users to enter in the search box but for plugins to modify the search so that it is more restrictive or more permissive.

@tylersmalley
Copy link
Contributor

@trevan, we're moving all saved objects to the SavedObject API introduced in #11632.

@trevan
Copy link
Contributor

trevan commented May 31, 2017

@tylersmalley, I must have missed that. I guess it would be nice to still allow the full DSL versus just the simple query string. I'll just have to figure out how to hack your changes to actually let the DSL through.

@jbudz
Copy link
Member

jbudz commented Jun 1, 2017

Related by not caused by this PR, the queries in the client look a bit off with multiple search_fields params and type specified twice

https://localhost:5601/imc/api/saved_objects/dashboard?type=dashboard&search=&per_page=1000&page=1&search_fields=title%5E3&search_fields=description

@jbudz
Copy link
Member

jbudz commented Jun 1, 2017

Also related but not caused by this PR, checking Save as new dashboard will cause a console error. Doesn't seem to have any adverse effects
image

@tylersmalley
Copy link
Contributor

Opened a PR to update the rest of the apps to use the API: stacey-gammon#5

Still outstanding is the export/import, which we can add a one off compatibility layer to.

@stacey-gammon stacey-gammon force-pushed the saved-object-api-for-loader-find branch from a48aeb9 to 2c155ab Compare June 12, 2017 13:40
@tylersmalley
Copy link
Contributor

LGTM, lets merge this and back port to 5.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants