Skip to content

Commit 64d1be9

Browse files
mayya-sharipovamayya-sharipova
authored
Fix warnings and tests for missing script values (#32382)
For main code: - ScriptModule will NOT produce a deprecation warning anymore, when `es.scripting.exception_for_missing_value` is not set. This warning is produced only when missing values in script are used and is produced only once. For tests: - make `es.scripting.exception_for_missing_value` equal to `false` by default in tests. This behaviour is similar with default node settings, and with tests running from IDE. - modify `ScriptDocValuesMissingV6BehaviourTests` to test the default behaviour in v6, where there are default values for missing values. - add `ScriptDocValuesMissingV7BehaviourTests` to test the default behavior in v7 where `es.scripting.exception_for_missing_value` is set to `true`, and where exceptions are produced for missing values. Relates to #29286 * Address feedback * Address feedback 2
1 parent eee0086 commit 64d1be9

File tree

9 files changed

+441
-118
lines changed

9 files changed

+441
-118
lines changed

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

-1
Original file line numberDiff line numberDiff line change
@@ -807,7 +807,6 @@ class BuildPlugin implements Plugin<Project> {
807807
systemProperty 'jna.nosys', 'true'
808808
// TODO: remove this deprecation compatibility setting for 7.0
809809
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
810-
systemProperty 'es.scripting.exception_for_missing_value', 'true'
811810
systemProperty 'compiler.java', project.ext.compilerJavaVersion.getMajorVersion()
812811
if (project.ext.inFipsJvm) {
813812
systemProperty 'runtime.java', project.ext.runtimeJavaVersion.getMajorVersion() + "FIPS"

‎docs/painless/painless-getting-started.asciidoc

+3-2
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,9 @@ the document, `doc['field'].value` for this document returns:
136136
IMPORTANT: Starting in 7.0, `doc['field'].value` throws an exception if
137137
the field is missing in a document. To enable this behavior now,
138138
set a {ref}/jvm-options.html[`jvm.option`]
139-
`-Des.scripting.exception_for_missing_value=true` on a node. If you do not enable
140-
this behavior, a deprecation warning is logged on start up.
139+
`-Des.scripting.exception_for_missing_value=true` on a node. If you do not
140+
enable this behavior, a deprecation warning may be logged when painless
141+
processes a missing value.
141142

142143
To check if a document is missing a value, you can call
143144
`doc['field'].size() == 0`.

‎server/build.gradle

+4-5
Original file line numberDiff line numberDiff line change
@@ -156,14 +156,13 @@ if (isEclipse) {
156156
compileJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked"
157157
compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked"
158158

159-
// TODO: remove ScriptDocValuesMissingV6BehaviourTests in 7.0
160-
additionalTest('testScriptDocValuesMissingV6Behaviour'){
161-
include '**/ScriptDocValuesMissingV6BehaviourTests.class'
162-
systemProperty 'es.scripting.exception_for_missing_value', 'false'
159+
additionalTest('testScriptDocValuesMissingV7Behaviour') {
160+
include '**/ScriptDocValuesMissingV7BehaviourTests.class'
161+
systemProperty 'es.scripting.exception_for_missing_value', 'true'
163162
}
164163
test {
165164
// these are tested explicitly in separate test tasks
166-
exclude '**/*ScriptDocValuesMissingV6BehaviourTests.class'
165+
exclude '**/*ScriptDocValuesMissingV7BehaviourTests.class'
167166
}
168167

169168
forbiddenPatterns {

‎server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

+183-26
Large diffs are not rendered by default.

‎server/src/main/java/org/elasticsearch/script/ScriptModule.java

-12
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,6 @@
1919

2020
package org.elasticsearch.script;
2121

22-
import org.elasticsearch.common.Booleans;
23-
import org.elasticsearch.common.logging.DeprecationLogger;
24-
import org.elasticsearch.common.logging.Loggers;
2522
import org.elasticsearch.common.settings.ClusterSettings;
2623
import org.elasticsearch.common.settings.Settings;
2724
import org.elasticsearch.plugins.ScriptPlugin;
@@ -67,11 +64,6 @@ public class ScriptModule {
6764
).collect(Collectors.toMap(c -> c.name, Function.identity()));
6865
}
6966

70-
public static final boolean EXCEPTION_FOR_MISSING_VALUE =
71-
Booleans.parseBoolean(System.getProperty("es.scripting.exception_for_missing_value", "false"));
72-
73-
private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ScriptModule.class));
74-
7567
private final ScriptService scriptService;
7668

7769
public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
@@ -95,10 +87,6 @@ public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
9587
}
9688
}
9789
}
98-
if (EXCEPTION_FOR_MISSING_VALUE == false)
99-
DEPRECATION_LOGGER.deprecated("Script: returning default values for missing document values is deprecated. " +
100-
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
101-
"to make behaviour compatible with future major versions.");
10290
scriptService = new ScriptService(settings, Collections.unmodifiableMap(engines), Collections.unmodifiableMap(contexts));
10391
}
10492

‎server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java

+16-19
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,10 @@
3636
import java.time.ZonedDateTime;
3737
import java.util.HashSet;
3838
import java.util.Set;
39-
import java.util.function.Consumer;
39+
import java.util.function.BiConsumer;
4040
import java.util.function.Function;
4141

42-
import static org.hamcrest.Matchers.containsInAnyOrder;
42+
import static org.hamcrest.Matchers.hasItems;
4343

4444
public class ScriptDocValuesDatesTests extends ESTestCase {
4545

@@ -74,7 +74,7 @@ public void assertDateDocValues(boolean useJavaTime, String... expectedWarnings)
7474
}
7575

7676
Set<String> warnings = new HashSet<>();
77-
Dates dates = wrap(values, deprecationMessage -> {
77+
Dates dates = wrap(values, (key, deprecationMessage) -> {
7878
warnings.add(deprecationMessage);
7979
/* Create a temporary directory to prove we are running with the
8080
* server's permissions. */
@@ -91,29 +91,26 @@ public void assertDateDocValues(boolean useJavaTime, String... expectedWarnings)
9191
for (int round = 0; round < 10; round++) {
9292
int d = between(0, values.length - 1);
9393
dates.setNextDocId(d);
94-
if (expectedDates[d].length > 0) {
95-
Object dateValue = AccessController.doPrivileged((PrivilegedAction<Object>) dates::getValue, noPermissionsAcc);
96-
assertEquals(expectedDates[d][0] , dateValue);
97-
Object bwcDateValue = AccessController.doPrivileged((PrivilegedAction<Object>) dates::getDate, noPermissionsAcc);
98-
assertEquals(expectedDates[d][0] , bwcDateValue);
99-
AccessController.doPrivileged((PrivilegedAction<Object>) dates::getDates, noPermissionsAcc);
100-
} else {
101-
Exception e = expectThrows(IllegalStateException.class, () -> dates.getValue());
102-
assertEquals("A document doesn't have a value for a field! " +
103-
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
104-
}
94+
Object dateValue = AccessController.doPrivileged((PrivilegedAction<Object>) dates::getValue, noPermissionsAcc);
95+
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dateValue);
96+
Object bwcDateValue = AccessController.doPrivileged((PrivilegedAction<Object>) dates::getDate, noPermissionsAcc);
97+
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), bwcDateValue);
98+
99+
AccessController.doPrivileged((PrivilegedAction<Object>) dates::getDates, noPermissionsAcc);
105100
assertEquals(values[d].length, dates.size());
106101
for (int i = 0; i < values[d].length; i++) {
107102
final int ndx = i;
108-
Object dateValue = AccessController.doPrivileged((PrivilegedAction<Object>) () -> dates.get(ndx), noPermissionsAcc);
109-
assertEquals(expectedDates[d][i], dateValue);
103+
Object dateValueI = AccessController.doPrivileged((PrivilegedAction<Object>) () -> dates.get(ndx), noPermissionsAcc);
104+
assertEquals(expectedDates[d][i], dateValueI);
110105
}
111106
}
112-
113-
assertThat(warnings, containsInAnyOrder(expectedWarnings));
107+
// using "hasItems" here instead of "containsInAnyOrder",
108+
// because values are randomly initialized, sometimes some of docs will not have any values
109+
// and warnings in this case will contain another deprecation warning on missing values
110+
assertThat(warnings, hasItems(expectedWarnings));
114111
}
115112

116-
private Dates wrap(long[][] values, Consumer<String> deprecationHandler, boolean useJavaTime) {
113+
private Dates wrap(long[][] values, BiConsumer<String, String> deprecationHandler, boolean useJavaTime) {
117114
return new Dates(new AbstractSortedNumericDocValues() {
118115
long[] current;
119116
int i;

‎server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java

+17-21
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import org.elasticsearch.index.fielddata.ScriptDocValues.Longs;
2323
import org.elasticsearch.test.ESTestCase;
24+
import org.joda.time.DateTime;
25+
import org.joda.time.DateTimeZone;
2426

2527
import java.io.IOException;
2628
import java.security.AccessControlContext;
@@ -34,9 +36,10 @@
3436
import java.time.ZonedDateTime;
3537
import java.util.HashSet;
3638
import java.util.Set;
37-
import java.util.function.Consumer;
39+
import java.util.function.BiConsumer;
40+
41+
import static org.hamcrest.Matchers.hasItems;
3842

39-
import static org.hamcrest.Matchers.containsInAnyOrder;
4043

4144
public class ScriptDocValuesLongsTests extends ESTestCase {
4245
public void testLongs() throws IOException {
@@ -47,18 +50,15 @@ public void testLongs() throws IOException {
4750
values[d][i] = randomLong();
4851
}
4952
}
50-
Longs longs = wrap(values, deprecationMessage -> {fail("unexpected deprecation: " + deprecationMessage);});
53+
Set<String> warnings = new HashSet<>();
54+
Longs longs = wrap(values, (key, deprecationMessage) -> {
55+
warnings.add(deprecationMessage);
56+
});
5157

5258
for (int round = 0; round < 10; round++) {
5359
int d = between(0, values.length - 1);
5460
longs.setNextDocId(d);
55-
if (values[d].length > 0) {
56-
assertEquals(values[d][0], longs.getValue());
57-
} else {
58-
Exception e = expectThrows(IllegalStateException.class, () -> longs.getValue());
59-
assertEquals("A document doesn't have a value for a field! " +
60-
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
61-
}
61+
assertEquals(values[d].length > 0 ? values[d][0] : 0, longs.getValue());
6262
assertEquals(values[d].length, longs.size());
6363
assertEquals(values[d].length, longs.getValues().size());
6464
for (int i = 0; i < values[d].length; i++) {
@@ -84,7 +84,7 @@ public void testDates() throws IOException {
8484
}
8585
}
8686
Set<String> warnings = new HashSet<>();
87-
Longs longs = wrap(values, deprecationMessage -> {
87+
Longs longs = wrap(values, (key, deprecationMessage) -> {
8888
warnings.add(deprecationMessage);
8989
/* Create a temporary directory to prove we are running with the
9090
* server's permissions. */
@@ -94,14 +94,7 @@ public void testDates() throws IOException {
9494
for (int round = 0; round < 10; round++) {
9595
int d = between(0, values.length - 1);
9696
longs.setNextDocId(d);
97-
if (dates[d].length > 0) {
98-
assertEquals(dates[d][0], longs.getDate());
99-
} else {
100-
Exception e = expectThrows(IllegalStateException.class, () -> longs.getDate());
101-
assertEquals("A document doesn't have a value for a field! " +
102-
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
103-
}
104-
97+
assertEquals(dates[d].length > 0 ? dates[d][0] : new DateTime(0, DateTimeZone.UTC), longs.getDate());
10598
assertEquals(values[d].length, longs.getDates().size());
10699
for (int i = 0; i < values[d].length; i++) {
107100
assertEquals(dates[d][i], longs.getDates().get(i));
@@ -135,12 +128,15 @@ public Void run() {
135128
}
136129
}, noPermissionsAcc);
137130

138-
assertThat(warnings, containsInAnyOrder(
131+
// using "hasItems" here instead of "containsInAnyOrder",
132+
// because values are randomly initialized, sometimes some of docs will not have any values
133+
// and warnings in this case will contain another deprecation warning on missing values
134+
assertThat(warnings, hasItems(
139135
"getDate on numeric fields is deprecated. Use a date field to get dates.",
140136
"getDates on numeric fields is deprecated. Use a date field to get dates."));
141137
}
142138

143-
private Longs wrap(long[][] values, Consumer<String> deprecationCallback) {
139+
private Longs wrap(long[][] values, BiConsumer<String, String> deprecationCallback) {
144140
return new Longs(new AbstractSortedNumericDocValues() {
145141
long[] current;
146142
int i;

‎server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java

+37-32
Original file line numberDiff line numberDiff line change
@@ -19,52 +19,41 @@
1919

2020
package org.elasticsearch.index.fielddata;
2121

22-
import org.elasticsearch.common.geo.GeoPoint;
23-
import org.elasticsearch.common.settings.Settings;
22+
import org.elasticsearch.common.geo.GeoPoint;;
2423
import org.elasticsearch.index.fielddata.ScriptDocValues.Longs;
2524
import org.elasticsearch.index.fielddata.ScriptDocValues.Dates;
2625
import org.elasticsearch.index.fielddata.ScriptDocValues.Booleans;
27-
import org.elasticsearch.plugins.ScriptPlugin;
28-
import org.elasticsearch.script.MockScriptEngine;
29-
import org.elasticsearch.script.ScriptContext;
30-
import org.elasticsearch.script.ScriptEngine;
31-
import org.elasticsearch.script.ScriptModule;
3226
import org.elasticsearch.test.ESTestCase;
3327

3428
import org.joda.time.DateTime;
3529
import org.joda.time.DateTimeZone;
3630
import org.joda.time.ReadableDateTime;
3731
import java.io.IOException;
38-
import java.util.Collection;
39-
import java.util.Collections;
32+
import java.util.HashSet;
33+
import java.util.Set;
34+
import java.util.function.BiConsumer;
4035

41-
import static java.util.Collections.singletonList;
36+
import static org.hamcrest.Matchers.contains;
37+
import static org.hamcrest.Matchers.equalTo;
4238

4339
public class ScriptDocValuesMissingV6BehaviourTests extends ESTestCase {
4440

45-
public void testScriptMissingValuesWarning(){
46-
new ScriptModule(Settings.EMPTY, singletonList(new ScriptPlugin() {
47-
@Override
48-
public ScriptEngine getScriptEngine(Settings settings, Collection<ScriptContext<?>> contexts) {
49-
return new MockScriptEngine(MockScriptEngine.NAME, Collections.singletonMap("1", script -> "1"));
50-
}
51-
}));
52-
assertWarnings("Script: returning default values for missing document values is deprecated. " +
53-
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
54-
"to make behaviour compatible with future major versions.");
55-
}
56-
5741
public void testZeroForMissingValueLong() throws IOException {
5842
long[][] values = new long[between(3, 10)][];
5943
for (int d = 0; d < values.length; d++) {
6044
values[d] = new long[0];
6145
}
62-
Longs longs = wrap(values);
46+
Set<String> warnings = new HashSet<>();
47+
Longs longs = wrap(values, (key, deprecationMessage) -> { warnings.add(deprecationMessage); });
6348
for (int round = 0; round < 10; round++) {
6449
int d = between(0, values.length - 1);
6550
longs.setNextDocId(d);
6651
assertEquals(0, longs.getValue());
6752
}
53+
assertThat(warnings, contains(equalTo(
54+
"returning default values for missing document values is deprecated. " +
55+
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
56+
"to make behaviour compatible with future major versions!")));
6857
}
6958

7059
public void testEpochForMissingValueDate() throws IOException {
@@ -73,36 +62,52 @@ public void testEpochForMissingValueDate() throws IOException {
7362
for (int d = 0; d < values.length; d++) {
7463
values[d] = new long[0];
7564
}
76-
Dates dates = wrapDates(values);
65+
Set<String> warnings = new HashSet<>();
66+
Dates dates = wrapDates(values, (key, deprecationMessage) -> { warnings.add(deprecationMessage); });
7767
for (int round = 0; round < 10; round++) {
7868
int d = between(0, values.length - 1);
7969
dates.setNextDocId(d);
8070
assertEquals(EPOCH, dates.getValue());
8171
}
72+
assertThat(warnings, contains(equalTo(
73+
"returning default values for missing document values is deprecated. " +
74+
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
75+
"to make behaviour compatible with future major versions!")));
8276
}
8377

8478
public void testFalseForMissingValueBoolean() throws IOException {
8579
long[][] values = new long[between(3, 10)][];
8680
for (int d = 0; d < values.length; d++) {
8781
values[d] = new long[0];
8882
}
89-
Booleans bools = wrapBooleans(values);
83+
Set<String> warnings = new HashSet<>();
84+
Booleans bools = wrapBooleans(values, (key, deprecationMessage) -> { warnings.add(deprecationMessage); });
9085
for (int round = 0; round < 10; round++) {
9186
int d = between(0, values.length - 1);
9287
bools.setNextDocId(d);
9388
assertEquals(false, bools.getValue());
9489
}
90+
assertThat(warnings, contains(equalTo(
91+
"returning default values for missing document values is deprecated. " +
92+
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
93+
"to make behaviour compatible with future major versions!")));
9594
}
9695

9796
public void testNullForMissingValueGeo() throws IOException{
97+
Set<String> warnings = new HashSet<>();
9898
final MultiGeoPointValues values = wrap(new GeoPoint[0]);
99-
final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values);
99+
final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(
100+
values, (key, deprecationMessage) -> { warnings.add(deprecationMessage); });
100101
script.setNextDocId(0);
101102
assertEquals(null, script.getValue());
103+
assertThat(warnings, contains(equalTo(
104+
"returning default values for missing document values is deprecated. " +
105+
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
106+
"to make behaviour compatible with future major versions!")));
102107
}
103108

104109

105-
private Longs wrap(long[][] values) {
110+
private Longs wrap(long[][] values, BiConsumer<String, String> deprecationCallback) {
106111
return new Longs(new AbstractSortedNumericDocValues() {
107112
long[] current;
108113
int i;
@@ -120,10 +125,10 @@ public int docValueCount() {
120125
public long nextValue() {
121126
return current[i++];
122127
}
123-
});
128+
}, deprecationCallback);
124129
}
125130

126-
private Booleans wrapBooleans(long[][] values) {
131+
private Booleans wrapBooleans(long[][] values, BiConsumer<String, String> deprecationCallback) {
127132
return new Booleans(new AbstractSortedNumericDocValues() {
128133
long[] current;
129134
int i;
@@ -141,10 +146,10 @@ public int docValueCount() {
141146
public long nextValue() {
142147
return current[i++];
143148
}
144-
});
149+
}, deprecationCallback);
145150
}
146151

147-
private Dates wrapDates(long[][] values) {
152+
private Dates wrapDates(long[][] values, BiConsumer<String, String> deprecationCallback) {
148153
return new Dates(new AbstractSortedNumericDocValues() {
149154
long[] current;
150155
int i;
@@ -162,7 +167,7 @@ public int docValueCount() {
162167
public long nextValue() {
163168
return current[i++];
164169
}
165-
});
170+
}, deprecationCallback);
166171
}
167172

168173

0 commit comments

Comments
 (0)