From 70ea56303247c7653e9f3e563970fd1ac6989c11 Mon Sep 17 00:00:00 2001 From: angela-helios Date: Fri, 31 Jul 2026 17:17:58 -0400 Subject: [PATCH] fix(sqlite): negate token :not at the resource level The :not modifier wrapped the row predicate in NOT(...) inside the per-parameter subquery, which asks "does any indexed row differ from the value" instead of FHIR's "does no value match". Multi-valued elements (identifier arrays, CodeableConcept codings, communication.language) leaked back in through their non-matching rows, and resources without the element at all -- matches, per the spec -- never joined the index and were silently dropped. The token handler now always builds the positive predicate and the query builder flips the subquery membership to resource_id NOT IN (...) when the modifier is :not, which handles both failure modes in one place. Postgres already implemented these semantics; this aligns sqlite with it. Closes #473 --- .../sqlite/search/parameter_handlers/token.rs | 16 ++- .../backends/sqlite/search/query_builder.rs | 16 ++- .../tests/search/modifier_tests.rs | 107 ++++++++++++++++++ 3 files changed, 131 insertions(+), 8 deletions(-) diff --git a/crates/persistence/src/backends/sqlite/search/parameter_handlers/token.rs b/crates/persistence/src/backends/sqlite/search/parameter_handlers/token.rs index 7c4d5cc74..632ce9a95 100644 --- a/crates/persistence/src/backends/sqlite/search/parameter_handlers/token.rs +++ b/crates/persistence/src/backends/sqlite/search/parameter_handlers/token.rs @@ -25,10 +25,13 @@ impl TokenHandler { ) -> SqlFragment { let param_num = param_offset + 1; - // Handle :not modifier + // :not builds the POSITIVE predicate here; the query builder negates + // at the resource level (`resource_id NOT IN (…)`, #473). Negating the + // row predicate turned :not into "some row differs": any multi-valued + // resource leaked in through its other rows, and resources with no + // rows for the parameter — matches, per FHIR — never joined at all. if matches!(modifier, Some(SearchModifier::Not)) { - let inner = Self::build_sql(value, None, param_offset); - return SqlFragment::with_params(format!("NOT ({})", inner.sql), inner.params); + return Self::build_sql(value, None, param_offset); } // Handle :text modifier - search on display text (Coding.display, CodeableConcept.text) @@ -266,11 +269,14 @@ mod tests { } #[test] - fn test_token_not_modifier() { + fn test_token_not_modifier_builds_positive_predicate() { + // :not is negated at the resource level by the query builder + // (resource_id NOT IN …, #473); the row predicate must stay positive. let value = SearchValue::new(SearchPrefix::Eq, "12345"); let frag = TokenHandler::build_sql(&value, Some(&SearchModifier::Not), 0); - assert!(frag.sql.starts_with("NOT (")); + assert!(!frag.sql.contains("NOT")); + assert!(frag.sql.contains("value_token_code = ?1")); } #[test] diff --git a/crates/persistence/src/backends/sqlite/search/query_builder.rs b/crates/persistence/src/backends/sqlite/search/query_builder.rs index c64b89036..d83197c59 100644 --- a/crates/persistence/src/backends/sqlite/search/query_builder.rs +++ b/crates/persistence/src/backends/sqlite/search/query_builder.rs @@ -414,11 +414,21 @@ impl QueryBuilder { combined = combined.or(cond); } - // Wrap in subquery to ensure proper AND/OR semantics + // Wrap in subquery to ensure proper AND/OR semantics. `:not` negates + // HERE, at the resource level, not inside the row predicate (#473): + // FHIR's :not means "no value of the parameter matches", so a + // multi-valued resource must not slip through via its other rows, and + // resources with no rows for the parameter count as matches — both of + // which NOT IN gives and a row-level NOT cannot. + let membership = if matches!(param.modifier, Some(SearchModifier::Not)) { + "NOT IN" + } else { + "IN" + }; Some(SqlFragment::with_params( format!( - "resource_id IN (SELECT resource_id FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2 AND param_name = '{}' AND ({}))", - param.name, combined.sql + "resource_id {} (SELECT resource_id FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2 AND param_name = '{}' AND ({}))", + membership, param.name, combined.sql ), combined.params, )) diff --git a/crates/persistence/tests/search/modifier_tests.rs b/crates/persistence/tests/search/modifier_tests.rs index afb682f32..d4cc2bf64 100644 --- a/crates/persistence/tests/search/modifier_tests.rs +++ b/crates/persistence/tests/search/modifier_tests.rs @@ -159,6 +159,113 @@ async fn test_not_modifier() { } } +/// :not means "no value of the parameter matches" — a resource whose element +/// is absent entirely has no matching value, so it must be returned (#473). +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_not_modifier_includes_resources_missing_the_element() { + let backend = create_sqlite_backend(); + let tenant = create_tenant(); + + let male = json!({"resourceType": "Patient", "gender": "male"}); + let female = json!({"resourceType": "Patient", "gender": "female"}); + let no_gender = json!({"resourceType": "Patient"}); + backend + .create(&tenant, "Patient", male, FhirVersion::default()) + .await + .unwrap(); + backend + .create(&tenant, "Patient", female, FhirVersion::default()) + .await + .unwrap(); + backend + .create(&tenant, "Patient", no_gender, FhirVersion::default()) + .await + .unwrap(); + + let query = SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "gender".to_string(), + param_type: SearchParamType::Token, + modifier: Some(SearchModifier::Not), + values: vec![SearchValue::token(None, "male")], + chain: vec![], + components: vec![], + }); + + let result = backend + .search(&tenant, &query.with_count(100)) + .await + .unwrap(); + + assert_eq!( + result.resources.len(), + 2, + "gender:not=male must return the female patient and the one with no gender" + ); + assert!( + result + .resources + .items + .iter() + .any(|r| r.content().get("gender").is_none()), + "a patient without a gender element must match gender:not=male" + ); +} + +/// :not must exclude a resource when ANY of its values matches — negating per +/// row let multi-valued resources leak back in through their other rows (#473). +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_not_modifier_excludes_multi_valued_match() { + let backend = create_sqlite_backend(); + let tenant = create_tenant(); + + let dual_coded = json!({ + "resourceType": "Observation", + "status": "final", + "code": {"coding": [ + {"system": "http://loinc.org", "code": "8302-2"}, + {"system": "http://snomed.info/sct", "code": "50373000"} + ]} + }); + let other = json!({ + "resourceType": "Observation", + "status": "final", + "code": {"coding": [{"system": "http://loinc.org", "code": "8867-4"}]} + }); + backend + .create(&tenant, "Observation", dual_coded, FhirVersion::default()) + .await + .unwrap(); + let other_id = backend + .create(&tenant, "Observation", other, FhirVersion::default()) + .await + .unwrap() + .id() + .to_string(); + + let query = SearchQuery::new("Observation").with_parameter(SearchParameter { + name: "code".to_string(), + param_type: SearchParamType::Token, + modifier: Some(SearchModifier::Not), + values: vec![SearchValue::token(Some("http://loinc.org"), "8302-2")], + chain: vec![], + components: vec![], + }); + + let result = backend + .search(&tenant, &query.with_count(100)) + .await + .unwrap(); + + let ids: Vec<&str> = result.resources.items.iter().map(|r| r.id()).collect(); + assert_eq!( + ids, + vec![other_id.as_str()], + "the observation carrying 8302-2 must not leak in via its SNOMED coding" + ); +} + // ============================================================================ // :text Modifier Tests // ============================================================================