Skip to content

Commit ef16bea

Browse files
author
Christoph Büscher
committed
Fix AnalyzeAction response serialization (#44284)
Currently we loose information about whether a token list in an AnalyzeAction response is null or an empty list, because we write a 0 value to the stream in both cases and deserialize to a null value on the receiving side. This change fixes this so we write an additional flag indicating whether the value is null or not, followed by the size of the list and its content. Closes #44078
1 parent 9ee7062 commit ef16bea

File tree

5 files changed

+207
-84
lines changed

5 files changed

+207
-84
lines changed

‎client/rest-high-level/src/test/java/org/elasticsearch/client/indices/AnalyzeResponseTests.java‎

Lines changed: 2 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,10 @@
2222
import org.elasticsearch.action.admin.indices.analyze.AnalyzeAction;
2323
import org.elasticsearch.client.AbstractResponseTestCase;
2424
import org.elasticsearch.common.xcontent.XContentParser;
25+
import org.elasticsearch.test.RandomObjects;
2526

2627
import java.io.IOException;
27-
import java.util.ArrayList;
2828
import java.util.Arrays;
29-
import java.util.HashMap;
30-
import java.util.List;
31-
import java.util.Map;
3229

3330
public class AnalyzeResponseTests extends AbstractResponseTestCase<AnalyzeAction.Response, AnalyzeResponse> {
3431

@@ -37,7 +34,7 @@ protected AnalyzeAction.Response createServerTestInstance() {
3734
int tokenCount = randomIntBetween(1, 30);
3835
AnalyzeAction.AnalyzeToken[] tokens = new AnalyzeAction.AnalyzeToken[tokenCount];
3936
for (int i = 0; i < tokenCount; i++) {
40-
tokens[i] = randomToken();
37+
tokens[i] = RandomObjects.randomToken(random());
4138
}
4239
if (randomBoolean()) {
4340
AnalyzeAction.CharFilteredText[] charfilters = null;
@@ -62,45 +59,6 @@ protected AnalyzeAction.Response createServerTestInstance() {
6259
return new AnalyzeAction.Response(Arrays.asList(tokens), null);
6360
}
6461

65-
private AnalyzeAction.AnalyzeToken randomToken() {
66-
String token = randomAlphaOfLengthBetween(1, 20);
67-
int position = randomIntBetween(0, 1000);
68-
int startOffset = randomIntBetween(0, 1000);
69-
int endOffset = randomIntBetween(0, 1000);
70-
int posLength = randomIntBetween(1, 5);
71-
String type = randomAlphaOfLengthBetween(1, 20);
72-
Map<String, Object> extras = new HashMap<>();
73-
if (randomBoolean()) {
74-
int entryCount = randomInt(6);
75-
for (int i = 0; i < entryCount; i++) {
76-
switch (randomInt(6)) {
77-
case 0:
78-
case 1:
79-
case 2:
80-
case 3:
81-
String key = randomAlphaOfLength(5);
82-
String value = randomAlphaOfLength(10);
83-
extras.put(key, value);
84-
break;
85-
case 4:
86-
String objkey = randomAlphaOfLength(5);
87-
Map<String, String> obj = new HashMap<>();
88-
obj.put(randomAlphaOfLength(5), randomAlphaOfLength(10));
89-
extras.put(objkey, obj);
90-
break;
91-
case 5:
92-
String listkey = randomAlphaOfLength(5);
93-
List<String> list = new ArrayList<>();
94-
list.add(randomAlphaOfLength(4));
95-
list.add(randomAlphaOfLength(6));
96-
extras.put(listkey, list);
97-
break;
98-
}
99-
}
100-
}
101-
return new AnalyzeAction.AnalyzeToken(token, position, startOffset, endOffset, posLength, type, extras);
102-
}
103-
10462
@Override
10563
protected AnalyzeResponse doParseToClientInstance(XContentParser parser) throws IOException {
10664
return AnalyzeResponse.fromXContent(parser);

‎server/src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeAction.java‎

Lines changed: 81 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@
1919

2020
package org.elasticsearch.action.admin.indices.analyze;
2121

22-
import org.elasticsearch.action.ActionType;
22+
import org.elasticsearch.Version;
2323
import org.elasticsearch.action.ActionRequestValidationException;
2424
import org.elasticsearch.action.ActionResponse;
25+
import org.elasticsearch.action.ActionType;
2526
import org.elasticsearch.action.support.single.shard.SingleShardRequest;
2627
import org.elasticsearch.common.ParseField;
2728
import org.elasticsearch.common.Strings;
@@ -292,22 +293,29 @@ public static class Response extends ActionResponse implements ToXContentObject
292293
private final List<AnalyzeToken> tokens;
293294

294295
public Response(List<AnalyzeToken> tokens, DetailAnalyzeResponse detail) {
296+
if (tokens == null && detail == null) {
297+
throw new IllegalArgumentException("Neither token nor detail set on AnalysisAction.Response");
298+
}
295299
this.tokens = tokens;
296300
this.detail = detail;
297301
}
298302

299303
public Response(StreamInput in) throws IOException {
300304
super.readFrom(in);
301-
int size = in.readVInt();
302-
if (size > 0) {
303-
tokens = new ArrayList<>(size);
304-
for (int i = 0; i < size; i++) {
305-
tokens.add(new AnalyzeToken(in));
305+
if (in.getVersion().onOrAfter(Version.V_7_3_0)) {
306+
AnalyzeToken[] tokenArray = in.readOptionalArray(AnalyzeToken::new, AnalyzeToken[]::new);
307+
tokens = tokenArray != null ? Arrays.asList(tokenArray) : null;
308+
} else {
309+
int size = in.readVInt();
310+
if (size > 0) {
311+
tokens = new ArrayList<>(size);
312+
for (int i = 0; i < size; i++) {
313+
tokens.add(new AnalyzeToken(in));
314+
}
315+
} else {
316+
tokens = null;
306317
}
307318
}
308-
else {
309-
tokens = null;
310-
}
311319
detail = in.readOptionalWriteable(DetailAnalyzeResponse::new);
312320
}
313321

@@ -347,21 +355,33 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
347355
@Override
348356
public void writeTo(StreamOutput out) throws IOException {
349357
super.writeTo(out);
350-
if (tokens != null) {
351-
out.writeVInt(tokens.size());
352-
for (AnalyzeToken token : tokens) {
353-
token.writeTo(out);
358+
if (out.getVersion().onOrAfter(Version.V_7_3_0)) {
359+
AnalyzeToken[] tokenArray = null;
360+
if (tokens != null) {
361+
tokenArray = tokens.toArray(new AnalyzeToken[0]);
354362
}
363+
out.writeOptionalArray(tokenArray);
355364
} else {
356-
out.writeVInt(0);
365+
if (tokens != null) {
366+
out.writeVInt(tokens.size());
367+
for (AnalyzeToken token : tokens) {
368+
token.writeTo(out);
369+
}
370+
} else {
371+
out.writeVInt(0);
372+
}
357373
}
358374
out.writeOptionalWriteable(detail);
359375
}
360376

361377
@Override
362378
public boolean equals(Object o) {
363-
if (this == o) return true;
364-
if (o == null || getClass() != o.getClass()) return false;
379+
if (this == o) {
380+
return true;
381+
}
382+
if (o == null || getClass() != o.getClass()) {
383+
return false;
384+
}
365385
Response that = (Response) o;
366386
return Objects.equals(detail, that.detail) &&
367387
Objects.equals(tokens, that.tokens);
@@ -402,8 +422,12 @@ public static class AnalyzeToken implements Writeable, ToXContentObject {
402422

403423
@Override
404424
public boolean equals(Object o) {
405-
if (this == o) return true;
406-
if (o == null || getClass() != o.getClass()) return false;
425+
if (this == o) {
426+
return true;
427+
}
428+
if (o == null || getClass() != o.getClass()) {
429+
return false;
430+
}
407431
AnalyzeToken that = (AnalyzeToken) o;
408432
return startOffset == that.startOffset &&
409433
endOffset == that.endOffset &&
@@ -583,8 +607,12 @@ public AnalyzeTokenList[] tokenfilters() {
583607

584608
@Override
585609
public boolean equals(Object o) {
586-
if (this == o) return true;
587-
if (o == null || getClass() != o.getClass()) return false;
610+
if (this == o) {
611+
return true;
612+
}
613+
if (o == null || getClass() != o.getClass()) {
614+
return false;
615+
}
588616
DetailAnalyzeResponse that = (DetailAnalyzeResponse) o;
589617
return customAnalyzer == that.customAnalyzer &&
590618
Objects.equals(analyzer, that.analyzer) &&
@@ -670,8 +698,12 @@ public static class AnalyzeTokenList implements Writeable, ToXContentObject {
670698

671699
@Override
672700
public boolean equals(Object o) {
673-
if (this == o) return true;
674-
if (o == null || getClass() != o.getClass()) return false;
701+
if (this == o) {
702+
return true;
703+
}
704+
if (o == null || getClass() != o.getClass()) {
705+
return false;
706+
}
675707
AnalyzeTokenList that = (AnalyzeTokenList) o;
676708
return Objects.equals(name, that.name) &&
677709
Arrays.equals(tokens, that.tokens);
@@ -691,16 +723,19 @@ public AnalyzeTokenList(String name, AnalyzeToken[] tokens) {
691723

692724
AnalyzeTokenList(StreamInput in) throws IOException {
693725
name = in.readString();
694-
int size = in.readVInt();
695-
if (size > 0) {
696-
tokens = new AnalyzeToken[size];
697-
for (int i = 0; i < size; i++) {
698-
tokens[i] = new AnalyzeToken(in);
726+
if (in.getVersion().onOrAfter(Version.V_7_3_0)) {
727+
tokens = in.readOptionalArray(AnalyzeToken::new, AnalyzeToken[]::new);
728+
} else {
729+
int size = in.readVInt();
730+
if (size > 0) {
731+
tokens = new AnalyzeToken[size];
732+
for (int i = 0; i < size; i++) {
733+
tokens[i] = new AnalyzeToken(in);
734+
}
735+
} else {
736+
tokens = null;
699737
}
700738
}
701-
else {
702-
tokens = null;
703-
}
704739
}
705740

706741
public String getName() {
@@ -733,13 +768,17 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
733768
@Override
734769
public void writeTo(StreamOutput out) throws IOException {
735770
out.writeString(name);
736-
if (tokens != null) {
737-
out.writeVInt(tokens.length);
738-
for (AnalyzeToken token : tokens) {
739-
token.writeTo(out);
740-
}
771+
if (out.getVersion().onOrAfter(Version.V_7_3_0)) {
772+
out.writeOptionalArray(tokens);
741773
} else {
742-
out.writeVInt(0);
774+
if (tokens != null) {
775+
out.writeVInt(tokens.length);
776+
for (AnalyzeToken token : tokens) {
777+
token.writeTo(out);
778+
}
779+
} else {
780+
out.writeVInt(0);
781+
}
743782
}
744783
}
745784
}
@@ -790,8 +829,12 @@ public void writeTo(StreamOutput out) throws IOException {
790829

791830
@Override
792831
public boolean equals(Object o) {
793-
if (this == o) return true;
794-
if (o == null || getClass() != o.getClass()) return false;
832+
if (this == o) {
833+
return true;
834+
}
835+
if (o == null || getClass() != o.getClass()) {
836+
return false;
837+
}
795838
CharFilteredText that = (CharFilteredText) o;
796839
return Objects.equals(name, that.name) &&
797840
Arrays.equals(texts, that.texts);

‎server/src/test/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeResponseTests.java‎

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,25 @@
1919

2020
package org.elasticsearch.action.admin.indices.analyze;
2121

22+
import org.elasticsearch.action.admin.indices.analyze.AnalyzeAction.AnalyzeToken;
2223
import org.elasticsearch.common.bytes.BytesReference;
24+
import org.elasticsearch.common.io.stream.Writeable.Reader;
2325
import org.elasticsearch.common.xcontent.ToXContent;
2426
import org.elasticsearch.common.xcontent.XContentBuilder;
2527
import org.elasticsearch.common.xcontent.XContentHelper;
2628
import org.elasticsearch.common.xcontent.json.JsonXContent;
27-
import org.elasticsearch.test.ESTestCase;
29+
import org.elasticsearch.test.AbstractWireSerializingTestCase;
30+
import org.elasticsearch.test.RandomObjects;
2831

2932
import java.io.IOException;
33+
import java.util.ArrayList;
34+
import java.util.Arrays;
3035
import java.util.List;
3136
import java.util.Map;
3237

3338
import static org.hamcrest.Matchers.equalTo;
3439

35-
public class AnalyzeResponseTests extends ESTestCase {
40+
public class AnalyzeResponseTests extends AbstractWireSerializingTestCase<AnalyzeAction.Response> {
3641

3742
@SuppressWarnings("unchecked")
3843
public void testNullResponseToXContent() throws IOException {
@@ -59,6 +64,64 @@ public void testNullResponseToXContent() throws IOException {
5964
assertThat(nullTokens.size(), equalTo(0));
6065
assertThat(name, equalTo(nameValue));
6166
}
67+
}
68+
69+
public void testConstructorArgs() {
70+
IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new AnalyzeAction.Response(null, null));
71+
assertEquals("Neither token nor detail set on AnalysisAction.Response", ex.getMessage());
72+
}
6273

74+
@Override
75+
protected AnalyzeAction.Response createTestInstance() {
76+
int tokenCount = randomIntBetween(0, 30);
77+
AnalyzeAction.AnalyzeToken[] tokens = new AnalyzeAction.AnalyzeToken[tokenCount];
78+
for (int i = 0; i < tokenCount; i++) {
79+
tokens[i] = RandomObjects.randomToken(random());
80+
}
81+
if (randomBoolean()) {
82+
AnalyzeAction.CharFilteredText[] charfilters = null;
83+
AnalyzeAction.AnalyzeTokenList[] tokenfilters = null;
84+
if (randomBoolean()) {
85+
charfilters = new AnalyzeAction.CharFilteredText[]{
86+
new AnalyzeAction.CharFilteredText("my_charfilter", new String[]{"one two"})
87+
};
88+
}
89+
if (randomBoolean()) {
90+
tokenfilters = new AnalyzeAction.AnalyzeTokenList[]{
91+
new AnalyzeAction.AnalyzeTokenList("my_tokenfilter_1", tokens),
92+
new AnalyzeAction.AnalyzeTokenList("my_tokenfilter_2", tokens)
93+
};
94+
}
95+
AnalyzeAction.DetailAnalyzeResponse dar = new AnalyzeAction.DetailAnalyzeResponse(
96+
charfilters,
97+
new AnalyzeAction.AnalyzeTokenList("my_tokenizer", tokens),
98+
tokenfilters);
99+
return new AnalyzeAction.Response(null, dar);
100+
}
101+
return new AnalyzeAction.Response(Arrays.asList(tokens), null);
63102
}
103+
104+
/**
105+
* Either add a token to the token list or change the details token list name
106+
*/
107+
@Override
108+
protected AnalyzeAction.Response mutateInstance(AnalyzeAction.Response instance) throws IOException {
109+
if (instance.getTokens() != null) {
110+
List<AnalyzeToken> extendedList = new ArrayList<>(instance.getTokens());
111+
extendedList.add(RandomObjects.randomToken(random()));
112+
return new AnalyzeAction.Response(extendedList, null);
113+
} else {
114+
AnalyzeToken[] tokens = instance.detail().tokenizer().getTokens();
115+
return new AnalyzeAction.Response(null, new AnalyzeAction.DetailAnalyzeResponse(
116+
instance.detail().charfilters(),
117+
new AnalyzeAction.AnalyzeTokenList("my_other_tokenizer", tokens),
118+
instance.detail().tokenfilters()));
119+
}
120+
}
121+
122+
@Override
123+
protected Reader<AnalyzeAction.Response> instanceReader() {
124+
return AnalyzeAction.Response::new;
125+
}
126+
64127
}

‎server/src/test/java/org/elasticsearch/indices/analyze/AnalyzeActionIT.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.elasticsearch.common.settings.Settings;
2525
import org.elasticsearch.plugins.Plugin;
2626
import org.elasticsearch.test.ESIntegTestCase;
27+
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
2728
import org.elasticsearch.test.MockKeywordPlugin;
2829
import org.hamcrest.core.IsNull;
2930

@@ -41,6 +42,7 @@
4142
import static org.hamcrest.Matchers.is;
4243
import static org.hamcrest.Matchers.startsWith;
4344

45+
@ClusterScope(minNumDataNodes = 2)
4446
public class AnalyzeActionIT extends ESIntegTestCase {
4547

4648
@Override
@@ -387,5 +389,16 @@ public void testAnalyzeNormalizedKeywordField() throws IOException {
387389
assertThat(token.getPositionLength(), equalTo(1));
388390
}
389391

392+
/**
393+
* Input text that doesn't produce tokens should return an empty token list
394+
*/
395+
public void testZeroTokenAnalysis() throws IOException {
396+
assertAcked(prepareCreate("test"));
397+
ensureGreen("test");
398+
399+
AnalyzeAction.Response analyzeResponse = client().admin().indices().prepareAnalyze("test", ".").get();
400+
assertNotNull(analyzeResponse.getTokens());
401+
assertThat(analyzeResponse.getTokens().size(), equalTo(0));
402+
}
390403

391404
}

0 commit comments

Comments
 (0)