fix: keep numeric/boolean owner and tag strings instead of crashing or dropping them

[deepanshupal]

An owner or tag set to a plain number or boolean, for example owner: 12345 in a dbt config, crashes the monitor. The same value in an alert's owners or tags is silently dropped.

Repro on current master:

from elementary.monitor.fetchers.models.schema import ArtifactSchema
ArtifactSchema(owners="prod").owners   # ['prod']
ArtifactSchema(owners="12345")         # ValidationError: value is not a valid list

from elementary.utils.json_utils import unpack_and_flatten_str_to_list
unpack_and_flatten_str_to_list("2024")  # []

Cause: both helpers run the value through json.loads first. 12345, 1.5 and true are valid JSON, so they come back as an int, float or bool. ExtendedBaseModel._load_var_to_list returns that scalar as is, which fails the List[str] validation. unpack_and_flatten_str_to_list only accepts a list and returns [] for anything else.

Fix: a JSON scalar is kept as the plain string token. Lists, comma separated strings and the existing handling of dicts and {} do not change. Source change is +6/-2 lines.

Tests: new tests/unit/utils/test_json_utils.py. 7 of the 9 cases fail on master and all pass with the change. tests/unit otherwise passes (486). The 4 failures in tests/unit/clients/dbt_runner/test_retry_logic.py also fail on unmodified master (timeouts) and are unrelated.

AI disclosure: this change was written with AI assistance (Claude), then reviewed and tested locally by the submitter.

Summary by CodeRabbit

• Bug Fixes
  • JSON scalar values such as numbers, booleans, and null are now preserved as single-item results instead of being discarded.
  • Inputs that cannot be parsed as JSON are retained as single-item results when converted to lists.
  • Existing handling for lists and comma-separated values remains unchanged.
  • JSON objects continue to follow their existing helper-specific behavior: they are omitted by one conversion helper and serialized into a single-item result by the other.

[github-actions]

👋 @deepanshupal
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 36a8f5e6-0ccb-4ee2-b5a5-889eae740826

📥 Commits

Reviewing files that changed from the base of the PR and between 2c93241 and 7511906.

📒 Files selected for processing (3)

• elementary/utils/json_utils.py
• elementary/utils/schema.py
• tests/unit/utils/test_json_utils.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The JSON unpacking helpers now preserve parsed scalar tokens as single-item string lists. unpack_and_flatten_str_to_list still returns an empty list for JSON objects, while _load_var_to_list retains its dictionary serialization behavior. Unit tests cover these cases and existing list and string inputs.

Changes

JSON list conversion

Layer / File(s)	Summary
Scalar fallbacks and tests elementary/utils/json_utils.py, elementary/utils/schema.py, tests/unit/utils/test_json_utils.py	The helpers preserve parsed scalar tokens as strings in single-item lists. Tests cover numeric and boolean tokens, arrays, comma-separated values, empty objects, and plain strings.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 75119

No actionable merge-blocking issue is identified. The reported retry-test timeout remains an unverified test result, not evidence of a regression in this change.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 75119

The change preserves scalar metadata through existing processing paths without adding permissions or exposed interfaces. Comparison with the previous behavior found no material security risk introduced or worsened by this PR.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated exposure change is retention of metadata within existing artifact and alert processing paths. Even assuming hostile metadata authors, equivalent arbitrary string content was already accepted through ordinary strings or JSON arrays; the new scalar handling does not establish additional identity or credential authority.

Trust Boundaries and Controls

• observed — The Slack consumer performs user-ID lookups only for values matching its email regular expression. Numeric and boolean tokens do not match that gate. Channel selection is obtained from integration parameters, separately from owner/subscriber conversion, and these consumer blocks are unchanged between the canonical base and head.

Resilience and Maintainability Implications

• observed — The changed conversion blocks do not mutate caller-owned lists or introduce a persistent multi-step transition. The general parser and model loader retain existing list pass-through behavior, while the deduplicating helper constructs its output locally.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preserving numeric and boolean owner and tag values as strings instead of crashing or dropping them.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[EladBarkay]

Thanks for the fix and the clear repro! Keeping bare numbers and booleans as strings is the right call. A few things before merging:

1. JSON-encoded strings keep their quotes. json.loads('"@alice"') returns the str @alice, which now goes down the new scalar branch. Both helpers then return the raw text with quotes (['"@alice"']), so it shows up quoted in alerts and doesn't dedup against @alice. The decoded string should be used instead.

2. Numbers inside a JSON list. "12345" now becomes ["12345"], but "[12345]" still becomes [12345], an int. A mixed list like '[1, "a"]' then fails with a TypeError in prettify_and_dedup_list, because it sorts ints and strings together. This predates your PR but is the same bug in list form. One caution: the conversion to strings should happen only in the owners/tags path (unpack_and_flatten_and_dedup_list_of_strings), not in unpack_and_flatten_str_to_list itself. test_rows_sample goes through that function too and relies on it returning a list of dicts unchanged.

3. The two helpers disagree. ExtendedBaseModel._load_var_to_list (report) and unpack_and_flatten_and_dedup_list_of_strings (alerts) parse the same owners and tags, but return different results for the same input:

   • Dicts: '{"a": 1}' becomes [] in alerts (silently dropped) and ['{"a": 1}'] in the report.
   • Comma splitting: "a, b" becomes ["a", "b"] in alerts and ["a, b"] in the report.
   • Whitespace: " prod " becomes ['prod'] in alerts and [' prod '] in the report. This PR adds the same split for scalars (" 12345 ").
   • Dedup and order: alerts dedup through set(), which loses order. The report keeps duplicates and order.

   At a minimum, the cases this PR touches (scalars, quoted strings) should give the same result in both.

Nit: consider merging the two parsers into one. This PR fixes the same bug separately in each helper, and #1 would need fixing twice as well. A single normalizer in json_utils, with _load_var_to_list calling it, would resolve #3 and stop the two from drifting further. Some callers depend on today's behavior, so a few things need deciding along the way:

• _load_var_to_list also parses depends_on_nodes (monitor/fetchers/models/schema.py). Comma splitting, stripping and dedup would start applying there too.
• unpack_and_flatten_str_to_list also parses test_rows_sample (alert_data.py), which expects a list of dicts. The shared logic can't convert list items to strings across the board.
• The model, test and source-freshness schemas don't split on commas today. If the shared version does, tags like "a, b" will start splitting in the report.

Please add tests for each case (quoted strings, lists with numbers, dicts, whitespace) covering both helpers.

[EladBarkay]

A JSON string scalar hits this branch too. '"@alice"' parses to the str @alice, but we return the original text with its quotes. The decoded value should win when it's a string.

[EladBarkay]

The list branch has the same problem as the scalar case for owners and tags: "[12345]" returns an int, and mixed lists fail when prettify_and_dedup_list sorts them. Note that test_rows_sample also comes through here and needs its list of dicts returned as is, so the conversion to strings belongs in unpack_and_flatten_and_dedup_list_of_strings rather than here.

[EladBarkay]

This drops a dict to [], while _load_var_to_list keeps it as [json.dumps(dict)]. That's one of the disagreements in point 3 of the summary.

[EladBarkay]

This is the second copy of the same scalar fix. It has the same quoted-string issue ('"prod"' becomes ['"prod"']) and keeps whitespace where the alerts path strips it. See the nit in the summary about delegating to one shared helper. Also a small nit: is None is redundant, because None already fails isinstance(..., list).

[deepanshupal]

Thanks for the detailed review. Updated the quoted-string and numeric-list paths and added regression coverage:

• JSON string scalars now use the decoded value, including "@alice" and "prod". Both helpers use the same scalar-token normalizer and strip scalar whitespace, including prod, 12345 and true.
• The owners/tags helper converts numeric and boolean list items to strings. [12345], [1, "a"], and an already-decoded mixed list no longer leave numeric values in the alert formatter.
• The general list parser still returns sample-row dicts unchanged. Tests cover that, report/alert owner-list results, and both helpers' existing dict behavior.

I kept comma splitting, dict handling, duplicate retention and ordering at their existing caller-specific boundaries rather than merging the full parsers. That avoids changing depends_on_nodes and report schema behavior in this fix. The shared scalar normalizer removes the duplicated quoted-string/scalar handling; a broader owners/tags normalizer would need a separate decision about those existing policies.

The initial reviewer-case tests produced nine failures before the repair. The updated parser/formatter tests pass (25), and the unit suite excluding tests/unit/clients/dbt_runner/test_retry_logic.py passes (512). The unfiltered run timed out after 120 seconds while in the retry tests, so I am not claiming that full run passed. Formatting and diff checks pass.