Skip to content

fix: exclude wiped dataclips from work order body search - #4889

Open
mvanhorn wants to merge 3 commits into
OpenFn:mainfrom
mvanhorn:fix/4824-clear-search-vector-on-wipe
Open

fix: exclude wiped dataclips from work order body search#4889
mvanhorn wants to merge 3 commits into
OpenFn:mainfrom
mvanhorn:fix/4824-clear-search-vector-on-wipe

Conversation

@mvanhorn

Copy link
Copy Markdown

Description

This PR fixes a data-retention hole where wiped dataclips remained searchable by their erased content.

When a dataclip is wiped (per-run wipe with save_dataclips: false, or the project data-retention job), its body/request are cleared to NULL and wiped_at is stamped, but its full-text search_vector column is left intact (the indexing trigger/worker only runs on insert, never on the wipe UPDATE). The work order body search matches that stale vector with no wiped_at guard, so a wiped dataclip stays searchable by the exact content that was meant to be erased.

The fix adds an is_nil(input_dataclip.wiped_at) guard to the :body branch of Invocation.build_search_fields_where/2, ANDed onto the full-text match. This targets the :input_dataclip binding that body search already uses, so erased dataclip content is no longer discoverable regardless of any stale search_vector, and other search fields (id / log / dataclip_name / status) are unaffected.

This mirrors the read-side guard already present in search_workorders_for_retry/2 via exclude_wiped_dataclips/1, but applied at the precise binding the body full-text predicate matches rather than the work order's own dataclip.

Closes #4824

Validation steps

  1. Create a work order whose input dataclip body contains a distinctive token, and confirm body search (search_fields: ["body"]) returns it.
  2. Wipe the dataclip (Lightning.Runs.wipe_dataclips/1, or the project data-retention job).
  3. Search again by the same token with the body filter and confirm it no longer appears, even though the dataclip's search_vector is still populated.
  4. Confirm a separate, non-wiped dataclip carrying the same token still matches (no over-filtering).

The two new tests in test/lightning/invocation_test.exs (describe "search_workorders/3") cover the regression and the non-wiped match case, including a positive control proving the body vector is populated before the negative assertion.

Additional notes for the reviewer

  1. The guard is read-side only and additive to the body predicate; it does not change the wipe path, the search_vector workers, or any migration.
  2. An earlier approach that also nulled search_vector in Query.wipe_dataclips/1 was dropped: search_vector is not a declared schema field (it is managed by raw-SQL workers), so a schema update_all referencing it would not be valid.

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our
Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review
    with Claude Code)
  • I have implemented and tested all related authorization policies.
    (e.g., :owner, :admin, :editor, :viewer) — n/a, this is a
    read-side search filter with no authorization surface.
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

When a dataclip is wiped (per-run wipe or the project data-retention job),
its body/request are cleared and wiped_at is stamped, but its full-text
search_vector is left intact, so wiped dataclips stayed searchable by the
exact erased content via work order body search.

Guard the body full-text match on is_nil(input_dataclip.wiped_at) so erased
dataclip content is no longer discoverable, regardless of any stale
search_vector. The guard targets the input_dataclip binding the body search
already uses, so it does not affect other search fields.

Fixes OpenFn#4824
The raw Repo.query! binding passed the dataclip id as a 36-char string to a
$1::uuid parameter, which Postgrex rejects (expects a 16-byte binary). Wrap it
in Ecto.UUID.dump!/1, matching the existing pattern in runs_test.exs.
@mvanhorn

Copy link
Copy Markdown
Author

Fixed the test_elixir failure: my new test passed the dataclip id as a string to a $1::uuid raw query parameter, which Postgrex rejects (it wants a 16-byte binary). Wrapped it in Ecto.UUID.dump!/1, matching the existing pattern in runs_test.exs. Verified locally against Elixir 1.18.3-otp-27 + Postgres - the test now passes.

Heads-up on the lint job: it's failing on deps.audit and hex.audit (format, credo, and sobelow all pass). Those are dependency-level audit findings unrelated to this PR, which only touches invocation.ex and its test, so they're likely pre-existing on main. Let me know if you'd like me to look at the flagged dependencies separately.

@mvanhorn

Copy link
Copy Markdown
Author

Heads up on the red lint check here: it's failing on mix deps.audit and mix hex.audit, not on anything in this PR. Both are flagging earmark 1.4.48 (GHSA-52mm-h59v-f3c7 stored-XSS advisory, plus the package now being retired). This PR only touches lib/lightning/invocation.ex and its test, and format and credo both pass. The earmark issue is repo-wide and currently fails on any branch. Happy to open a separate PR adding GHSA-52mm-h59v-f3c7 to the deps.audit --ignore-advisory-ids list (alongside the ones already there) if that's helpful, otherwise feel free to handle it however you prefer.

@midigofrank midigofrank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @mvanhorn , thanks for picking this up. Nicely done

Only CHANGELOG.md conflicted, and it was two sets of additions to the same
Fixed list rather than a real disagreement, so both are kept. The invocation
change and its test merged cleanly.

Verified on the pinned toolchain from .tool-versions (Elixir 1.18.3-otp-27):
test/lightning/invocation_test.exs is 73 tests, 0 failures. Confirmed the new
test is falsifiable by reverting the is_nil(dataclip.wiped_at) guard, which
fails it, and restoring it, which passes.
@mvanhorn

mvanhorn commented Aug 1, 2026

Copy link
Copy Markdown
Author

Merged main, the branch is mergeable again.

Only CHANGELOG.md conflicted, and it was two sets of additions to the same Fixed list rather than a real disagreement, so both are kept. lib/lightning/invocation.ex and its test merged cleanly, so the change itself is untouched.

Verified on the toolchain the repo pins in .tool-versions, Elixir 1.18.3-otp-27:

mix test test/lightning/invocation_test.exs
73 tests, 0 failures, 3 skipped

I also checked the new test actually proves something rather than passing by accident. Reverting just the is_nil(dataclip.wiped_at) guard fails it, and restoring the guard passes it:

guard removed   73 tests, 1 failure, 72 excluded
guard restored  73 tests, 0 failures, 72 excluded

The earlier note about the red lint check still applies as far as I can tell: it was failing on mix deps.audit and mix hex.audit flagging earmark 1.4.48, which this PR does not touch.

@mvanhorn

mvanhorn commented Aug 1, 2026

Copy link
Copy Markdown
Author

Correcting myself on the lint check: it is green now, on this merge commit.

The reason is the merge itself. Main dropped the earmark dependency, so mix deps.audit and mix hex.audit no longer have the retired 1.4.48 package to flag. Only earmark_parser remains in mix.lock.

So all five CircleCI checks pass on e538afc: compile, lint, check_dialyzer, test_elixir and test_javascript. Disregard the last paragraph of my previous comment.

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

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

Wiped dataclips remain searchable — search_vector not cleared on wipe

2 participants