Skip to content

Commit d2ff05b

Browse files
szybiaszybia
committed
niels comments
1. create better test 2. fix double-truncation discovered due to better test
1 parent 06dadee commit d2ff05b

File tree

4 files changed

+77
-90
lines changed

4 files changed

+77
-90
lines changed

‎server/src/main/java/org/elasticsearch/cluster/metadata/LifecycleExecutionState.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,11 @@ public static String potentiallyTruncateLongJsonWithExplanation(String json) {
291291
if (json == null) {
292292
return null;
293293
}
294-
if (json.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH) {
294+
final boolean jsonWithinLimit = json.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH;
295+
final boolean jsonAlreadyTruncated = json.endsWith("chars truncated)\"}");
296+
// already-truncated JSON will always be over the limit, since we add the extra `chars truncated` explanation at the end.
297+
// hence another check to not 1) double-truncate needlessly 2) lose information (we'll truncate current number of truncated chars)
298+
if (jsonWithinLimit || jsonAlreadyTruncated) {
295299
return json;
296300
}
297301
assert json.startsWith("{\"") && json.endsWith("\"}") : "expected more specific JSON format";

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.test.EqualsHashCodeTestUtils;
1414

1515
import java.util.HashMap;
16+
import java.util.List;
1617
import java.util.Map;
1718

1819
import static org.hamcrest.Matchers.equalTo;
@@ -171,6 +172,16 @@ public void testPotentiallyTruncateLongJsonWithExplanationTwoCharsTruncated() {
171172
assertEquals(expectedOutput, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
172173
}
173174

175+
public void testPotentiallyTruncateLongJsonWithExplanationEarlyReturn() {
176+
List.of(
177+
"",
178+
"chars truncated)\"}",
179+
randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH) + "chars truncated)\"}"
180+
).forEach(value -> {
181+
assertSame(value, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(value));
182+
});
183+
}
184+
174185
private LifecycleExecutionState mutate(LifecycleExecutionState toMutate) {
175186
LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate);
176187
switch (randomIntBetween(0, 18)) {

‎x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/ExplainLifecycleIT.java‎

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,10 @@
1010
import org.apache.http.entity.ContentType;
1111
import org.apache.http.entity.StringEntity;
1212
import org.apache.http.util.EntityUtils;
13-
import org.apache.logging.log4j.LogManager;
14-
import org.apache.logging.log4j.Logger;
1513
import org.elasticsearch.client.Request;
1614
import org.elasticsearch.client.Response;
1715
import org.elasticsearch.cluster.metadata.IndexMetadata;
16+
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
1817
import org.elasticsearch.common.Strings;
1918
import org.elasticsearch.common.settings.Settings;
2019
import org.elasticsearch.core.TimeValue;
@@ -44,6 +43,7 @@
4443
import static org.elasticsearch.xpack.TimeSeriesRestDriver.explainIndex;
4544
import static org.hamcrest.Matchers.allOf;
4645
import static org.hamcrest.Matchers.containsString;
46+
import static org.hamcrest.Matchers.equalTo;
4747
import static org.hamcrest.Matchers.greaterThan;
4848
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
4949
import static org.hamcrest.Matchers.hasKey;
@@ -53,7 +53,6 @@
5353
import static org.hamcrest.Matchers.nullValue;
5454

5555
public class ExplainLifecycleIT extends IlmESRestTestCase {
56-
private static final Logger logger = LogManager.getLogger(ExplainLifecycleIT.class);
5756
private static final String FAILED_STEP_RETRY_COUNT_FIELD = "failed_step_retry_count";
5857
private static final String IS_AUTO_RETRYABLE_ERROR_FIELD = "is_auto_retryable_error";
5958

@@ -321,6 +320,65 @@ public void testStepInfoPreservedOnAutoRetry() throws Exception {
321320
}, 30, TimeUnit.SECONDS);
322321
}
323322

323+
public void testTruncatedPreviousStepInfoDoesNotBreakExplainJson() throws Exception {
324+
final String policyName = "policy-" + randomAlphaOfLength(5).toLowerCase(Locale.ROOT);
325+
326+
final Request createPolice = new Request("PUT", "_ilm/policy/" + policyName);
327+
createPolice.setJsonEntity("""
328+
{
329+
"policy": {
330+
"phases": {
331+
"hot": {
332+
"actions": {
333+
"rollover": {
334+
"max_docs": 1
335+
}
336+
}
337+
}
338+
}
339+
}
340+
}
341+
""");
342+
assertOK(client().performRequest(createPolice));
343+
344+
final String indexBase = "my-logs";
345+
final String indexName = indexBase + "-" + randomAlphaOfLength(5).toLowerCase(Locale.ROOT);
346+
final String longMissingAliasName = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
347+
348+
final Request templateRequest = new Request("PUT", "_index_template/template_" + policyName);
349+
final String templateBody = Strings.format("""
350+
{
351+
"index_patterns": ["%s-*"],
352+
"template": {
353+
"settings": {
354+
"index.lifecycle.name": "%s",
355+
"index.lifecycle.rollover_alias": "%s"
356+
}
357+
}
358+
}
359+
""", indexBase, policyName, longMissingAliasName);
360+
templateRequest.setJsonEntity(templateBody);
361+
362+
assertOK(client().performRequest(templateRequest));
363+
364+
final Request indexRequest = new Request("POST", "/" + indexName + "/_doc/1");
365+
indexRequest.setJsonEntity("{\"test\":\"value\"}");
366+
assertOK(client().performRequest(indexRequest));
367+
368+
final String expectedReason = Strings.format(
369+
"index.lifecycle.rollover_alias [%s... (122 chars truncated)",
370+
longMissingAliasName.substring(0, longMissingAliasName.length() - 81)
371+
);
372+
final Map<String, Object> expectedStepInfo = Map.of("type", "illegal_argument_exception", "reason", expectedReason);
373+
assertBusy(() -> {
374+
final Map<String, Object> explainIndex = explainIndex(client(), indexName);
375+
376+
final String assertionMessage = "Assertion failed for the following response: " + explainIndex;
377+
final Object previousStepInfo = explainIndex.get("previous_step_info");
378+
assertThat(assertionMessage, previousStepInfo, equalTo(expectedStepInfo));
379+
}, 30, TimeUnit.SECONDS);
380+
}
381+
324382
private void assertUnmanagedIndex(Map<String, Object> explainIndexMap) {
325383
assertThat(explainIndexMap.get("managed"), is(false));
326384
assertThat(explainIndexMap.get("time_since_index_creation"), is(nullValue()));

‎x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportExplainLifecycleActionTests.java‎

Lines changed: 0 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,13 @@
77

88
package org.elasticsearch.xpack.ilm.action;
99

10-
import org.elasticsearch.ElasticsearchException;
1110
import org.elasticsearch.cluster.metadata.IndexMetadata;
1211
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
1312
import org.elasticsearch.cluster.metadata.ProjectMetadata;
14-
import org.elasticsearch.common.Strings;
1513
import org.elasticsearch.index.IndexVersion;
1614
import org.elasticsearch.test.ESTestCase;
1715
import org.elasticsearch.xcontent.NamedXContentRegistry;
1816
import org.elasticsearch.xcontent.ParseField;
19-
import org.elasticsearch.xcontent.ToXContent;
20-
import org.elasticsearch.xcontent.XContentFactory;
21-
import org.elasticsearch.xcontent.XContentParser;
22-
import org.elasticsearch.xcontent.XContentParserConfiguration;
23-
import org.elasticsearch.xcontent.XContentType;
2417
import org.elasticsearch.xpack.core.ilm.ErrorStep;
2518
import org.elasticsearch.xpack.core.ilm.IndexLifecycleExplainResponse;
2619
import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata;
@@ -38,7 +31,6 @@
3831

3932
import static org.elasticsearch.cluster.metadata.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY;
4033
import static org.elasticsearch.xpack.ilm.action.TransportExplainLifecycleAction.getIndexLifecycleExplainResponse;
41-
import static org.hamcrest.Matchers.equalTo;
4234
import static org.hamcrest.Matchers.is;
4335
import static org.hamcrest.Matchers.notNullValue;
4436
import static org.hamcrest.Matchers.nullValue;
@@ -225,84 +217,6 @@ public void testGetIndexLifecycleExplainResponse_rolloverOnlyIfHasDocuments_adds
225217
assertThat(rolloverAction.getConditions().getMinDocs(), is(1L));
226218
}
227219

228-
public void testPreviousStepInfoTruncationDoesNotBreakExplainJson() throws Exception {
229-
final String policyName = "policy";
230-
final String indexName = "index";
231-
232-
final String longReasonMessage = "a".repeat(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
233-
final String errorJsonWhichWillBeTruncated = Strings.toString((builder, params) -> {
234-
ElasticsearchException.generateThrowableXContent(
235-
builder,
236-
ToXContent.EMPTY_PARAMS,
237-
new IllegalArgumentException(longReasonMessage)
238-
);
239-
return builder;
240-
});
241-
242-
final LifecycleExecutionState stateWithTruncatedStepInfo = LifecycleExecutionState.builder()
243-
.setPhase("hot")
244-
.setAction("rollover")
245-
.setStep("some_step")
246-
.setPhaseDefinition(PHASE_DEFINITION)
247-
.setStepInfo(errorJsonWhichWillBeTruncated)
248-
.build();
249-
250-
// Simulate transition to next step where previous_step_info is copied
251-
final LifecycleExecutionState stateWithPreviousStepInfo = LifecycleExecutionState.builder(stateWithTruncatedStepInfo)
252-
.setPreviousStepInfo(stateWithTruncatedStepInfo.stepInfo())
253-
.setStepInfo(null)
254-
.build();
255-
256-
final IndexMetadata indexMetadata = IndexMetadata.builder(indexName)
257-
.settings(settings(IndexVersion.current()).put(LifecycleSettings.LIFECYCLE_NAME, policyName))
258-
.numberOfShards(1)
259-
.numberOfReplicas(0)
260-
.putCustom(ILM_CUSTOM_METADATA_KEY, stateWithPreviousStepInfo.asMap())
261-
.build();
262-
263-
final ProjectMetadata project = ProjectMetadata.builder(randomProjectIdOrDefault())
264-
.put(indexMetadata, true)
265-
.putCustom(
266-
IndexLifecycleMetadata.TYPE,
267-
new IndexLifecycleMetadata(
268-
Map.of(policyName, LifecyclePolicyMetadataTests.createRandomPolicyMetadata(policyName)),
269-
randomFrom(OperationMode.values())
270-
)
271-
)
272-
.build();
273-
274-
final IndexLifecycleExplainResponse response = TransportExplainLifecycleAction.getIndexLifecycleExplainResponse(
275-
indexName,
276-
project,
277-
false,
278-
true,
279-
REGISTRY,
280-
randomBoolean()
281-
);
282-
283-
final String serialized = Strings.toString(response);
284-
// test we produce valid JSON
285-
try (
286-
XContentParser p = XContentFactory.xContent(XContentType.JSON)
287-
.createParser(XContentParserConfiguration.EMPTY.withRegistry(REGISTRY), serialized)
288-
) {
289-
final IndexLifecycleExplainResponse deserialized = IndexLifecycleExplainResponse.PARSER.apply(p, null);
290-
assertThat(deserialized.toString(), equalTo(response.toString()));
291-
final String actualPreviousStepInfo = deserialized.getPreviousStepInfo().utf8ToString();
292-
293-
final String expectedPreviousStepInfoFormat = """
294-
{"type":"illegal_argument_exception","reason":"%s"}""";
295-
final int jsonBaseLength = Strings.format(expectedPreviousStepInfoFormat, "").length();
296-
final String expectedReason = Strings.format(
297-
"%s... (%d chars truncated)",
298-
"a".repeat(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - jsonBaseLength),
299-
jsonBaseLength
300-
);
301-
final String expectedPreviousStepInfo = Strings.format(expectedPreviousStepInfoFormat, expectedReason);
302-
assertEquals(actualPreviousStepInfo, expectedPreviousStepInfo);
303-
}
304-
}
305-
306220
private static IndexLifecycleMetadata createIndexLifecycleMetadata() {
307221
return new IndexLifecycleMetadata(
308222
Map.of(POLICY_NAME, LifecyclePolicyMetadataTests.createRandomPolicyMetadata(POLICY_NAME)),

0 commit comments

Comments
 (0)