Skip to content

fix: make multi-field CIM validation deterministic (ADDON-89948) - #964

Open
mkolasinski-splunk wants to merge 5 commits into
mainfrom
codex/ADDON-89948-multifield-query
Open

fix: make multi-field CIM validation deterministic (ADDON-89948)#964
mkolasinski-splunk wants to merge 5 commits into
mainfrom
codex/ADDON-89948-multifield-query

Conversation

@mkolasinski-splunk

Copy link
Copy Markdown
Contributor

Summary

  • evaluate each CIM field's validity directly inside the stats aggregation
  • avoid sequential calculated validity fields in multi-field searches
  • preserve accurate invalid-value reporting for every field
  • add regression coverage for multi-field counts and invalid values

Root cause

On distributed Splunk Cloud searches, sequential eval commands used to construct multiple *_valid and *_invalid fields could produce inconsistent per-field results before aggregation. Valid values were consequently counted as invalid and could be displayed in the failure report as incorrect invalid values.

The aggregation now evaluates each field independently with count(eval(...)) and values(eval(...)), avoiding those intermediate calculated fields.

Validation

  • PSA unit suite: 529 passed
  • Black and diff checks: passed
  • retained Cloud control with PSA main and released TA packet semantics: 8/13 and 5/13 valid
  • retained Cloud with this fix and the same released semantics: 13/13 valid for both fields on both event sources
  • injected invalid-value probe: bad-in and bad-out were reported only for their respective fields on both Cloud event sources
  • exact TA 5.1.1 zone and packet clusters with this PSA fix: 2/2 passed on retained Cloud
  • exact TA 5.1.1 zone and packet clusters with this PSA fix: 2/2 passed on local Splunk Enterprise

Ticket

ADDON-89948

@mkolasinski-splunk
mkolasinski-splunk requested a review from a team as a code owner August 25, 2026 05:21

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07164cbf4b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +89 to +91
"NOT ({validity}) IN ({values})".format(
validity=self.validity,
values=self.get_eval_query_from_values(self.negative_values),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve case-insensitive negative-value matching

When a field contains a differently cased sentinel such as UNKNOWN or Null, this eval-time IN comparison does not match the configured defaults (unknown and null) because Splunk eval comparisons are case-sensitive. The previous searchmatch implementation used case-insensitive search matching, so these values were rejected; they now pass through the if expression and are counted as valid. Preserve the former matching semantics, for example by normalizing both operands before comparison.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in 76f67ff, which landed after the 07164cb revision this review was based on. get_validity_expression() normalizes the left operand with lower(tostring(<validity>)) and get_eval_query_from_values() lowercases each configured literal, so both sides of the eval IN are lowercase and UNKNOWN/Null still match the configured unknown/null negative values exactly as searchmatch did. Covered by test_get_validity_expression and test_get_validity_expression_compares_configured_values_case_insensitively. No shipped data model uses * inside a value other than the bare * wildcard, which is still short-circuited, so no other searchmatch matching behaviour is lost.

Comment on lines +70 to +71
if self.validity_query is None:
self.validity_query = "\n" f"| nomv {self.name}" if self.multi_value else ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve multivalue fields until dependent validity checks run

When an earlier field's validity expression depends on a later multivalue field, such as count == mvcount(recipients), this retained nomv is now emitted before any of the validity expressions are evaluated because evaluation was deferred into stats. The dependent expression therefore sees the flattened field (normally an mvcount of 1) and marks otherwise valid events invalid; previously the earlier field's eval ran before the later field's nomv. Avoid destructively flattening all multivalue fields before cross-field validity expressions are evaluated.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is real, but it is pre-existing rather than introduced here, and the suggested fix is not safe on its own.

Why no shipped data model changes behaviour:

  • Each test_cim_required_fields parameter carries either a single field or one fields_cluster (cim_tests/test_generator.py:120-144), so a validity expression can only observe another field's nomv when both fields are in the same cluster.
  • The only shipped cluster containing a multivalue field plus a field whose validity references it is Email / "All Email" [recipient, recipient_count]. Cluster order comes from the dataset fields list (cim_tests/data_set.py:78-92), where recipient (index 16) precedes recipient_count (index 17), so | nomv recipient already ran before the old | eval recipient_count_valid=if(... recipient_count==mvcount(recipient) ...). Identical before and after this PR.
  • Network_Resolution / "DNS" declares no clusters, so answer_count and query_count are only ever tested as single-field searches where no | nomv answer / | nomv query is emitted at all — mvcount() is evaluated against the real multivalue field, before and after.

Why the flatten cannot simply move into the stats eval: | nomv is load-bearing for field_count, not only for validity. count(<field>) counts every value of a multivalue field, and test_cim_required_fields fails with "Field X should not be multi-value" when field_count > event_count (cim_tests/test_templates.py:130). Dropping it would require field_count, the validity count and the invalid-value list to each flatten per field, and for the one shipped multivalue field with a custom validity (Change / object_attrs, if(like(object_attrs,'%"%'),null(),object_attrs)) that means textually rewriting user-supplied SPL. That changes behaviour for every multivalue field in every TA and cannot be validated without a live Splunk, so it does not belong in this fix.

I will document the constraint on gen_validity_query() and track the underlying limitation — mvcount()-based validity is unreliable whenever the field is flattened in the same search — as a separate follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant