Skip to content

fix: require an exact-match key field for update/delete document#2

Merged
federico-martinez-invgate merged 1 commit into
mainfrom
fix/reject-tokenized-key-in-update-delete
Jul 9, 2026
Merged

fix: require an exact-match key field for update/delete document#2
federico-martinez-invgate merged 1 commit into
mainfrom
fix/reject-tokenized-key-in-update-delete

Conversation

@federico-martinez-invgate

Copy link
Copy Markdown
Contributor

Problem

updateDocument / deleteDocument accepted any field as key_field and ran
delete_term(Term::from_field_text(field, value)) with no check that the field is a non-tokenized
exact key. Passing a tokenized text field (or a STORED-only attribute) silently corrupted the
index
:

  • update → duplication: a multi-word value is never stored as a single term, so delete_term
    matches nothing, the delete is a no-op, and the add creates a duplicate document for the key.
  • delete → mass deletion: a value that is an indexed token deletes every document containing
    that token, not one.

Fix

  • field_is_exact_key() in schema.rs: a field is a valid key iff it's a STRING str field —
    indexed with the "raw" tokenizer (STRING uses "raw", TEXT uses "default", attributes have no
    indexing options). This rejects both tokenized text fields and non-indexed attribute fields.
  • resolve_key_field() guard in writer.rs, called by both delete_by_id and update_document,
    which returns a clear error before any writer mutation. The existing "field does not exist"
    path is preserved. Plain TantivyException in PHP (no new exception type) — this is a programmer
    error, not a runtime-recoverable condition.

Tests

Two new tests reproduce both corruption modes against the old behavior (update → 2 copies;
delete → 0 docs) and now assert the operations are rejected without corrupting the index. Full suite:
20/20 pass (17 core + 3 ext), clippy -D warnings and fmt --check clean.

Real-product validation

Audited service-desk-main before merge: the only updateDocument/deleteDocument call-sites (the
write feed) derive key_field from tantivyIdField() = id_key / doc_key, both in the keys
bucket for all three indexes (Incidents / KnowledgeBase / CatalogCategories). Ran the real engine:
update-by-key leaves no duplicate, delete-by-key removes only the target (including composite
doc_key), and the negative cases (tokenized/attribute key) throw the expected exception. No
call-sites need changes.

🤖 Generated with Claude Code

updateDocument/deleteDocument accepted ANY field as key_field and ran
delete_term(Term::from_field_text(field, value)) with no check that the field
is a non-tokenized exact key. Passing a tokenized `text` field (or a
STORED-only `attribute`) silently corrupted the index: the whole value is
never stored as a single term, so delete_term either matched nothing
(update -> the delete is a no-op and the add creates a DUPLICATE document for
the key) or matched a shared token (delete -> MASS DELETION of every document
containing that token).

Add field_is_exact_key() in schema.rs -- a field is a valid key iff it is a
STRING str field, i.e. indexed with the "raw" tokenizer (STRING uses "raw",
TEXT uses "default", attributes have no indexing options) -- and a
resolve_key_field() guard used by both delete_by_id and update_document that
rejects tokenized/attribute fields with a clear, actionable error BEFORE any
writer mutation. The pre-existing "field does not exist" path is preserved.

Two tests reproduce both corruption modes (update duplication, delete
mass-deletion) against the old behavior and now assert the operations are
rejected without corrupting the index. Validated end-to-end against the real
Service Desk indexes (Incidents / KnowledgeBase / CatalogCategories), whose
update/delete paths only ever use id_key / doc_key (both in the `keys`
bucket), so the fail-loud behavior never triggers for correct usage.
@federico-martinez-invgate federico-martinez-invgate merged commit 0deaa97 into main Jul 9, 2026
12 checks passed
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