chore: clear the ExSlop DualKeyAccess backlog check

[chasers]

Fixes all 15 ExSlop.Check.Warning.DualKeyAccess findings, then enables the check by removing it from ex_slop_backlog in config/.credo.exs. The backlog drops from 10 checks to 9.

The check fires on || chains that read one map with both an atom key and a string key. Each site now reads one key type.

Backend config keeps one shape

config_encrypted round-trips through JSON, so it holds string keys after a raw Repo.get and atom keys after typecast_config_string_map_to_atom_map/1. Both shapes reached Adaptor.cast_config/2, so the webhook adaptor read each field twice.

Backend.validate_config/1 now atomizes the existing config once. This matches the contract redact_config/1 already had, documented on the Jason.Encoder impl. The atomize helper moves out of that impl so both callers share it. The webhook, sentry and postgres adaptors drop their string branches.

Attrs normalize at the context boundary

Alerting.create_alert_query/2, Alerting.update_alert_query/2, Backends.update_backend/2 and KeyValues.bulk_upsert_key_values/2 take attrs from both API params and internal callers. They stringify the top level keys once through the new Utils.Map.stringify_top_level_keys/1, which leaves values untouched. A deep stringify would corrupt struct and payload values. In update_backend/2 this also collapses the dual Map.has_key?/2 reads.

Dead branches

LogEvent.mapper/3 reads string keys and returns a string-keyed body, so the atom branch in id/1 never ran. The same holds for the params in log_event_factory/1, which stringifies them first.

Verification

MIX_ENV=test mix ci passes. Credo now runs 75 checks, up from 74.

Test files run: backends, alerting, key_values, log_event, logs, utils, the webhook, sentry and postgres adaptors, the backend, rule and key value controllers, and the alerts and rules LiveViews. All pass.

Note for reviewers: mix ci fails under MIX_ENV=dev, because the reach baseline was written for the test scope and dev adds priv/tasks. Run MIX_ENV=test mix ci locally, as CI does. That is pre-existing and out of scope here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YQ8Y8zCvSKUFNPvkwH2WS4

[chasers]

Pushed fa51356de after a review pass. It fixes two regressions in the first commit.

1. Backends.update_backend/2 deleted alert query associations from API calls. The stringify made Map.has_key?(attrs, "alert_queries") true for a raw request body. Api.BackendController.update/2 passes body params straight through, and the association is on_replace: :delete, so PATCH /api/backends/:token with {"alert_queries": []} deleted every join row for that backend. The function now reads the atom key from the original attrs, which keeps the old behaviour. test/logflare/backends_test.exs:600 covers it, and the test fails without the fix.

2. Backend.validate_config/1 raised on the write path. The shared atomize helper ran on every changeset, so a stored config key with no live atom raised ArgumentError instead of returning a changeset error. It now normalizes the stored config through the adaptor's own cast_config/1, the same way Backends.typecast_config_string_map_to_atom_map/1 already does. No atom creation, and keys no adaptor declares drop out. The Jason.Encoder helper goes back to private, so the public function the first commit added is gone.

Verified with MIX_ENV=test mix ci and the backends, alerting, key_values, utils, backend controller and adaptor suites. The ClickHouse and syslog suites need containers, so CI covers those.

[chasers]

@claude please review this PR.

[claude]

Claude finished @chasers's task in 5m 5s —— View job

---

Review

Went through both commits (8e4828a clears the DualKeyAccess findings, fa51356 fixes two regressions found in review) and traced the invariants each change relies on. Overall this is solid — the fix commit addresses real bugs and does so the right way, in line with existing codebase patterns.

fa51356 (the fix commit) looks correct:

• Backends.update_backend/2 (lib/logflare/backends.ex:289-294) now captures alerts_modified/alert_queries from the original attrs (atom key only) before stringifying, restoring the pre-PR semantics where alert_queries can only be mutated via an atom-keyed internal call, never via a raw string-keyed API body. I confirmed the original (pre-PR) code checked Map.has_key?(attrs, :alert_queries) with no string fallback, unlike default_ingest?/config/source_id which did have dual fallback — so this asymmetry is intentional and the fix correctly preserves it. The new regression test at test/logflare/backends_test.exs:600 exercises exactly the PATCH .../:token scenario described in the PR body.
• Backend.validate_config/1 (lib/logflare/backends/backend.ex:87,108-114) now normalizes the stored config via mod.cast_config/1 + apply_changes/1 instead of blind String.to_existing_atom/1. This matches the established pattern in Backends.typecast_config_string_map_to_atom_map/1 (lib/logflare/backends.ex:483-496) exactly. I checked all 16 adaptors in lib/logflare/backends/adaptor/adaptor_mapping() — every one defines cast_config(params, existing_config \\ %{}), so the 1-arg call is always valid, and unknown/stale keys are silently dropped by Ecto.Changeset.cast instead of raising.

First commit's core simplifications check out:

• config_encrypted is atom-keyed by the time it reaches redact_config/1, webhook_adaptor.ex's unredact_headers/2/unredact_url/2, sentry_adaptor.ex's redact_config/1, and pg_repo.ex's schema lookup — verified every call path into these (Backend.validate_config/1, typecast_config_string_map_to_atom_map/1, the Jason.Encoder impl) normalizes first, so dropping the string-key fallback is safe.
• LogEvent.id/1's dropped atom branch is genuinely dead: both callers of mapper/3 (make/2, make_from_db/2) only ever pass string-keyed params (per mapper/3's own @spec).
• Logflare.Factory.rule_factory/1's dropped string-key fallback for lql_string is safe — every insert(:rule, ...)/build(:rule, ...) call site in the test suite uses the atom keyword form, never a string-keyed map.
• Utils.Map.stringify_top_level_keys/1 is a clean, minimal addition and its usages in alerting.ex/key_values.ex preserve prior behavior (the original code already did dual-key fallback for backends/key/value, so collapsing to a single string-keyed read after normalization is behavior-preserving).

Minor observations (not blockers):

• stringify_top_level_keys/1 is a generically useful helper but doesn't guard against a map that (pathologically) has both :k and "k" present simultaneously — Map.new/2's enumeration order would decide which value wins. Not a real-world concern given how attrs are actually constructed in every caller here, just worth knowing if this helper gets reused elsewhere later.
• The doc comment on Adaptor.cast_config/2 (lib/logflare/backends/adaptor.ex:222) — "Backend normalizes the stored config through cast_config/1" — is accurate post-fix; nothing to change, just noting it was correctly updated between the two commits.

Verification note: I was not able to run mix test, mix credo, or mix ci in this environment — Bash tool calls for mix/gh commands required interactive approval that wasn't available here, so I relied on static reading of the diff plus tracing call sites/invariants by hand. The PR description states MIX_ENV=test mix ci passes and the regression test was verified to fail without the fix — I have no reason to doubt that, but I'd suggest a maintainer confirm CI is green before merging if it hasn't already run.

No blocking issues found.

Branch ·