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

Improve request structure for scripts and templates #11091

Closed
colings86 opened this issue May 11, 2015 · 4 comments · Fixed by #11164

Comments

@colings86
Copy link
Member

commented May 11, 2015

Created from the discussion at #8393 (comment)

The ScriptParameterParser added in 1.5 was an effort to make the parsing of scripts in Elasticsearch consistent but did not go far enough. The way scripts are defined in requests is still not one consistent method across the codebase. We want scripts to be defined in an object containing the script, params, lang, and type. This will be defined in the request in the following way:

Inline Scripts:

...
"script": {
    "inline": "doc[foo] + 1",
    "lang": "groovy",
    "params": {
        ...
    }
}
...

File Scripts:

...
"script": {
    "file": "my_file_script",
    "lang": "groovy",
    "params": {
        ...
    }
}
...

Index Scripts:

...
"script": {
    "id": "my_indexed_script",
    "lang": "groovy",
    "params": {
        ...
    }
}
...

Inline scripts can also be specified using the following short form, but the language will be the default language and the params will be empty:

...
"script": "doc[foo] + 1"
...

We should have similar constructs for templates. Also, the script and template classes should be available in the Java API and should be able to parse and render (build) themselves in XContent as well as being able to be serialized between nodes.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 11, 2015

Can you deprecate rather than remove the
{
"script": " name/body",
"lang": " whatever"
}
style?

Or maybe offer the new style in 1.6 if you are dropping the old in 2.0?

Created from the discussion at #8393 (comment)
#8393 (comment)

The ScriptParameterParser added in 1.5 was an effort to make the parsing of
scripts in Elasticsearch consistent but did not go far enough. The way
scripts are defined in requests is still not one consistent method across
the codebase. We want scripts to be defined in an object containing the
script, params, lang, and type. This will be defined in the request in the
following way:

Inline Scripts:

..."script": {
"inline": "doc[foo] + 1",
"lang": "groovy",
"params": {
...
}
}
...

File Scripts:

..."script": {
"file": "my_file_script",
"lang": "groovy",
"params": {
...
}
}
...

Index Scripts:

..."script": {
"id": "my_indexed_script",
"lang": "groovy",
"params": {
...
}
}
...

Inline scripts can also be specified using the following short form, but
the language will be the default language and the params will be empty:

..."script": "doc[foo] + 1"
...

We should have similar constructs for templates. Also, the script and
template classes should be available in the Java API and should be able to
parse and render (build) themselves in XContent as well as being able to be
serialized between nodes.


Reply to this email directly or view it on GitHub
#11091.

@colings86

This comment has been minimized.

Copy link
Member Author

commented May 11, 2015

@nik9000 yes, it is the intention to support both the current format and the new format in 1.6. Sorry, I should have mentioned that in the issue.

@colings86 colings86 added the >breaking label May 11, 2015
@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 11, 2015

Thanks!

On Mon, May 11, 2015 at 10:29 AM, Colin Goodheart-Smithe <
notifications@github.com> wrote:

@nik9000 https://github.com/nik9000 yes, it is the intention to support
both the current format and the new format in 1.6. Sorry, I should have
mentioned that in the issue.


Reply to this email directly or view it on GitHub
#11091 (comment)
.

colings86 added a commit to colings86/elasticsearch that referenced this issue May 14, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
colings86 added a commit to colings86/elasticsearch that referenced this issue May 18, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
colings86 added a commit to colings86/elasticsearch that referenced this issue May 29, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
@colings86

This comment has been minimized.

Copy link
Member Author

commented May 29, 2015

Backporting the changes to 1.6 proved far too complex to have the confidence in the change, so the plan is now to merge this to 2.0 only and support the old API in the REST layer so that stuff thats stored like transforms and templates continue to work, but to remove support for the old script API in the Java API and the wire format. When the old script API is used through the REST layer it will log a deprecation warning to the new deprecation logger

colings86 added a commit to colings86/elasticsearch that referenced this issue May 29, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
@colings86 colings86 removed the v1.6.0 label May 29, 2015
colings86 added a commit to colings86/elasticsearch that referenced this issue May 29, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
colings86 added a commit to colings86/elasticsearch that referenced this issue May 29, 2015
This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes elastic#11091
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.