Fix ScheduledJob.parse type overload broken by shared-parse refactor

[DarshitChanpura]

Description

PR #981 unified the two ScheduledJob.parse overloads behind a single top-level field walk. However, the two overloads are invoked with the parser at different positions:

• parse(xcp, id, version) — parser is positioned before the outer START_OBJECT.
• parse(xcp, type, id, version) — the sweeper's JobSweeper.isSweepableJobType() has already consumed the outer START_OBJECT and advanced the parser to the wrapper's FIELD_NAME (e.g. "monitor").

The unified walk assumed the first position for both. On the sweeper path it therefore descended into the monitor object, found no wrapper key, and threw — surfacing as Unable to parse ScheduledJob source in alerting logs. Because the job failed to parse, it was never scheduled, and the postIndex/postDelete shard-listener callbacks that drive AlertMover.moveAlerts never fired. This broke a range of alerting integration tests (delete-trigger alert movement, workflow scheduling, doc-level monitor execution, monitor stats scheduling).

Fix

• Restore the type overload to parse the already-located wrapper directly (matching its original, pre-Override DocRequest.type() on alerting request classes; tolerate ancillary top-level fields in ScheduledJob.parse #981 contract).
• Keep the no-type overload order-independent so security-injected top-level fields (e.g. all_shared_principals from the resource-sharing framework's DLS) are still tolerated before or after the wrapper.
• Add XContentTests covering both parser positions and leading/trailing ancillary fields. ScheduledJob.parse previously had no direct test coverage, which is how the regression slipped through.

Related

• Regressed by Override DocRequest.type() on alerting request classes; tolerate ancillary top-level fields in ScheduledJob.parse #981
• Unblocks Onboards Alerting Plugin to Centralized Resource Authz alerting#2180

Check List

• New functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 20a9d58)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 20a9d58
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve backward compatibility for type overload The type overload now assumes the caller has pre-positioned the parser on the wrapper FIELD_NAME, which is a silent breaking change for any existing caller who used the previous behavior (parsing from before the outer START_OBJECT). Consider detecting the current token and handling both cases (walk the outer object if positioned at START_OBJECT or before, otherwise parse directly from the field name) to preserve backward compatibility. src/main/kotlin/org/opensearch/commons/alerting/model/ScheduledJob.kt [78-84] @Throws(IOException::class) fun parse(xcp: XContentParser, type: String, id: String = NO_ID, version: Long = NO_VERSION): ScheduledJob { - // Parser is on the wrapper field name (e.g. "monitor"); advance to its value object. - XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, xcp.currentToken(), xcp) - XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, xcp.nextToken(), xcp) - val job = xcp.namedObject(ScheduledJob::class.java, type, null) - return job.fromDocument(id, version) + if (xcp.currentToken() == XContentParser.Token.FIELD_NAME) { + XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, xcp.nextToken(), xcp) + val job = xcp.namedObject(ScheduledJob::class.java, type, null) + return job.fromDocument(id, version) + } + // Fallback: walk the outer object matching the expected type. + return parse(xcp, id, version) } Suggestion importance[1-10]: 6 __ Why: The type overload's contract has indeed changed to require pre-positioning at the wrapper FIELD_NAME, which is a behavioral change for existing callers. Adding a fallback would preserve backward compatibility, though this may be intentional per the PR design.	Low
General	Guard field-name handling in test helper After xcp.nextToken(); xcp.skipChildren(), the loop calls xcp.nextToken() again which may skip over the following field name or misalign parsing when the value is a scalar (skipChildren on a scalar leaves the parser on the value, so the next token is correctly the next field, but for scalar fields this works differently). Verify token advancement: after skipping a scalar value, calling nextToken() moves to the next field name, which is what the loop expects — but the current loop then re-enters and treats that field name correctly. However, if skipChildren is called on a non-container value, behavior is a no-op; ensure this is intentional and add a safety check. src/test/kotlin/org/opensearch/commons/alerting/model/XContentTests.kt [837-847] var token = xcp.nextToken() while (token != null && token != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME && xcp.currentName() in wrappers) { - return xcp.currentName() - } if (token == XContentParser.Token.FIELD_NAME) { + val name = xcp.currentName() + if (name in wrappers) return name xcp.nextToken() xcp.skipChildren() } token = xcp.nextToken() } Suggestion importance[1-10]: 2 __ Why: The suggestion is a minor refactor for test helper code; the existing logic works correctly and the improvement is marginal.	Low

---

Previous suggestions

Suggestions up to commit 74e5c4c

Category	Suggestion	Impact
General	Validate field name matches expected type The type overload does not validate that the current field name matches the provided type. If the caller mispositions the parser or passes a mismatched type, namedObject may resolve the wrong subtype silently or fail obscurely. Add a check that xcp.currentName() == type before dispatching to namedObject. src/main/kotlin/org/opensearch/commons/alerting/model/ScheduledJob.kt [78-84] @Throws(IOException::class) fun parse(xcp: XContentParser, type: String, id: String = NO_ID, version: Long = NO_VERSION): ScheduledJob { // Parser is on the wrapper field name (e.g. "monitor"); advance to its value object. XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, xcp.currentToken(), xcp) + require(xcp.currentName() == type) { + "Expected wrapper field '$type' but found '${xcp.currentName()}'" + } XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, xcp.nextToken(), xcp) val job = xcp.namedObject(ScheduledJob::class.java, type, null) return job.fromDocument(id, version) } Suggestion importance[1-10]: 5 __ Why: Adding a check that xcp.currentName() == type is a reasonable defensive validation to prevent silent mismatches, but the caller (sweeper) already detects the type from the current field name, making mismatches unlikely in practice.	Low
	Fully consume outer object after parse After parsing the wrapper's inner object via namedObject, the parser is left on the wrapper's END_OBJECT but the outer object's END_OBJECT is not consumed. Callers that expected the previous behavior (fully consumed document) may break. Consider advancing to and validating the outer END_OBJECT for parity with the no-type overload. src/main/kotlin/org/opensearch/commons/alerting/model/ScheduledJob.kt [78-84] @Throws(IOException::class) fun parse(xcp: XContentParser, type: String, id: String = NO_ID, version: Long = NO_VERSION): ScheduledJob { // Parser is on the wrapper field name (e.g. "monitor"); advance to its value object. XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, xcp.currentToken(), xcp) XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, xcp.nextToken(), xcp) val job = xcp.namedObject(ScheduledJob::class.java, type, null) + // Drain any remaining outer fields so the parser ends on the outer END_OBJECT. + var token = xcp.nextToken() + while (token != null && token != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + xcp.nextToken() + xcp.skipChildren() + } + token = xcp.nextToken() + } return job.fromDocument(id, version) } Suggestion importance[1-10]: 4 __ Why: The suggestion notes a real behavioral difference from the previous overload (which consumed the full document), which could matter for callers depending on parser state. However, the sweeper use case likely doesn't require this, and the change adds complexity.	Low

[github-actions]

Persistent review updated to latest commit 20a9d58