Skip to content

Commit 8ddeae5

Browse files
szybiaszybia
committed
ILM Explain: valid JSON on truncated step info
1 parent e8a9eb1 commit 8ddeae5

File tree

4 files changed

+134
-20
lines changed

4 files changed

+134
-20
lines changed

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

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,16 @@ public Map<String, String> asMap() {
287287
return Collections.unmodifiableMap(result);
288288
}
289289

290-
public static String truncateWithExplanation(String input) {
291-
if (input != null && input.length() > MAXIMUM_STEP_INFO_STRING_LENGTH) {
292-
return Strings.cleanTruncate(input, MAXIMUM_STEP_INFO_STRING_LENGTH)
293-
+ "... ("
294-
+ (input.length() - MAXIMUM_STEP_INFO_STRING_LENGTH)
295-
+ " chars truncated)";
296-
} else {
297-
return input;
290+
public static String potentiallyTruncateLongJsonWithExplanation(String json) {
291+
if (json == null) {
292+
return null;
298293
}
294+
// Avoid no-op truncations: we must append `"}` (2 chars) to the end to keep JSON valid
295+
if (json.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH + 2) {
296+
return json;
297+
}
298+
final int actuallyRemovedCharacters = json.length() - MAXIMUM_STEP_INFO_STRING_LENGTH - 2;
299+
return Strings.cleanTruncate(json, MAXIMUM_STEP_INFO_STRING_LENGTH) + "... (" + actuallyRemovedCharacters + " chars truncated)\"}";
299300
}
300301

301302
public static class Builder {
@@ -340,7 +341,7 @@ public Builder setFailedStep(String failedStep) {
340341
}
341342

342343
public Builder setStepInfo(String stepInfo) {
343-
this.stepInfo = truncateWithExplanation(stepInfo);
344+
this.stepInfo = potentiallyTruncateLongJsonWithExplanation(stepInfo);
344345
return this;
345346
}
346347

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

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package org.elasticsearch.xpack.core.ilm;
99

1010
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
11+
import org.elasticsearch.common.Strings;
1112
import org.elasticsearch.test.ESTestCase;
1213
import org.elasticsearch.test.EqualsHashCodeTestUtils;
1314

@@ -30,11 +31,11 @@ public void testTruncatingStepInfo() {
3031
assertThat(custom.get("step_info"), equalTo(state.stepInfo()));
3132
String longStepInfo = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 100);
3233
LifecycleExecutionState newState = LifecycleExecutionState.builder(state).setStepInfo(longStepInfo).build();
33-
// Length includes the post suffix
34-
assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 25));
34+
// Length includes the post suffix (`... (X chars truncated)`)
35+
assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 26));
3536
assertThat(
36-
newState.stepInfo().substring(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH, 1049),
37-
equalTo("... (100 chars truncated)")
37+
newState.stepInfo().substring(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH, 1050),
38+
equalTo("... (98 chars truncated)\"}")
3839
);
3940
}
4041

@@ -145,6 +146,38 @@ public void testGetCurrentStepKey() {
145146
assertNull(error6.getMessage());
146147
}
147148

149+
public void testPotentiallyTruncateLongJsonWithExplanationNotTruncated() {
150+
// +2 because with the JSON ending in `"}`, we'll add it back anyway so it's a NOP
151+
final String input = randomAlphaOfLengthBetween(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 2);
152+
assertSame(input, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
153+
}
154+
155+
public void testPotentiallyTruncateLongJsonWithExplanationOneCharTruncated() {
156+
final String jsonBaseFormat = "{\"key\": \"%s\"}";
157+
final int baseLength = Strings.format(jsonBaseFormat, "").length();
158+
final String value = randomAlphanumericOfLength(
159+
// +3 because +1 and +2 are no-ops, they will truncate the end of JSON `"}` and then add it back,
160+
// so we need another char to truncate.
161+
LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 3
162+
);
163+
final String input = Strings.format(jsonBaseFormat, value);
164+
final String expectedOutput = Strings.format(jsonBaseFormat, value.substring(0, value.length() - 1) + "... (1 chars truncated)");
165+
assertEquals(expectedOutput, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
166+
}
167+
168+
public void testPotentiallyTruncateLongJsonWithExplanationTwoCharsTruncated() {
169+
final String jsonBaseFormat = "{\"key\": \"%s\"}";
170+
final int baseLength = Strings.format(jsonBaseFormat, "").length();
171+
final String value = randomAlphanumericOfLength(
172+
// +4 because +1 and +2 are no-ops, they will truncate the end of JSON `"}` and then add it back,
173+
// so we need another two chars to truncate.
174+
LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 4
175+
);
176+
final String input = Strings.format(jsonBaseFormat, value);
177+
final String expectedOutput = Strings.format(jsonBaseFormat, value.substring(0, value.length() - 2) + "... (2 chars truncated)");
178+
assertEquals(expectedOutput, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
179+
}
180+
148181
private LifecycleExecutionState mutate(LifecycleExecutionState toMutate) {
149182
LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate);
150183
switch (randomIntBetween(0, 18)) {

‎x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryItem.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,7 @@ public static ILMHistoryItem failure(
8787
) {
8888
Objects.requireNonNull(error, "ILM failures require an attached exception");
8989
String fullErrorString = exceptionToString(error);
90-
String truncatedErrorString = LifecycleExecutionState.truncateWithExplanation(fullErrorString);
91-
if (truncatedErrorString.equals(fullErrorString) == false) {
92-
// Append a closing quote and closing brace to attempt to make it valid JSON.
93-
// There is no requirement that it actually be valid JSON, so this is
94-
// best-effort, but does not cause problems if it is still invalid.
95-
truncatedErrorString += "\"}";
96-
}
90+
String truncatedErrorString = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(fullErrorString);
9791
return new ILMHistoryItem(index, policyId, timestamp, indexAge, false, executionState, truncatedErrorString);
9892
}
9993

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

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

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

10+
import org.elasticsearch.ElasticsearchException;
1011
import org.elasticsearch.cluster.metadata.IndexMetadata;
1112
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
1213
import org.elasticsearch.cluster.metadata.ProjectMetadata;
14+
import org.elasticsearch.common.Strings;
1315
import org.elasticsearch.index.IndexVersion;
1416
import org.elasticsearch.test.ESTestCase;
1517
import org.elasticsearch.xcontent.NamedXContentRegistry;
1618
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;
1724
import org.elasticsearch.xpack.core.ilm.ErrorStep;
1825
import org.elasticsearch.xpack.core.ilm.IndexLifecycleExplainResponse;
1926
import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata;
@@ -31,6 +38,7 @@
3138

3239
import static org.elasticsearch.cluster.metadata.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY;
3340
import static org.elasticsearch.xpack.ilm.action.TransportExplainLifecycleAction.getIndexLifecycleExplainResponse;
41+
import static org.hamcrest.Matchers.equalTo;
3442
import static org.hamcrest.Matchers.is;
3543
import static org.hamcrest.Matchers.notNullValue;
3644
import static org.hamcrest.Matchers.nullValue;
@@ -217,6 +225,84 @@ public void testGetIndexLifecycleExplainResponse_rolloverOnlyIfHasDocuments_adds
217225
assertThat(rolloverAction.getConditions().getMinDocs(), is(1L));
218226
}
219227

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 jsonCharsBeforeReasonValue = Strings.format(expectedPreviousStepInfoFormat, "").length() - 2; // -2 for `"}`
296+
final String expectedReason = Strings.format(
297+
"%s... (%d chars truncated)",
298+
"a".repeat(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - jsonCharsBeforeReasonValue),
299+
jsonCharsBeforeReasonValue
300+
);
301+
final String expectedPreviousStepInfo = Strings.format(expectedPreviousStepInfoFormat, expectedReason);
302+
assertThat(actualPreviousStepInfo, equalTo(expectedPreviousStepInfo));
303+
}
304+
}
305+
220306
private static IndexLifecycleMetadata createIndexLifecycleMetadata() {
221307
return new IndexLifecycleMetadata(
222308
Map.of(POLICY_NAME, LifecyclePolicyMetadataTests.createRandomPolicyMetadata(POLICY_NAME)),

0 commit comments

Comments
 (0)