Prevent requests that use scripts or now() from being cached #20750
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit changes the strict REST parameters message to say that unconsumed parameters are unrecognized rather than unused. Additionally, the test is beefed up to include two unused parameters. Relates #20745
This commit adds a did you mean feature to the strict REST params error message. This works by comparing any unconsumed parameters to all of the consumer parameters, comparing the Levenstein distance between those parameters, and taking any consumed parameters that are close to an unconsumed parameter as candiates for the did you mean. * Fix pluralization in strict REST params message This commit fixes the pluralization in the strict REST parameters error message so that the word "parameter" is not unconditionally written as "parameters" even when there is only one unrecognized parameter. * Strength strict REST params did you mean test This commit adds an unconsumed parameter that is too far from every consumed parameter to have any candidate suggestions. Relates #20747
Before this change the processing of the ranges in the date range (and other range type) aggregations was done when the Aggregator was created. This meant that the SearchContext did not know that now had been used in a range until after the decision to cache was made. This change moves the processing of the ranges to the aggregation builders so that the search context is made aware that now has been used before it decides if the request should be cached
s1monw
added
>bug
blocker
:Core/Infra/Scripting
rjernst
approved these changes
Oct 5, 2016
| @@ -327,4 +338,75 @@ private ParsedQuery toQuery(QueryBuilder queryBuilder, CheckedFunction<QueryBuil | |||
| public final Index index() { | |||
| return indexSettings.getIndex(); | |||
| } | |||
|
|
|||
| /** | |||
| * Compiles (or retrieves from cache) and executes the provided script | |||
There was a problem hiding this comment.
This doesn't actually execute right? It is the binding that happens immediately (vs lazy below is binding to params later).
jpountz
approved these changes
Oct 5, 2016
| return search(lookup, compiledScript, script.getParams()); | ||
| } | ||
|
|
||
| public SearchScript search(SearchLookup lookup, CompiledScript compiledScript, Map<String, Object> params) { |
There was a problem hiding this comment.
Can you add some minimal javadocs?
| if (isCachable(firstQuery)) { | ||
| assert context.isCachable() : firstQuery.toString(); | ||
| } else { | ||
| assert context.isCachable() == false : firstQuery.toString(); |
There was a problem hiding this comment.
since it's a test case, should it use the junit assertions instead of the assert keyword?
colings86
added a commit
that referenced
this pull request
Oct 6, 2016
Prevent requests that use scripts or now() from being cached
colings86
added a commit
that referenced
this pull request
Oct 6, 2016
Prevent requests that use scripts or now() from being cached
clintongormley
added
:Search/Search
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We already have some mechanism in place that prevents requests from being cached in the request cache if they use now(). Yet we don't have any streamlined way to assert that we are not accessing it later. We found some issues in #20645 that relate to stored scripts or scripts that are not pure functions. The immediate fix is to disable caching for these scripts. Unfortunately the script access wasn't streamlined in aggregations nor in query parsing / creation. This change adds a contained API that allows up to make cachabiltily decisions in a single place and causes requests to fail if they access scripts or
now()after we cache the request.Relates to #20645