Synthetic source error on script loads #88334

tmgordeeva · 2022-07-07T00:08:00Z

Scripts will now error out if they attempt to load source with synthetic source on. As part of this change, SourceLookup is now refactored to split the cases where we intend to actually load the source (ie fetch phase) and cases where we use a source already provided.

Refactors SourceLookup into a static source lookup used for most cases where we access the source again after the fetch phase, and a re-loading lookup used by scripts. The re-loading lookup now also fails with an error when we are using synthetic source preventing silent failures or non-sensical behavior from scripts.

elasticmachine · 2022-07-07T00:08:04Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

tmgordeeva · 2022-07-07T00:08:34Z

I swore this PR existed somewhere, but checking on the status today I was surprised not to see it. If there is a duplicate somewhere somehow my bad!

elasticsearchmachine · 2022-07-07T19:38:37Z

Hi @tmgordeeva, I've created a changelog YAML for you.

…e-scripts

tmgordeeva · 2022-07-18T21:36:33Z

@romseygeek Nik's out, and I heard you might be able to take a look at this?

nik9000 · 2022-07-20T13:37:30Z

server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java

     */
    public SearchLookup lookup() {
        if (this.lookup == null) {
+            SourceFieldMapper sourceMapper = mappingLookup == null ? null : (SourceFieldMapper) mappingLookup.getMapper("_source");


I might just do boolean isSynthetic = here so you don't have to reason about "null means false"

Or, rather, "null means true"

nik9000 · 2022-07-20T13:37:52Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificanceLookup.java

+        } else {
+            Query rewrittenBackground = context.filterQuery(backgroundQuery);
+            this.backgroundFilter = rewrittenBackground;
+        }


I think this snuck in from another PR.

nik9000 · 2022-07-20T13:40:34Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

-    public int docId() {
-        return docId;
-    }
+    int docId();


While you are here, can you add javadoc for these? It'll really help whoever comes after. I know I often don't. But I should...

nik9000 · 2022-07-20T13:42:50Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

 import static java.util.Collections.emptyMap;

-public class SourceLookup implements Map<String, Object> {
+public interface SourceLookup extends Map<String, Object> {


I wonder if we should/can confine the Map extension to scripts. It'd an odd thing to do outside of script compatibility and this is used so many places. It's probably not worth it for this PR, but it's an interesting question.

nik9000 · 2022-07-20T13:46:28Z

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java

+        Function<String, MappedFieldType> fieldTypeLookup,
+        BiFunction<MappedFieldType, Supplier<SearchLookup>, IndexFieldData<?>> fieldDataLookup,
+        boolean canReloadSource
+    ) {


I tend not to like adding these extra ctors if I can get away without it. Is this temporary? I think we may have talked about this but I'm forgetting.

Either way, probably worth copying the javadoc from above.

From a quick look it appears that all of the uses of the extra constructor are in test code, which makes me agree with Nik. Let's nuke it - it makes the PR a bit noisier but I think it will save confusion further down the line.

nik9000 · 2022-07-20T13:50:12Z

server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java

            stub.put(nestedFieldName, entry);
-            SourceLookup nestedSourceLookup = new SourceLookup();
+            SourceLookup nestedSourceLookup = new SourceLookup.Static();
            nestedSourceLookup.setSource(filteredSource);


Seeing this makes me think "SourceLookup.Static should take the source as a parameter - but I think that'd only work sometimes, right? In the fetch phase we don't have it available right away, I think.

nik9000 · 2022-07-20T13:51:26Z

docs/changelog/88334.yaml

@@ -0,0 +1,5 @@
+pr: 88334
+summary: Synthetic source error on script loads
+area: Geo


Hmmm.... I think maybe the thing made a mistake. This isn't in Geo at all.

nik9000 · 2022-07-20T13:52:23Z

...java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorTests.java

        }
    }

+    public void testFieldBackground() throws IOException {


I think this also snuck in.

nik9000 · 2022-07-20T13:54:15Z

server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java

            fieldFetcher.setNextReader(readerContext);
-            SourceLookup sourceLookup = new SourceLookup();
+            SourceLookup sourceLookup = new SourceLookup.Static();
            sourceLookup.setSegmentAndDocument(readerContext, 0);


Oh this is fun. Static isn't going to be able to do anything with the readerContext or the docId. But this is how we pass those things into the fetching framework, right?

nik9000 · 2022-07-20T13:57:04Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

-    @SuppressWarnings("rawtypes")
-    public void putAll(Map m) {
-        throw new UnsupportedOperationException();
+    class Static extends AbstractSourceLookup {


These both deserve javadoc please.

I wonder if PreLoaded is a better name than Static. My mind jumps to java's static keyword and I keep thinking "there is only one of this". But static here means something more like "unchanging".

Speaking of, how hard would be be to make the Static version truly immutable? Not worth it in this PR, I'd wager, but maybe actually good?

…e-scripts

SourceLookup now takes a SourceProvider which will describe how we expect to access the source. Methods for reading and parsing the source have been split out into the different source providers. It's now more explicit when SourceLookup is expecting to just re-use an existing map or bytes source object.

…scripts

romseygeek

This looks great, thanks @tmgordeeva! I left a few comments but I think we're close.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificanceLookup.java

romseygeek · 2022-08-01T09:33:36Z

.../percolator/src/main/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhase.java

                                slot
                            );
-                            subContext.sourceLookup().setSource(document);
+                            subContext.sourceLookup().setSourceProvider(new SourceLookup.BytesSourceProvider(document));


One of these days we will get rid of these setters. Ah well, progress not perfection!

romseygeek · 2022-08-01T09:35:36Z

server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java

     */
    public SearchLookup lookup() {
        if (this.lookup == null) {
+            SourceFieldMapper sourceMapper = mappingLookup == null ? null : (SourceFieldMapper) mappingLookup.getMapper("_source");


Can we make this a new getSourceProvider() method on MappingLookup? That way we can easily add in future enhancements like a nested source provider.

romseygeek · 2022-08-01T09:39:14Z

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java

+        Function<String, MappedFieldType> fieldTypeLookup,
+        BiFunction<MappedFieldType, Supplier<SearchLookup>, IndexFieldData<?>> fieldDataLookup,
+        boolean canReloadSource
+    ) {


From a quick look it appears that all of the uses of the extra constructor are in test code, which makes me agree with Nik. Let's nuke it - it makes the PR a bit noisier but I think it will save confusion further down the line.

romseygeek · 2022-08-01T09:40:42Z

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java

        Function<String, MappedFieldType> fieldTypeLookup,
-        TriFunction<MappedFieldType, Supplier<SearchLookup>, MappedFieldType.FielddataOperation, IndexFieldData<?>> fieldDataLookup
+        TriFunction<MappedFieldType, Supplier<SearchLookup>, MappedFieldType.FielddataOperation, IndexFieldData<?>> fieldDataLookup,
+        SourceLookup sourceLookup


I think I'd prefer to pass the SourceProvider as a parameter here, rather than the SourceLookup directly

romseygeek · 2022-08-01T09:42:44Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

-            }
+        if (sourceProvider instanceof ReaderSourceProvider) {
+            ((ReaderSourceProvider) sourceProvider).setSegmentAndDocument(context, docId);
        }


Let's add a setSegmentAndDocument() method to the SourceProvider interface as well.

…scripts

romseygeek

Nearly there, I would like to double check one thing re script access to _source.

romseygeek · 2022-08-15T12:14:55Z

docs/changelog/88334.yaml

+pr: 88334
+summary: Synthetic source error on script loads
+area: Geo
+type: bug


Let's make this area: Search and type: enhancement

romseygeek · 2022-08-15T12:21:48Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

-
-    private LeafReader reader;
-    private CheckedBiConsumer<Integer, FieldsVisitor, IOException> fieldReader;
+public class SourceLookup {


Can we double-check that removing the Map interface here doesn't break scripts that look things up via _source['foo']? There's a lot of dynamic typing that goes on in scripts and I'm a bit nervous that this will silently cause some issues. Maybe @stu-elastic or @jdconrad have a view? If it works, then that's awesome as having it here has been annoying me for several years now :)

I looked through this with @stu-elastic, and this looks like this should be a safe change. Everywhere we allow source access we do through a DynamicMap where we call the source method to get the source Map directly instead of using SourceLookup. The one edge case I can see is a custom plugin may have its own specially defined context; however, we don't guarantee any stable api right now.

I think we need to also change the construction of the asMap member of LeafSearchLookup. Although this hasn't caused any test failures, which either means we have a gap in test coverage or that this isn't actually used anymore.

…scripts

romseygeek

Let's get this merged! Thanks for all the iterations @tmgordeeva

Tatyana Gordeeva added 2 commits June 27, 2022 19:49

Propagate alias filters to significance aggs filters

dc2593e

tmgordeeva added the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Jul 7, 2022

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 7, 2022

elasticsearchmachine added the v8.4.0 label Jul 7, 2022

tmgordeeva added the >bug label Jul 7, 2022

tmgordeeva added 2 commits July 7, 2022 12:38

Update docs/changelog/88334.yaml

2ee3a5f

Merge remote-tracking branch 'origin/master' into fix/synthetic-sourc…

0219dc9

…e-scripts

nik9000 reviewed Jul 20, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix/synthetic-sourc…

df940db

…e-scripts

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:05

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

Tatyana Gordeeva added 5 commits July 27, 2022 10:06

Merge remote-tracking branch 'origin/main' into fix/synthetic-source-…

919ff56

…scripts

Style cleanup

b09f10f

Merge remote-tracking branch 'origin/main' into fix/synthetic-source-…

8e45012

…scripts

Map source return potentially null source type for empty maps

304e9d3

nik9000 requested a review from romseygeek July 29, 2022 18:28

Cluster tests cast to SourceLookup instead of Map

f25ee51

romseygeek requested changes Aug 1, 2022

View reviewed changes

Tatyana Gordeeva added 4 commits August 3, 2022 15:30

SourceLookup cleanup

a6d2a32

Style cleanup

f774e13

More style cleanup

4e0d175

Merge remote-tracking branch 'origin/main' into fix/synthetic-source-…

8085c74

…scripts

romseygeek reviewed Aug 15, 2022

View reviewed changes

Tatyana Gordeeva added 2 commits August 25, 2022 16:19

Merge remote-tracking branch 'origin/main' into fix/synthetic-source-…

4ea7092

…scripts

Return SourceLookup implementing Map

05b44ae

romseygeek approved these changes Aug 26, 2022

View reviewed changes

tmgordeeva merged commit db5ddb3 into elastic:main Aug 26, 2022

tmgordeeva deleted the fix/synthetic-source-scripts branch August 26, 2022 18:32

nik9000 mentioned this pull request Sep 12, 2022

Synthetic Source #86603

Closed

50 tasks

Synthetic source error on script loads #88334

Synthetic source error on script loads #88334

Uh oh!

Conversation

tmgordeeva commented Jul 7, 2022

Uh oh!

elasticmachine commented Jul 7, 2022

Uh oh!

tmgordeeva commented Jul 7, 2022

Uh oh!

elasticsearchmachine commented Jul 7, 2022

Uh oh!

tmgordeeva commented Jul 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants