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

Add types deprecation to script contexts #37554

Merged
merged 6 commits into from
Jan 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@
import org.elasticsearch.ingest.AbstractProcessor;
import org.elasticsearch.ingest.IngestDocument;
import org.elasticsearch.ingest.Processor;
import org.elasticsearch.script.DeprecationMap;
import org.elasticsearch.script.IngestScript;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptException;
import org.elasticsearch.script.ScriptService;

import java.io.InputStream;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.elasticsearch.ingest.ConfigurationUtils.newConfigurationException;
Expand All @@ -46,6 +49,16 @@
*/
public final class ScriptProcessor extends AbstractProcessor {

private static final Map<String, String> DEPRECATIONS;
static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"_type",
"[types removal] Looking up doc types [_type] in scripts is deprecated."
);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}

public static final String TYPE = "script";

private final Script script;
Expand All @@ -72,7 +85,8 @@ public final class ScriptProcessor extends AbstractProcessor {
@Override
public IngestDocument execute(IngestDocument document) {
IngestScript.Factory factory = scriptService.compile(script, IngestScript.CONTEXT);
factory.newInstance(script.getParams()).execute(document.getSourceAndMetadata());
factory.newInstance(script.getParams()).execute(
new DeprecationMap(document.getSourceAndMetadata(), DEPRECATIONS, "script_processor"));
CollectionUtils.ensureNoSelfReferences(document.getSourceAndMetadata(), "ingest script");
return document;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,28 @@ Script.DEFAULT_SCRIPT_LANG, new MockScriptEngine(
assertThat(ingestDocument.getSourceAndMetadata(), hasKey("bytes_total"));
assertThat(ingestDocument.getSourceAndMetadata().get("bytes_total"), is(randomBytesTotal));
}

public void testTypeDeprecation() throws Exception {
String scriptName = "script";
ScriptService scriptService = new ScriptService(Settings.builder().build(),
Collections.singletonMap(
Script.DEFAULT_SCRIPT_LANG, new MockScriptEngine(
Script.DEFAULT_SCRIPT_LANG,
Collections.singletonMap(
scriptName, ctx -> {
ctx.get("_type");
return null;
}
),
Collections.emptyMap()
)
),
new HashMap<>(ScriptModule.CORE_CONTEXTS)
);
Script script = new Script(ScriptType.INLINE, Script.DEFAULT_SCRIPT_LANG, scriptName, Collections.emptyMap());
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), Collections.emptyMap());
ScriptProcessor processor = new ScriptProcessor(randomAlphaOfLength(10), script, scriptService);
processor.execute(ingestDocument);
assertWarnings("[types removal] Looking up doc types [_type] in scripts is deprecated.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ protected <T extends ActionRequest> T applyScript(Consumer<Map<String, Object>>
UpdateScript.Factory factory = (params, ctx) -> new UpdateScript(Collections.emptyMap(), ctx) {
@Override
public void execute() {
scriptBody.accept(ctx);
scriptBody.accept(getCtx());
}
};;
when(scriptService.compile(any(), eq(UpdateScript.CONTEXT))).thenReturn(factory);
Expand All @@ -67,6 +67,11 @@ public void execute() {
return (result != null) ? (T) result.self() : null;
}

public void testTypeDeprecation() {
applyScript((Map<String, Object> ctx) -> ctx.get("_type"));
assertWarnings("[types removal] Looking up doc types [_type] in scripts is deprecated.");
}

public void testScriptAddingJunkToCtxIsError() {
try {
applyScript((Map<String, Object> ctx) -> ctx.put("junk", "junk"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
Expand All @@ -31,12 +32,24 @@
import java.util.concurrent.TimeUnit;
import java.util.function.LongSupplier;
import java.util.stream.Collectors;

import org.elasticsearch.script.DeprecationMap;
import org.elasticsearch.script.IngestConditionalScript;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptService;

public class ConditionalProcessor extends AbstractProcessor {

private static final Map<String, String> DEPRECATIONS;
static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"_type",
"[types removal] Looking up doc types [_type] in scripts is deprecated."
);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}

static final String TYPE = "conditional";

private final Script condition;
Expand Down Expand Up @@ -81,7 +94,8 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
boolean evaluate(IngestDocument ingestDocument) {
IngestConditionalScript script =
scriptService.compile(condition, IngestConditionalScript.CONTEXT).newInstance(condition.getParams());
return script.execute(new UnmodifiableIngestData(ingestDocument.getSourceAndMetadata()));
return script.execute(new UnmodifiableIngestData(
new DeprecationMap(ingestDocument.getSourceAndMetadata(), DEPRECATIONS, "conditional-processor")));
}

Processor getProcessor() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class AbstractSortScript implements ScorerAware {
this.leafLookup = lookup.getLeafSearchLookup(leafContext);
Map<String, Object> parameters = new HashMap<>(params);
parameters.putAll(leafLookup.asMap());
this.params = new DeprecationMap(parameters, DEPRECATIONS);
this.params = new DeprecationMap(parameters, DEPRECATIONS, "sort-script");
}

protected AbstractSortScript() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public abstract class AggregationScript implements ScorerAware {
private Object value;

public AggregationScript(Map<String, Object> params, SearchLookup lookup, LeafReaderContext leafContext) {
this.params = new DeprecationMap(new HashMap<>(params), DEPRECATIONS);
this.params = new DeprecationMap(new HashMap<>(params), DEPRECATIONS, "aggregation-script");
this.leafLookup = lookup.getLeafSearchLookup(leafContext);
this.params.putAll(leafLookup.asMap());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ public final class DeprecationMap implements Map<String, Object> {

private final Map<String, String> deprecations;

public DeprecationMap(Map<String, Object> delegate, Map<String, String> deprecations) {
private final String logKeyPrefix;

public DeprecationMap(Map<String, Object> delegate, Map<String, String> deprecations, String logKeyPrefix) {
this.delegate = delegate;
this.deprecations = deprecations;
this.logKeyPrefix = logKeyPrefix;
}

@Override
Expand All @@ -64,7 +67,7 @@ public boolean containsValue(final Object value) {
public Object get(final Object key) {
String deprecationMessage = deprecations.get(key);
if (deprecationMessage != null) {
deprecationLogger.deprecated(deprecationMessage);
deprecationLogger.deprecatedAndMaybeLog(logKeyPrefix + "_" + key, deprecationMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i would use - as the separator, so there is a clear distinction with the map key, since all our context names use underscores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
return delegate.get(key);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public FieldScript(Map<String, Object> params, SearchLookup lookup, LeafReaderCo
this.leafLookup = lookup.getLeafSearchLookup(leafContext);
params = new HashMap<>(params);
params.putAll(leafLookup.asMap());
this.params = new DeprecationMap(params, DEPRECATIONS);
this.params = new DeprecationMap(params, DEPRECATIONS, "field-script");
}

// for expression engine
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public ScoreScript(Map<String, Object> params, SearchLookup lookup, LeafReaderCo
this.leafLookup = lookup.getLeafSearchLookup(leafContext);
params = new HashMap<>(params);
params.putAll(leafLookup.asMap());
this.params = new DeprecationMap(params, DEPRECATIONS);
this.params = new DeprecationMap(params, DEPRECATIONS, "score-script");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public MapScript(Map<String, Object> params, Map<String, Object> state, SearchLo
if (leafLookup != null) {
params = new HashMap<>(params); // copy params so we aren't modifying input
params.putAll(leafLookup.asMap()); // add lookup vars
params = new DeprecationMap(params, DEPRECATIONS); // wrap with deprecations
params = new DeprecationMap(params, DEPRECATIONS, "map-script"); // wrap with deprecations
}
this.params = params;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public TermsSetQueryScript(Map<String, Object> params, SearchLookup lookup, Leaf
Map<String, Object> parameters = new HashMap<>(params);
this.leafLookup = lookup.getLeafSearchLookup(leafContext);
parameters.putAll(leafLookup.asMap());
this.params = new DeprecationMap(parameters, DEPRECATIONS);
this.params = new DeprecationMap(parameters, DEPRECATIONS, "term-set-query-script");
}

protected TermsSetQueryScript() {
Expand Down
14 changes: 13 additions & 1 deletion server/src/main/java/org/elasticsearch/script/UpdateScript.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,25 @@

package org.elasticsearch.script;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

/**
* An update script.
*/
public abstract class UpdateScript {

private static final Map<String, String> DEPRECATIONS;
static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"_type",
"[types removal] Looking up doc types [_type] in scripts is deprecated."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the prefix [types removal]? I dont' think we have any prefixes like that in other deprecation messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had to choose a shared prefix in order for there to be a consistent way to detect types deprecation messages in REST tests (and ignore them). I think @jdconrad is just using this prefix here for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from the other types removal messages. I'd like to keep it for consistency unless there are other objections.

);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}

public static final String[] PARAMETERS = { };

/** The context used to compile {@link UpdateScript} factories. */
Expand All @@ -40,7 +52,7 @@ public abstract class UpdateScript {

public UpdateScript(Map<String, Object> params, Map<String, Object> ctx) {
this.params = params;
this.ctx = ctx;
this.ctx = new DeprecationMap(ctx, DEPRECATIONS, "update-script");
}

/** Return the parameters for this script. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class LeafDocLookup implements Map<String, ScriptDocValues<?>> {
= new DeprecationLogger(LogManager.getLogger(LeafDocLookup.class));
static final String TYPES_DEPRECATION_KEY = "type-field-doc-lookup";
static final String TYPES_DEPRECATION_MESSAGE =
"[types removal] Looking up doc types in scripts is deprecated.";
"[types removal] Looking up doc types [_type] in scripts is deprecated.";

private final Map<String, ScriptDocValues<?>> localCacheFieldData = new HashMap<>(4);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,54 @@ public void testActsOnImmutableData() throws Exception {
assertMutatingCtxThrows(ctx -> ((List<Object>)ctx.get("listField")).remove("bar"));
}

public void testTypeDeprecation() throws Exception {
String scriptName = "conditionalScript";
ScriptService scriptService = new ScriptService(Settings.builder().build(),
Collections.singletonMap(
Script.DEFAULT_SCRIPT_LANG,
new MockScriptEngine(
Script.DEFAULT_SCRIPT_LANG,
Collections.singletonMap(
scriptName, ctx -> {
ctx.get("_type");
return true;
}
),
Collections.emptyMap()
)
),
new HashMap<>(ScriptModule.CORE_CONTEXTS)
);

LongSupplier relativeTimeProvider = mock(LongSupplier.class);
when(relativeTimeProvider.getAsLong()).thenReturn(0L, TimeUnit.MILLISECONDS.toNanos(1), 0L, TimeUnit.MILLISECONDS.toNanos(2));
ConditionalProcessor processor = new ConditionalProcessor(
randomAlphaOfLength(10),
new Script(
ScriptType.INLINE, Script.DEFAULT_SCRIPT_LANG,
scriptName, Collections.emptyMap()), scriptService,
new Processor() {
@Override
public IngestDocument execute(final IngestDocument ingestDocument){
return ingestDocument;
}

@Override
public String getType() {
return null;
}

@Override
public String getTag() {
return null;
}
}, relativeTimeProvider);

IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), Collections.emptyMap());
processor.execute(ingestDocument);
assertWarnings("[types removal] Looking up doc types [_type] in scripts is deprecated.");
}

private static void assertMutatingCtxThrows(Consumer<Map<String, Object>> mutation) throws Exception {
String scriptName = "conditionalScript";
CompletableFuture<Exception> expectedException = new CompletableFuture<>();
Expand Down