Skip to content

Commit 37e97d7

Browse files
author
Christoph Büscher
authored
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 83be984 commit 37e97d7

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_8_0_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

@@ -346,21 +354,33 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
346354

347355
@Override
348356
public void writeTo(StreamOutput out) throws IOException {
349-
if (tokens != null) {
350-
out.writeVInt(tokens.size());
351-
for (AnalyzeToken token : tokens) {
352-
token.writeTo(out);
357+
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
358+
AnalyzeToken[] tokenArray = null;
359+
if (tokens != null) {
360+
tokenArray = tokens.toArray(new AnalyzeToken[0]);
353361
}
362+
out.writeOptionalArray(tokenArray);
354363
} else {
355-
out.writeVInt(0);
364+
if (tokens != null) {
365+
out.writeVInt(tokens.size());
366+
for (AnalyzeToken token : tokens) {
367+
token.writeTo(out);
368+
}
369+
} else {
370+
out.writeVInt(0);
371+
}
356372
}
357373
out.writeOptionalWriteable(detail);
358374
}
359375

360376
@Override
361377
public boolean equals(Object o) {
362-
if (this == o) return true;
363-
if (o == null || getClass() != o.getClass()) return false;
378+
if (this == o) {
379+
return true;
380+
}
381+
if (o == null || getClass() != o.getClass()) {
382+
return false;
383+
}
364384
Response that = (Response) o;
365385
return Objects.equals(detail, that.detail) &&
366386
Objects.equals(tokens, that.tokens);
@@ -401,8 +421,12 @@ public static class AnalyzeToken implements Writeable, ToXContentObject {
401421

402422
@Override
403423
public boolean equals(Object o) {
404-
if (this == o) return true;
405-
if (o == null || getClass() != o.getClass()) return false;
424+
if (this == o) {
425+
return true;
426+
}
427+
if (o == null || getClass() != o.getClass()) {
428+
return false;
429+
}
406430
AnalyzeToken that = (AnalyzeToken) o;
407431
return startOffset == that.startOffset &&
408432
endOffset == that.endOffset &&
@@ -582,8 +606,12 @@ public AnalyzeTokenList[] tokenfilters() {
582606

583607
@Override
584608
public boolean equals(Object o) {
585-
if (this == o) return true;
586-
if (o == null || getClass() != o.getClass()) return false;
609+
if (this == o) {
610+
return true;
611+
}
612+
if (o == null || getClass() != o.getClass()) {
613+
return false;
614+
}
587615
DetailAnalyzeResponse that = (DetailAnalyzeResponse) o;
588616
return customAnalyzer == that.customAnalyzer &&
589617
Objects.equals(analyzer, that.analyzer) &&
@@ -669,8 +697,12 @@ public static class AnalyzeTokenList implements Writeable, ToXContentObject {
669697

670698
@Override
671699
public boolean equals(Object o) {
672-
if (this == o) return true;
673-
if (o == null || getClass() != o.getClass()) return false;
700+
if (this == o) {
701+
return true;
702+
}
703+
if (o == null || getClass() != o.getClass()) {
704+
return false;
705+
}
674706
AnalyzeTokenList that = (AnalyzeTokenList) o;
675707
return Objects.equals(name, that.name) &&
676708
Arrays.equals(tokens, that.tokens);
@@ -690,16 +722,19 @@ public AnalyzeTokenList(String name, AnalyzeToken[] tokens) {
690722

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

705740
public String getName() {
@@ -732,13 +767,17 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
732767
@Override
733768
public void writeTo(StreamOutput out) throws IOException {
734769
out.writeString(name);
735-
if (tokens != null) {
736-
out.writeVInt(tokens.length);
737-
for (AnalyzeToken token : tokens) {
738-
token.writeTo(out);
739-
}
770+
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
771+
out.writeOptionalArray(tokens);
740772
} else {
741-
out.writeVInt(0);
773+
if (tokens != null) {
774+
out.writeVInt(tokens.length);
775+
for (AnalyzeToken token : tokens) {
776+
token.writeTo(out);
777+
}
778+
} else {
779+
out.writeVInt(0);
780+
}
742781
}
743782
}
744783
}
@@ -789,8 +828,12 @@ public void writeTo(StreamOutput out) throws IOException {
789828

790829
@Override
791830
public boolean equals(Object o) {
792-
if (this == o) return true;
793-
if (o == null || getClass() != o.getClass()) return false;
831+
if (this == o) {
832+
return true;
833+
}
834+
if (o == null || getClass() != o.getClass()) {
835+
return false;
836+
}
794837
CharFilteredText that = (CharFilteredText) o;
795838
return Objects.equals(name, that.name) &&
796839
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)