From 9de0c94a0e29efc5a6b97e992ac4191215c80def Mon Sep 17 00:00:00 2001 From: Federico Martinez Date: Thu, 9 Jul 2026 20:47:02 -0300 Subject: [PATCH] fix: require an exact-match key field for update/delete document 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. --- crates/tantivy-core/src/schema.rs | 21 +++++- crates/tantivy-core/src/writer.rs | 119 ++++++++++++++++++++++++++++-- 2 files changed, 131 insertions(+), 9 deletions(-) diff --git a/crates/tantivy-core/src/schema.rs b/crates/tantivy-core/src/schema.rs index 54bf144..d23ec6a 100644 --- a/crates/tantivy-core/src/schema.rs +++ b/crates/tantivy-core/src/schema.rs @@ -1,5 +1,5 @@ use serde::Deserialize; -use tantivy::schema::{Schema, STORED, STRING, TEXT}; +use tantivy::schema::{FieldEntry, FieldType, Schema, STORED, STRING, TEXT}; #[derive(Debug, Deserialize)] pub struct FieldsDescriptor { @@ -40,6 +40,25 @@ pub fn build_schema(fields: &FieldsDescriptor) -> Schema { b.build() } +/// ¿Es `entry` un campo clave de match exacto (bucket `keys` -> STRING)? +/// +/// Sólo estos campos pueden usarse como key_field en delete_by_id/update_document: el valor +/// completo se indexa como UN único término, así `delete_term` matchea exactamente el doc de esa +/// clave. Un campo `text` (TEXT, tokenizado con "default") indexa tokens sueltos, no el valor +/// entero, y un `attribute` (STORED, sin indexar) no tiene términos: usar cualquiera de los dos +/// como clave corrompe el índice (duplicados o borrado en masa). +/// +/// El discriminante fiable en tantivy 0.25 es el tokenizer de las opciones de indexado: STRING usa +/// "raw" (valor tal cual = un término), TEXT usa "default". Un campo sin opciones de indexado +/// (attribute) no es indexado -> no es clave. +pub fn field_is_exact_key(entry: &FieldEntry) -> bool { + matches!( + entry.field_type(), + FieldType::Str(opts) + if opts.get_indexing_options().is_some_and(|o| o.tokenizer() == "raw") + ) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/tantivy-core/src/writer.rs b/crates/tantivy-core/src/writer.rs index e176638..311e365 100644 --- a/crates/tantivy-core/src/writer.rs +++ b/crates/tantivy-core/src/writer.rs @@ -1,6 +1,8 @@ +use tantivy::schema::Field; use tantivy::{TantivyDocument, TantivyError, Term}; use crate::registry::IndexState; +use crate::schema::field_is_exact_key; /// Prefijo estable y neutro (no depende del idioma del mensaje) que marca el caso "el writer lock /// exclusivo del índice está tomado por otro proceso" (p. ej. un rebuild en curso). Los bindings PHP @@ -69,15 +71,33 @@ pub fn add_document(state: &mut IndexState, doc_json: &str) -> Result<(), String Ok(()) } +/// Resuelve `key_field` a un `Field` VÁLIDO como clave de borrado exacto, o devuelve un error claro. +/// +/// delete_by_id/update_document borran con `delete_term(Term::from_field_text(field, valor))`, que +/// sólo funciona si el valor entero se indexa como UN término: es decir, un campo del bucket `keys` +/// (STRING, tokenizer "raw"). Pasar un campo `text` (tokenizado) o un `attribute` (sin indexar) +/// corrompe el índice silenciosamente (duplicados en update, borrado en masa en delete), así que se +/// rechaza acá ANTES de tocar el writer. +fn resolve_key_field(state: &IndexState, key_field: &str) -> Result { + let field = state + .schema + .get_field(key_field) + .map_err(|_| format!("campo clave '{key_field}' no existe"))?; + if !field_is_exact_key(state.schema.get_field_entry(field)) { + return Err(format!( + "campo clave '{key_field}' no es una clave de match exacto: debe ser un campo 'keys' \ + (no tokenizado), no un campo 'text' ni un atributo" + )); + } + Ok(field) +} + pub fn delete_by_id( state: &mut IndexState, key_field: &str, key_value: &str, ) -> Result<(), String> { - let field = state - .schema - .get_field(key_field) - .map_err(|_| format!("campo clave '{key_field}' no existe"))?; + let field = resolve_key_field(state, key_field)?; let term = Term::from_field_text(field, key_value); ensure_writer(state)?.delete_term(term); Ok(()) @@ -89,10 +109,7 @@ pub fn update_document( key_value: &str, doc_json: &str, ) -> Result<(), String> { - let field = state - .schema - .get_field(key_field) - .map_err(|_| format!("campo clave '{key_field}' no existe"))?; + let field = resolve_key_field(state, key_field)?; let term = Term::from_field_text(field, key_value); let doc = parse_doc(state, doc_json)?; let w = ensure_writer(state)?; @@ -221,4 +238,90 @@ mod tests { close(h2); let _ = std::fs::remove_dir_all(&dir); } + + // BUG (corrupción de datos): update_document/delete_by_id aceptan CUALQUIER campo como + // key_field, sin validar que sea una clave exacta (bucket `keys` -> STRING, tokenizer "raw"). + // Si se pasa un campo TEXT (tokenizado, tokenizer "default"), Term::from_field_text arma un + // término con el valor COMPLETO, que no existe como término en el índice (sólo existen los + // tokens sueltos). Estos dos tests fijan el comportamiento CORRECTO post-fix: pasar un campo + // tokenizado como clave debe devolver Err y NO corromper el índice. Fallan contra el código + // actual (que devuelve Ok y corrompe). + + #[test] + fn update_on_a_tokenized_field_is_rejected_and_does_not_duplicate() { + let dir = std::env::temp_dir().join(format!("tv_w_{}_tok_update", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let h = open_or_create(cfg(dir.to_str().unwrap())).unwrap(); + + // sembramos un doc con un título multi-palabra en el campo TEXT "title". + with_state(h, |s| { + add_document(s, r#"{"id_key":"1","title":"hola mundo"}"#) + }) + .unwrap(); + with_state(h, commit).unwrap(); + assert_eq!(with_state(h, |s| s.doc_count()).unwrap(), 1); + + // "title" es TEXT (tokenizado) -> NO es una clave válida. Un update contra él debe + // rechazarse. Con el código actual, delete_term("hola mundo") no matchea nada (el índice + // sólo tiene los términos "hola" y "mundo"), el delete es no-op y el add duplica el doc. + let res = with_state(h, |s| { + update_document( + s, + "title", + "hola mundo", + r#"{"id_key":"1","title":"hola mundo"}"#, + ) + }); + with_state(h, commit).unwrap(); + + let n = with_state(h, |s| s.doc_count()).unwrap(); + assert_eq!( + n, 1, + "update sobre una clave tokenizada duplicó el doc: quedan {n} copias (esperaba 1)" + ); + assert!( + res.is_err(), + "update con key_field tokenizado ('title') debe devolver Err, devolvió Ok" + ); + + close(h); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn delete_on_a_tokenized_field_is_rejected_and_does_not_mass_delete() { + let dir = std::env::temp_dir().join(format!("tv_w_{}_tok_delete", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let h = open_or_create(cfg(dir.to_str().unwrap())).unwrap(); + + // dos docs DISTINTOS que comparten el token "shared" en el campo tokenizado "title". + with_state(h, |s| { + add_document(s, r#"{"id_key":"1","title":"shared alpha"}"#) + }) + .unwrap(); + with_state(h, |s| { + add_document(s, r#"{"id_key":"2","title":"shared beta"}"#) + }) + .unwrap(); + with_state(h, commit).unwrap(); + assert_eq!(with_state(h, |s| s.doc_count()).unwrap(), 2); + + // borrar por un campo TEXT usando un token compartido borra TODOS los docs que lo + // contienen (borrado en masa). Debe rechazarse en vez de ejecutarse. + let res = with_state(h, |s| delete_by_id(s, "title", "shared")); + with_state(h, commit).unwrap(); + + let n = with_state(h, |s| s.doc_count()).unwrap(); + assert_eq!( + n, 2, + "delete sobre una clave tokenizada borró en masa: quedan {n} docs (esperaba 2)" + ); + assert!( + res.is_err(), + "delete con key_field tokenizado ('title') debe devolver Err, devolvió Ok" + ); + + close(h); + let _ = std::fs::remove_dir_all(&dir); + } }