Fix JSON escaping in task details log metadata

[zheliu2]

Description

Fixes #8528

The task details JSON logs (opensearch_task_detailslog.json) contain improperly escaped metadata field values. When the metadata contains JSON-like content (e.g., search source), the quotation marks within the value are not escaped, resulting in invalid JSON log events.

Testing

Added unit test to verify proper JSON escaping of metadata values.

Issues Resolved

Fixes #8528

Check List

• New functionality includes testing
• Commits are signed (DCO)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ecefdcf)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Test Coverage The new test testTaskDetailsLogWithJsonMetadata verifies that the metadata value is stored correctly in the message object, but does not verify that the final JSON log output is properly escaped. The fix is in OpenSearchJsonLayout where %enc{...}{JSON} is applied during log rendering, but the test only checks the raw value via getValueFor("metadata"). A test that validates the actual rendered JSON output (e.g., that quotes within the metadata are escaped as \") would better confirm the fix works end-to-end. public void testTaskDetailsLogWithJsonMetadata() { String jsonMetadata = "{\"query\":{\"match_all\":{}},\"size\":10}"; SearchShardTask task = new SearchShardTask( 1, "transport", "indices:data/read/search[phase/query]", "test", null, Collections.singletonMap(Task.X_OPAQUE_ID, "my_id"), () -> jsonMetadata ); SearchShardTaskDetailsLogMessage p = new SearchShardTaskDetailsLogMessage(task); // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); } Key Override Behavior When a field key in opensearchMessageFields matches an existing key in map (e.g., "message"), the new value wraps it with %enc{%OpenSearchMessageField{message}}{JSON} instead of the original message pattern. This changes the rendering behavior for overridden fields. Verify that this is the intended behavior and that the testLayoutWithAdditionalFieldOverride test accurately reflects the expected output when a standard field like message is overridden. for (String key : opensearchMessageFields) { map.put(key, inQuotes("%enc{%OpenSearchMessageField{" + key + "}}{JSON}")); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ecefdcf

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Test should verify JSON escaping behavior The test only verifies that the raw metadata value is stored correctly, but it doesn't verify that the JSON escaping actually works when the metadata is rendered in a log message. Since the PR's purpose is to fix JSON escaping, the test should also verify that the output is properly escaped when embedded in a JSON log context (e.g., special characters like " are escaped as "). server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [75-76] // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); +// Verify that the metadata value contains characters that require JSON escaping +assertThat(p.getValueFor("metadata"), containsString("\"")); Suggestion importance[1-10]: 3 __ Why: The suggestion adds a redundant assertion - checking that jsonMetadata contains " is already implied by the equalTo(jsonMetadata) check since jsonMetadata is defined with quotes. The suggestion doesn't actually test JSON escaping in a log rendering context, making it a marginal improvement at best.	Low

---

Previous suggestions

Suggestions up to commit 108b278

Category	Suggestion	Impact
General	Add assertion for JSON escaping behavior The test verifies that raw JSON metadata is stored correctly, but the PR's purpose is to fix JSON escaping in log metadata. The test should also verify that when the metadata is rendered in a JSON log context, the content is properly escaped (e.g., special characters like " are escaped as "). Consider adding an assertion that checks the escaped output to validate the actual fix. server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [75-76] // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); +// Verify that JSON metadata is properly escaped when rendered in JSON log output +String expectedEscaped = "{\\\"query\\\":{\\\"match_all\\\":{}},\\\"size\\\":10}"; +assertThat(p.getValueFor("metadata").replace("\"", "\\\""), equalTo(expectedEscaped)); Suggestion importance[1-10]: 2 __ Why: The suggestion's improved_code is logically flawed - it manually applies escaping to the raw string and then checks if it equals an escaped version, which is a tautological test that doesn't validate actual framework behavior. The PR's fix is about using %enc{...}{JSON} in the log pattern (in OpenSearchJsonLayout.java), not about escaping within getValueFor("metadata"), so the test as written correctly validates that the raw metadata is stored correctly.	Low

Suggestions up to commit 807e7ff

Category	Suggestion	Impact
General	Test should verify JSON escaping behavior The test verifies that raw JSON metadata is stored correctly, but the PR's purpose is to fix JSON escaping in log output. The test should also verify that when the log message is rendered through the JSON layout, the metadata is properly escaped (e.g., inner quotes are escaped as "). Without testing the escaped output, the test doesn't validate the actual fix. server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [75-76] // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); +// Verify that the metadata value would be properly JSON-escaped when rendered +String expectedEscaped = "{\\\"query\\\":{\\\"match_all\\\":{}},\\\"size\\\":10}"; +assertThat(p.getValueFor("metadata").replace("\"", "\\\""), equalTo(expectedEscaped)); Suggestion importance[1-10]: 2 __ Why: The suggestion's improved_code is logically flawed - it manually applies escaping to the raw value and then compares it to a manually escaped string, which doesn't actually test the JSON layout rendering. The existing test correctly verifies that getValueFor("metadata") returns the raw JSON string, which is the appropriate unit test scope. Testing the full rendering pipeline would require a different test setup entirely.	Low

Suggestions up to commit e4efd0f

Category	Suggestion	Impact
General	Add edge case tests for special characters The test should also cover edge cases for JSON escaping, such as metadata containing special characters like backslashes, newlines, or quotes that could break JSON formatting. Adding a test case with such characters would better validate the fix introduced in this PR. server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [64-72] SearchShardTask task = new SearchShardTask( 1, "transport", "indices:data/read/search[phase/query]", "test", null, Collections.singletonMap(Task.X_OPAQUE_ID, "my_id"), () -> jsonMetadata ); +// Also test with special characters that require JSON escaping +String specialCharsMetadata = "{\"query\":\"test\\\"quoted\\\"\",\"newline\":\"line1\\nline2\"}"; +SearchShardTask taskWithSpecialChars = new SearchShardTask( + 2, + "transport", + "indices:data/read/search[phase/query]", + "test", + null, + Collections.singletonMap(Task.X_OPAQUE_ID, "my_id"), + () -> specialCharsMetadata +); +SearchShardTaskDetailsLogMessage p2 = new SearchShardTaskDetailsLogMessage(taskWithSpecialChars); +assertThat(p2.getValueFor("metadata"), equalTo(specialCharsMetadata)); Suggestion importance[1-10]: 3 __ Why: While adding edge case tests for special characters is a valid suggestion in principle, the improved_code only asserts that getValueFor("metadata") returns the raw metadata unchanged, which doesn't actually test JSON encoding behavior. The suggestion doesn't meaningfully validate the fix introduced in this PR.	Low
	Test verifies JSON encoding behavior The test only verifies that the raw metadata is stored correctly, but doesn't verify that the JSON encoding (the actual fix in this PR) works properly when the metadata is rendered in the log output. Consider adding an assertion that checks the encoded output to ensure special JSON characters are properly escaped when rendered through the layout. server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [75-76] // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); +// Verify that JSON special characters in metadata are properly escaped in log output +String encodedMetadata = p.getValueFor("metadata").replace("\"", "\\\""); +assertThat(encodedMetadata, not(containsString("\"query\":"))); Suggestion importance[1-10]: 2 __ Why: The suggestion's improved_code is logically flawed - it manually replaces quotes and then asserts the encoded string doesn't contain "query":, which would always pass regardless of the fix. The test as written correctly validates that the raw metadata value is stored properly, and the JSON encoding is handled by the layout pattern (tested in OpenSearchJsonLayoutTests).	Low

Suggestions up to commit 78306c1

Category	Suggestion	Impact
General	Test should verify JSON encoding behavior The test only verifies that the raw metadata is stored correctly, but doesn't verify that the JSON encoding (the actual fix) works properly when the metadata is rendered in a log message. Consider adding an assertion that checks the encoded output to ensure special JSON characters are properly escaped when the field is rendered. server/src/test/java/org/opensearch/tasks/consumer/SearchShardTaskDetailsLogMessageTests.java [75-76] // Verify that metadata with JSON content is stored correctly assertThat(p.getValueFor("metadata"), equalTo(jsonMetadata)); +// Verify that the metadata field is properly JSON-encoded in the log pattern +// (i.e., inner quotes are escaped so the outer JSON structure remains valid) +String rendered = p.getValueFor("metadata").toString(); +assertThat(rendered, not(containsString("\"query\""))); // raw unescaped quotes should not appear if encoded Suggestion importance[1-10]: 2 __ Why: The improved_code is logically contradictory - it first asserts equalTo(jsonMetadata) (raw JSON with unescaped quotes), then asserts that unescaped quotes should NOT appear, which would fail. The suggestion also conflates storage-level testing with rendering/encoding concerns that are outside the scope of this test method.	Low

[github-actions]

Persistent review updated to latest commit e4efd0f

[github-actions]

✅ Gradle check result for e4efd0f: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.31%. Comparing base (22a8d9d) to head (ecefdcf).
⚠️ Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
...pensearch/common/logging/OpenSearchJsonLayout.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20802      +/-   ##
============================================
- Coverage     73.33%   73.31%   -0.03%     
- Complexity    72306    72315       +9     
============================================
  Files          5796     5796              
  Lines        330263   330263              
  Branches      47663    47663              
============================================
- Hits         242189   242122      -67     
- Misses        68660    68761     +101     
+ Partials      19414    19380      -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gaobinlong]

Thanks, I think this PR needs a change log.

[github-actions]

Persistent review updated to latest commit 807e7ff

[github-actions]

✅ Gradle check result for a5bbbfd: SUCCESS

[github-actions]

Persistent review updated to latest commit 108b278

[github-actions]

❌ Gradle check result for 108b278: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit ecefdcf

[github-actions]

❌ Gradle check result for ecefdcf: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for ecefdcf: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for ecefdcf: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.