fix(collector): correct jsonpath alias parsing for rows missing path

[orangeCatDeveloper]

What's changed?

Fixes #3307.
Fixes #3260.

A custom alias field $.status.containerStatuses[0].restartCount (monitor pod restart count, jsonPath parsing) returns an empty or wrong column. Example response, parseScript: $.items.*:

{"items": [
  {"metadata": {"name": "pod-a"}, "status": {"containerStatuses": [{"restartCount": 5}]}},
  {"metadata": {"name": "pod-b"}, "status": {"phase": "Pending"}},
  {"metadata": {"name": "pod-c"}, "status": {"containerStatuses": [{"restartCount": 2}]}}
]}

Two independent bugs. Both hit this user.

Bug 1: rows receive each other's values (HttpCollectImpl)

Problem. The alias column was filled by one global query, then distributed by row index:

query  $.items.*.status.containerStatuses[0].restartCount  →  [5, 2]

pod-b has no containerStatuses, and jayway skips it without a placeholder. Distributing [5, 2] by row index:

row	pod	gets	should be
0	pod-a	5	5
1	pod-b	2 (pod-c's value)	null
2	pod-c	nothing	2

Fix. Ask each row's own object instead of the whole response: new JsonPathParser.parseRowWithJsonPath(rowObject, alias). A row that lacks the path returns an empty list → NULL cell. No cross-row indexing, nothing to misalign.

Bug 2: the calculates step silently eats the value (MetricsCollect)

Problem. calculates: rc=$.status.containerStatuses[0].restartCount must be classified: is the right side a column reference or a formula? The old rule was "if it compiles as JEXL, it's a formula". This path does compile — JEXL reads [0] as array access on a variable named $.status.containerStatuses. That variable doesn't exist, so it evaluates to null with no log. The column stays empty even when Bug 1 is fixed.

Fix. Before compiling, check: right side exactly equals one of the metric's aliasFields? Then it is by definition a column reference → map it directly, skip JEXL. Real formulas keep working — docker's cpu_delta=$.cpu_stats... - $.precpu_stats... doesn't equal any single aliasField.

Test plan

• Unit tests reproduce both bugs (red before the fix, green after): HttpCollectImplTest, MetricsCollectTest, JsonPathParserTest. Full collector suites pass.
• End-to-end on a local instance with a mocked pods API, same monitor, only collector jars swapped: before → rc all null; after → 5 / null / 2.
• Regression: official spring_gateway (profile=$.activeProfiles[0]) and docker (name=$.Names[0]) monitors return identical values before and after.

Checklist

• I have read the Contributing Guide
• I have written the necessary doc or comment.
• I have added the necessary unit tests and all cases have passed.

Add or update API

• I have added the necessary e2e tests and all cases have passed.

[Aias00]

Thanks for the detailed fix and the thorough test plan — both bugs are real and the per-row / alias-reference approach is the right direction. I verified the logic against calculateFields and the tests, and everything checks out except one issue that leaks into the shared parser. Requesting a change before merge.

🔴 Must fix — shared Configuration makes PARSER also suppress exceptions

File: hertzbeat-collector/hertzbeat-collector-common/.../util/JsonPathParser.java

Configuration conf = Configuration.defaultConfiguration()
        .addOptions(Option.DEFAULT_PATH_LEAF_TO_NULL)
        .addOptions(Option.ALWAYS_RETURN_LIST);
PARSER = JsonPath.using(conf);
// a single row legitimately may not contain the queried path
ROW_PARSER = JsonPath.using(conf.addOptions(Option.SUPPRESS_EXCEPTIONS));

In json-path 2.9.0 (confirmed via the repo's pom.xml), Configuration#addOptions(...) mutates the instance in place and returns this. Because both PARSER and ROW_PARSER hold a reference to the same conf, after conf.addOptions(SUPPRESS_EXCEPTIONS) runs, PARSER now also has SUPPRESS_EXCEPTIONS.

Consequences:

• The intended design (only the row-level parser suppresses missing-path exceptions) is defeated.
• parseContentWithJsonPath is used across the codebase (including the new row() helper in JsonPathParserTest). With SUPPRESS_EXCEPTIONS it will silently return null on a missing path instead of throwing PathNotFoundException, which can mask real misconfiguration errors in other collectors and change existing behavior.

It's a silent bug — all tests still pass — so it needs to be fixed explicitly. Suggested fix: give ROW_PARSER its own Configuration instead of mutating the shared one:

ROW_PARSER = JsonPath.using(Configuration.defaultConfiguration()
        .addOptions(Option.DEFAULT_PATH_LEAF_TO_NULL)
        .addOptions(Option.ALWAYS_RETURN_LIST)
        .addOptions(Option.SUPPRESS_EXCEPTIONS));

🟡 Minor / optional

1. aliasFields.contains(expressionStr) is an exact-string match. A calculates alias reference that differs only in whitespace/casing from the aliasField would fall through to JEXL and still silently null. Acceptable given the YAML is generated from the same fields, but a one-line comment documenting the "RHS must exactly equal an aliasField" contract would help future maintainers.
2. In HttpCollectImpl, when an alias returns multiple values (wildcard), resultValue = aliasValues then String.valueOf(...) produces a "[...]" string. Harmless for single-value aliases, but worth a note or an explicit "take first / join" decision.

🟢 Verified correct

• Bug 1 (cross-row misalignment): evaluating parseRowWithJsonPath(objectValue, alias) per row, with a missing path yielding an empty list → NULL cell, eliminates the row-index alignment entirely. HttpCollectImplTest#parseResponseByJsonPathKeepsRowAlignmentWhenAliasPathMissing reproduces the 3-row case (pod-a/5, pod-b-pending/NULL, pod-c/2) well.
• Bug 2 (calculates silently eaten): I traced fieldAliasMap consumption in calculateFields (value = aliasFieldValueMap.get(aliasField)). The new branch routes through the existing alias-mapping path and is mutually exclusive with fieldExpressionMap, so the logic is sound. MetricsCollectTest#calculateFieldsMapsIndexedJsonPathAlias asserts the expected result.
• Test coverage spans common / basic / collector modules and adds two new test classes — good.

Suggested next step

Fix the ROW_PARSER configuration isolation, and (optional but nice) add a one-line regression test asserting that parseContentWithJsonPath still throws / behaves as before on a missing path. Once that's in, this is good to merge.

[orangeCatDeveloper]

Thanks for the careful review! I double-checked the must-fix point against the json-path 2.9.0 sources: Configuration#addOptions copies the option set and returns a new instance via Configuration.builder()...build() — the class is documented as immutable — so PARSER never inherits SUPPRESS_EXCEPTIONS. A runtime check confirms it: conf != conf.addOptions(...), and the original conf still holds only [DEFAULT_PATH_LEAF_TO_NULL, ALWAYS_RETURN_LIST].

To make that contract explicit I added the regression test you suggested — JsonPathParserTest#parseContentWithJsonPathStillThrowsWhenPathMissing — pinning that parseContentWithJsonPath still throws PathNotFoundException on a missing path. Also added short comments for the two minor notes (alias exact-match contract, multi-value wildcard rendering).

[Aias00]

Re-review: withdrawing the "shared Configuration" concern

After the author pushed 407b623 test(collector): pin PARSER missing-path behavior to dispute the High-severity finding, I verified the actual behavior of json-path 2.9.0 empirically rather than relying on memory.

The original concern (now withdrawn)

I claimed that conf.addOptions(Option.SUPPRESS_EXCEPTIONS) mutates the shared conf in place, leaking SUPPRESS_EXCEPTIONS into PARSER. That is incorrect for 2.9.0.

Empirical proof (json-path 2.9.0, exact PR pattern)

Configuration conf = Configuration.defaultConfiguration()
        .addOptions(Option.DEFAULT_PATH_LEAF_TO_NULL)
        .addOptions(Option.ALWAYS_RETURN_LIST);
// conf options BEFORE: [DEFAULT_PATH_LEAF_TO_NULL, ALWAYS_RETURN_LIST]

Configuration conf2 = conf.addOptions(Option.SUPPRESS_EXCEPTIONS);
// conf options AFTER : [DEFAULT_PATH_LEAF_TO_NULL, ALWAYS_RETURN_LIST]   <- unchanged
// conf2 options      : [..., SUPPRESS_EXCEPTIONS]                         <- new instance
// SAME INSTANCE? false

Behavioral check:

• PARSER.read("$.missing") still throws PathNotFoundException → PARSER does not suppress.
• ROW_PARSER.read("$.missing") returns [] → ROW_PARSER correctly suppresses.

So addOptions() in 2.9.0 returns a new immutable Configuration; the design intent (only row-level parsing suppresses exceptions) is preserved. The shared-configuration bug does not exist. I was wrong here — my apologies.

Medium points — addressed

The new commit also:

• documents the alias exact-match contract (aliasFields.contains(expressionStr)),
• documents wildcard/multi-value alias rendering decisions,
• adds JsonPathParserTest (81 LOC) and MetricsCollectTest (73 LOC) that pin this contract and the row-alignment / calculates-skip-JEXL behavior.

Verdict: APPROVED

The original two real fixes (per-row alias evaluation preventing row misalignment, and calculates equal to an aliasField skipping JEXL) remain correct and well-tested, the disputed concern is disproven, and the documentation/tests are a net improvement. Good work.