Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions #31044

rationull · 2018-06-02T06:35:42Z

I noticed this issue when working on PR #30111. @s1monw committed changes in January in #28335 checking for self-references in collections that come from scripts. In the case of scripted metric aggs, we have the params._agg map and _aggs list that the various involved scripts all modify. The original change added the self-reference check for the result of the combine script, but params._agg can play effectively the same role for init and map scripts so arguably it should be held to the same standard.

This PR adds a similar check after execution of the init, map, and reduce scripts. I also added unit tests for the init, map, and combine cases. Reduce doesn't seem to be quite as convenient to unit test (please correct me if I'm missing something here!) but I didn't see this as a blocker since the existing checks aren't unit tested in general anyway. Another option is to consolidate these test cases into ScriptedMetricITwhich should be able to check all four scripts; conceptually unit tests are a better fit though.

As part of this I added an overload for CollectionUtils.ensureNoSelfReferences() that takes an additional string parameter to slightly customize the error message, because this allows for unit test verification that a single particular check actually failed.

I haven't discussed any of this with anyone so any feedback is appreciated, and I realize this might be closed as paranoid overkill.

Potential further improvements
Taking this to its logical conclusion, it seem like params as well as all other variables available on the script context should be automatically checked for self-references. Seems like for Painless at least this could be implemented (maybe as another item on #31009 with the other new safety checks?) although it might be overkill as I don't know how important it is to avoid the potential for stack overflows here.

@rjernst @colings86 this conflicts with my changes in #30111 but I wanted to go ahead with this in a separate PR because I had some time to look at it. Of course I will handle any conflicts depending on which one gets merged first (and if this one gets merged at all).

…etric agg script executions Previously this was called for the combine script only. This change checks for self references for init, map, and reduce scripts as well, and adds unit test coverage for the init, map, and combine cases.

elasticmachine · 2018-06-04T08:26:34Z

Pinging @elastic/es-search-aggs

s1monw

LGTM I left a comment. thanks for doing this

s1monw · 2018-06-07T08:49:50Z

server/src/main/java/org/elasticsearch/common/util/CollectionUtils.java

+     * Deeply inspects a Map, Iterable, or Object array looking for references back to itself.
+     * @throws IllegalArgumentException if a self-reference is found
+     * @param value The object to evaluate looking for self references
+     */
    public static void ensureNoSelfReferences(Object value) {


I think we should make the hit mandatory and remove this method.

Nice -- I always like less ambiguity in error situation especially for testing. I can throw that change into this PR if that works for you.

…ageHint per PR feedback

rationull · 2018-06-08T05:30:09Z

server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java

@@ -843,8 +843,8 @@ public void testEnsureNotNull() {
    }

    public void testEnsureNoSelfReferences() throws IOException {
-        CollectionUtils.ensureNoSelfReferences(emptyMap());
-        CollectionUtils.ensureNoSelfReferences(null);
+        builder().map(emptyMap());


I fixed these to call the functions that are supposed to be under test here, but I did not go so far as to introduce a message hint to the XContentBuilder version of ensureNoSelfReferences(), just because that's where I arbitrarily drew the line on scope. Happy to revisit if you'd like.

rationull · 2018-06-08T05:55:36Z

Removed the CollectionUtils.ensureNoSelfReferences() overload without a message hint. Also tweaked the message hints from my first commit a bit for better consistency with the new ones required due to removing that overload.

s1monw · 2018-06-11T06:39:11Z

thanks for doing this.

This commit adapts some test after #31044 has been merged.

* master: Remove RestGetAllAliasesAction (#31308) Temporary fix for broken build Reenable Checkstyle's unused import rule (#31270) Remove remaining unused imports before merging #31270 Fix non-REST doc snippet [DOC] Extend SQL docs Immediately flush channel after writing to buffer (#31301) [DOCS] Shortens ML API intros Use quotes in the call invocation (#31249) move security ingest processors to a sub ingest directory (#31306) Add 5.6.11 version constant. Fix version detection. SQL: Whitelist SQL utility class for better scripting (#30681) [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299) CCS: don't proxy requests for already connected node (#31273) Mute ScriptedMetricAggregatorTests testSelfReferencingAggStateAfterMap [test] opensuse packaging turn up debug logging Add unreleased version 6.3.1 Removes experimental tag from scripted_metric aggregation (#31298) [Rollup] Metric config parser must use builder so validation runs (#31159) [ML] Check licence when datafeeds use cross cluster search (#31247) Add notion of internal index settings (#31286) Test: Remove broken yml test feature (#31255) REST hl client: cluster health to default to cluster level (#31268) [ML] Update test thresholds to account for changes to memory control (#31289) Log warnings when cluster state publication failed to some nodes (#31233) Fix AntFixture waiting condition (#31272) Ignore numeric shard count if waiting for ALL (#31265) [ML] Implement new rules design (#31110) index_prefixes back-compat should test 6.3 (#30951) Core: Remove plain execute method on TransportAction (#30998) Update checkstyle to 8.10.1 (#31269) Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202) Modify pipelining handlers to require full requests (#31280) Revert upgrade to Netty 4.1.25.Final (#31282) Use armored input stream for reading public key (#31229) Fix Netty 4 Server Transport tests. Again. REST hl client: adjust wait_for_active_shards param in cluster health (#31266) REST high-level Client: remove deprecated API methods (#31200) [DOCS] Mark SQL feature as experimental [DOCS] Updates machine learning custom URL screenshots (#31222) Fix naming conventions check for XPackTestCase Fix security Netty 4 transport tests Fix race in clear scroll (#31259) [DOCS] Clarify audit index settings when remote indexing (#30923) Delete typos in SAML docs (#31199) REST high-level client: add Cluster Health API (#29331) [ML][TEST] Mute tests using rules (#31204) Support RequestedAuthnContext (#31238) SyncedFlushResponse to implement ToXContentObject (#31155) Add Get Aliases API to the high-level REST client (#28799) Remove some line length supressions (#31209) Validate xContentType in PutWatchRequest. (#31088) [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024) Suppress extras FS on caching directory tests Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)" Revert "Fix snippets in upgrade docs" Fix snippets in upgrade docs [DOCS] Added 6.3 info & updated the upgrade table. (#30940) LLClient: Support host selection (#30523) Upgrade to Netty 4.1.25.Final (#31232) Enable custom credentials for core REST tests (#31235) Move ESIndexLevelReplicationTestCase to test framework (#31243) Encapsulate Translog in Engine (#31220) HLRest: Add get index templates API (#31161) Remove all unused imports and fix CRLF (#31207) [Tests] Fix self-referencing tests [TEST] Fix testRecoveryAfterPrimaryPromotion [Docs] Remove mention pattern files in Grok processor (#31170) Use stronger write-once semantics for Azure repository (#30437) Don't swallow exceptions on replication (#31179) Limit the number of concurrent requests per node (#31206) Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions (#31044) Move java version checker back to its own jar (#30708) [test] add fix for rare virtualbox error (#31212)

colings86 added >non-issue :Analytics/Aggregations Aggregations labels Jun 4, 2018

s1monw approved these changes Jun 7, 2018

View reviewed changes

Remove CollectionUtil.ensureNoSelfReference() overload without a mess…

34b9e4b

…ageHint per PR feedback

rationull commented Jun 8, 2018

View reviewed changes

s1monw merged commit 85c26d6 into elastic:master Jun 11, 2018

tlrx added a commit that referenced this pull request Jun 11, 2018

[Tests] Fix self-referencing tests

a191665

This commit adapts some test after #31044 has been merged.

jonaslan mentioned this pull request Sep 11, 2018

Do not check self references in metric agg per document #33593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions #31044

Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions #31044

rationull commented Jun 2, 2018

elasticmachine commented Jun 4, 2018

s1monw left a comment

s1monw Jun 7, 2018

rationull Jun 7, 2018

rationull Jun 8, 2018

rationull commented Jun 8, 2018

s1monw commented Jun 11, 2018

Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions #31044

Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions #31044

Conversation

rationull commented Jun 2, 2018

elasticmachine commented Jun 4, 2018

s1monw left a comment

Choose a reason for hiding this comment

s1monw Jun 7, 2018

Choose a reason for hiding this comment

rationull Jun 7, 2018

Choose a reason for hiding this comment

rationull Jun 8, 2018

Choose a reason for hiding this comment

rationull commented Jun 8, 2018

s1monw commented Jun 11, 2018