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

Get all search templates/scripts #25179

Open
brusic opened this issue Jun 12, 2017 · 14 comments
Open

Get all search templates/scripts #25179

brusic opened this issue Jun 12, 2017 · 14 comments
Assignees
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache help wanted adoptme Team:Core/Infra Meta label for core/infra team

Comments

@brusic
Copy link
Contributor

brusic commented Jun 12, 2017

TL;DR I want to get all the search templates defined in the cluster metadata and I am willing to implement it myself

I found myself needing to get all the search templates defined in the cluster metadata. Noticed that there was no endpoint to get all the templates, so I though I should just add it myself. After diving into the code, I noticed why it is not currently implemented.

Templates are just scripts, so the search template endpoints end up going through the script service, which does not support getAll. The actual scripts are stored in a collection referenced only by the id. I thought I implement a hack by simply iterating through all the script and looking at the key name, but the lang context has been recently removed. I could create another data structure to keep track of the lang context, but I do not like the idea of supporting different data structures (basically multi key maps).

There has been some discussion about splitting templates from scripts (#16314) with an initial attempt by @nik9000 (#23744). I did not review the rejected PR, but perhaps it would support exactly what I need. If not, this separation would have been useful since the template service would support my (eventual) PR.

My next step would be to implement a generic getAllStoredScripts type class/method, which would support a lang param. Two issues come to mind:

  1. The implementation for my use case would be rendered obsolete if Split templates from scripts? #16314 gets implemented.
  2. Would highlighting all the scripts be a security concern? Due to potential overload, I would have implemented it as a cat endpoint with the actual script not being returned (I just need names).

Would it be wise to implement such an endpoint? I do not want to code something, only to have a fail to be picked up for non-technical reasons.

@brusic brusic changed the title Getting all search tempaltes/scripts Getting all search templates/scripts Jun 12, 2017
@brusic brusic changed the title Getting all search templates/scripts Get all search templates/scripts Jun 12, 2017
@rjernst
Copy link
Member

rjernst commented Jun 12, 2017

@brusic The work on script contexts (#20426) has rendered the need for a separate template service obsolete. The separate stored template api will also soon be removed, in favor of using the stored scripts api (#24596). While designing the script contexts, we considered having context be a required parameter to storing a script. However, this was deemed too cumbersome for users. The stored script API does now allow passing a context (#25014), but this context is only used for validation of the script for that context, and is not stored with it.

Essentially, the stored scripts are allowed to be used in any context in which they will compile. I can see adding an api to retrieve a list of the stored script names (along with lang), but if you want to filter on the intended use, I think you should do so with your own naming convention.

@brusic
Copy link
Contributor Author

brusic commented Jun 12, 2017

I incorrectly used the term "context" to refer to the previously used "namespace". Previously, the keys were compound keys with the namespace and id. Does not matter that they are gone, decision making based on the key format is a hack and I should not have even thought about it!

In REST terms, I would love to do something like
GET /_cat/scripts?lang=mustache (although I do not like having an concrete implementation as the key name for templates) Technically what I need is /_cat/search_templates.

I was not aware of the other script issues. As always, Elasticsearch is a moving target and difficult to modify without knowing the direction. I will see how the stored scripts API pans out (subscribed to the relevant issues), although in the meantime I would need to keep search template state locally.

@rjernst
Copy link
Member

rjernst commented Jun 12, 2017

I think before we have a cat API for scripts, we would need one that returns the scripts directly. I'm going to mark this issue as adoptme and low hanging fruit because I think it should be rather easy. I still recommend using your own naming convention in order to allow know "these scripts will be used as templates".

@rjernst rjernst added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache help wanted adoptme good first issue low hanging fruit labels Jun 12, 2017
@elisabeth-sorrell
Copy link

@brusic Have you already started working on this?

@brusic
Copy link
Contributor Author

brusic commented Jan 19, 2018

Quite honestly, I have forgotten about it since it was no longer needed for the project I was working for at that time. Since 6.x is out, I might give it a shot.

@SachithS
Copy link

SachithS commented Feb 7, 2018

Hi there,
I'm first-timer to the project. Can I start working on this? Please guide me on this.

@brusic
Copy link
Contributor Author

brusic commented Feb 7, 2018

@SachithS I have already submitted a pull request for this issue. It is referenced above. It will require a breaking change, so I doubt it will be released before 7.0. Still awaiting on the comments to the PR.

@SachithS
Copy link

SachithS commented Feb 7, 2018

@brusic , thanks for letting me know. Will try to find a task to work on.

@rjernst
Copy link
Member

rjernst commented Feb 26, 2018

I marked this as discuss because I'm not sure PRs marked for discussion are picked up in the issue searches being used for discussion. See the questions in #38368 about plurality of the request paths for this api.

@ozencem
Copy link

ozencem commented Aug 3, 2019

Hi @rjernst, I had been working on #42735, without realizing there was a duplicate issue, and you closed it as duplicate, which makes sense.

After reading up on this issue, it seems to me like it hasn't been being worked on for a while, although the latest activity seems to be that @jdconrad self-assigned the issue. I would still like to work on this if it's OK with @jdconrad and yourself. What do you think?

@ozencem
Copy link

ozencem commented Aug 3, 2019

I have also read the comments under the associated pull request and gained some background information and learned about the challenges in implementing this feature. I think it makes a lot of sense to create a new endpoint _script as the team discussed on a FixIt Friday. However, simply adding that endpoint to the current request would not work as the current response object only supports returning a single stored script and that response needs to be changed as well to support returning multiple scripts.

I have thought about this and my proposal that would not introduce any breaking changes would be:

  1. Add new request/response/action classes in plural form, e.g. RestGetStoredScriptsAction.
  2. Allow both _script/{id} and _script endpoints for these classes to support returning both all and individual stored scripts.
  3. The response should support returning single or multiple stored scripts, unlike the existing response.
  4. Add deprecation warnings for the old _scripts/{id} endpoint in favor of _script/{id}.
  5. The old API and associated classes can be deprecated any time.

The only problem I can see with this approach is the inconsistency between the new singular endpoint _script, and its plural request/response/action classes (although a similar inconsistency already exists between singular classes/actions vs the plural action name and endpoint name). However, it still semantically makes sense as the response can actually include multiple scripts and it seems like all other solutions would introduce a breaking change. Additionally, all of these can also be easily refactored to be singular again after deprecating the old API if desired.

I hope I am not missing anything here. What do you think, @rjernst @jdconrad? I am happy to create a preliminary PR for this in the next couple of weeks if you're alright with it.

@rjernst
Copy link
Member

rjernst commented Aug 13, 2019

@ozencem There is already an existing PR #28368 that has been worked on for some time. I recommend you look at the discussions there for for the answers to many of your questions.

@stu-elastic
Copy link
Contributor

If #28368 remains stalled, we should add this feature ourselves since it'd be useful for admins and potentially kibana.

@brusic
Copy link
Contributor Author

brusic commented Apr 2, 2020

I've been done with the code for a while now.

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@stu-elastic stu-elastic removed the needs:triage Requires assignment of a team area label label Dec 9, 2020
@jdconrad jdconrad removed the good first issue low hanging fruit label Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache help wanted adoptme Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

8 participants