fix: Prevent InvalidTypeIdException when Prometheus metric has 'type'…

[nagendramohan]

… label

Add @JsonTypeInfo(use = JsonTypeInfo.Id.NONE) on PrometheusResult to override the parent DataSourceResult interface's polymorphic type handling. This prevents Jackson from interpreting a metric label named 'type' as the polymorphic type discriminator, which caused InvalidTypeIdException during deserialization.

The type dispatch is already handled explicitly via switch statement in ExecuteDirectQueryActionResponse, so polymorphic type annotations are not needed on the concrete class.

Resolves #5684

Description

PromQL queries fail with InvalidTypeIdException when a metric contains a label named type. This happens because the DataSourceResult
interface uses @JsonTypeInfo(property = "type") for polymorphic deserialization, and Jackson interprets the metric's type label as the
type discriminator.

Fix: Add @JsonTypeInfo(use = JsonTypeInfo.Id.NONE) on PrometheusResult to override the parent's polymorphic type handling. The type
dispatch is already handled explicitly via switch statement in ExecuteDirectQueryActionResponse, so annotation-based polymorphism is unnecessary on the concrete class.

Includes a regression test with a metric containing "type": "gauge" label.

Related Issues

Resolves #5684

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request
  created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check
here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 08faca3)

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 switch The switch statement at line 136-142 has a 'case "prometheus"' that assigns to 'result' but then immediately breaks without using it. The variable 'result' is declared at line 134 but never assigned in any case before being used later. This will cause a compilation error ('variable result might not have been initialized'). DataSourceResult result; // Parse based on the determined data source type switch (dataSourceType.toLowerCase()) { case "prometheus": result = OBJECT_MAPPER.readValue(rawResult, PrometheusResult.class); break; // Add cases for other data source types as they're implemented default: throw new IOException("Unsupported data source type: " + dataSourceType);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 08faca3

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for rawResult Add null check for rawResult before parsing to prevent NullPointerException. If rawResult is null, throw an IOException with a descriptive message indicating the data source name and type. direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java [136-142] +if (rawResult == null) { + throw new IOException("Received null result from " + dataSourceName); +} switch (dataSourceType.toLowerCase()) { case "prometheus": result = OBJECT_MAPPER.readValue(rawResult, PrometheusResult.class); break; // Add cases for other data source types as they're implemented default: throw new IOException("Unsupported data source type: " + dataSourceType); Suggestion importance[1-10]: 5 __ Why: Adding a null check for rawResult is a reasonable defensive programming practice that could prevent NullPointerException during JSON parsing. However, the suggestion receives a moderate score because it's unclear from the PR context whether rawResult can actually be null in practice, and the existing code already has error handling in the try-catch block that would catch such exceptions.	Low

---

Previous suggestions

Suggestions up to commit 48e4732

Category	Suggestion	Impact
Possible issue	Missing result storage in map The switch statement is missing a final assignment of result to parsedResults. After the switch block, add parsedResults.put(dataSourceName, result); to ensure the parsed result is actually stored in the map that gets returned. direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java [134-142] DataSourceResult result; // Parse based on the determined data source type switch (dataSourceType.toLowerCase()) { case "prometheus": result = OBJECT_MAPPER.readValue(rawResult, PrometheusResult.class); break; // Add cases for other data source types as they're implemented default: throw new IOException("Unsupported data source type: " + dataSourceType); +} +parsedResults.put(dataSourceName, result); Suggestion importance[1-10]: 10 __ Why: Critical bug: the result variable is parsed but never added to parsedResults map, causing the method to return an empty map. This breaks the core functionality of the method.	High

Suggestions up to commit 2b5e994

Category	Suggestion	Impact
General	Remove redundant type name annotation The @JsonTypeName annotation is redundant when @JsonTypeInfo(use = JsonTypeInfo.Id.NONE) is used. The NONE type info disables polymorphic type handling, making the type name annotation ineffective. Consider removing @JsonTypeName to avoid confusion. direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/datasource/PrometheusResult.java [25-28] -@JsonTypeName("prometheus") @JsonTypeInfo(use = JsonTypeInfo.Id.NONE) @JsonIgnoreProperties(ignoreUnknown = true) public class PrometheusResult implements DataSourceResult { Suggestion importance[1-10]: 3 __ Why: While technically correct that @JsonTypeName is ineffective with JsonTypeInfo.Id.NONE, removing it may break compatibility if the type info strategy changes in the future. The annotation causes no harm and may serve as documentation. This is a minor code cleanup suggestion with minimal impact.	Low

[nagendramohan]

Friendly ping — this fixes a crash when Prometheus metrics contain a field named type (issue #5684). Small change: one annotation + regression test. Happy to address any feedback.

[lezzago]

Code review — findings

The fix (adding @JsonTypeInfo(use = JsonTypeInfo.Id.NONE) to PrometheusResult to stop Jackson from treating a metric's type label as a polymorphic discriminator) is functionally correct for the reported case, and the regression test validates it. A few points worth considering before merge — the first two are the substantive ones.

---

1. Transport round-trip becomes asymmetric across mixed-version nodes (correctness).
writeTo() serializes via OBJECT_MAPPER.writeValueAsString(result). Previously PrometheusResult inherited @JsonTypeInfo(As.PROPERTY, property="type") from DataSourceResult, so serialization emitted a root "type":"prometheus". With Id.NONE, a patched node now emits no root type. But the StreamInput constructor still reads via readValue(resultJson, PrometheusResult.class) — the same polymorphic path that made addTypeFieldToJson(...) necessary in the first place. So during a rolling upgrade, a new node's payload (no root type) fed to an old node fails deserialization (missing type id / InvalidTypeIdException) → RuntimeException: Failed to deserialize Prometheus result.

Scope: one-directional (new→old only; old→new is fine since Id.NONE + ignoreUnknown drop the stray root type), and this is @opensearch.experimental. Bounded, but worth a conscious call.

2. The fix leaves the polymorphism/injection machinery as dead code.
With Id.NONE, the root type injected by addTypeFieldToJson(...) becomes an unknown property that @JsonIgnoreProperties(ignoreUnknown = true) silently drops — so the injection has no effect. Both readValue sites target PrometheusResult.class directly (never the DataSourceResult interface), which means @JsonSubTypes / @JsonTypeName and the interface's @JsonTypeInfo are now vestigial too. A cleaner fix would remove the polymorphism + addTypeFieldToJson scaffolding entirely rather than layer Id.NONE on top of it. (Non-blocking, but it removes a latent trap for the next maintainer.)

3. The contains("\"type\":") guard is a fragile whole-string substring scan (ExecuteDirectQueryActionResponse#parseResult). Any metric label named type flips the branch — it's harmless now only because of point 2, but it no longer expresses meaningful intent. Cleaning up point 2 removes this too.

4. Test coverage gap. The new regression test exercises only the raw-string constructor → parseResult path. It doesn't cover writeTo() → new ExecuteDirectQueryActionResponse(StreamInput), which is exactly the path affected by point 1. A round-trip test with a type label in the metric would give real coverage there.

5. Nit: the test file still ends without a trailing newline (\ No newline at end of file). This is pre-existing on main, not introduced here, but since the file is being touched anyway, ./gradlew spotlessApply would clean it up.

[github-actions]

Persistent review updated to latest commit 48e4732

[nagendramohan]

Thanks for the thorough review @lezzago — the breakdown was really helpful.

I ended up taking your suggestion in #2 and reworking the fix rather than layering Id.NONE on top of the existing machinery. The new commit removes the polymorphism scaffolding entirely:

• Dropped @JsonTypeInfo/@JsonSubTypes from DataSourceResult and @JsonTypeName/@JsonTypeInfo(Id.NONE) from PrometheusResult
• Deleted addTypeFieldToJson() and the contains(""type":") guard, and now read PrometheusResult directly

The reason this is safe: the concrete result type is already carried as the separate dataSourceType string and dispatched via the switch in both
writeTo() and the StreamInput constructor, so the in-JSON type discriminator was never actually exercised through the interface. Removing it is what
resolves #2 and #3 — the fragile substring guard goes away with it.

For #4, I added a writeTo() → StreamInput round-trip test (testStreamSerializationPreservesTypeMetricLabel) that carries a metric label literally named type, so the transport path is now covered directly. And #5 — ran spotlessApply.

That leaves your #1, which I want to be upfront about. This does change the serialized shape (no root type in the result JSON), so there's a bounded,
one-directional mixed-version window during a rolling upgrade: a new node's payload sent to an old node could fail to deserialize (old → new is fine,
since the stray type is just ignored). It only affects the result-JSON shape — dispatch is unaffected because it keys off the separate dataSourceType
string — and the API is @opensearch.experimental.

My read is that's an acceptable tradeoff given the experimental status, but I'd rather not make that call unilaterally. Are you comfortable with it
as-is, or would you prefer I add a compatibility guard (e.g. keep tolerating/emitting the root type for a transition period)? Happy to go either way.

[github-actions]

Persistent review updated to latest commit 08faca3