From e06778fa75efbcf58c3e1518c449faab51b11346 Mon Sep 17 00:00:00 2001 From: Evgeny Formanenko Date: Thu, 9 Jul 2026 20:12:42 +0300 Subject: [PATCH] fix(query): substrate events emit "callAddress" twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EventFieldSelection`'s projection lists `this.call_address` at two positions, so selecting `callAddress` adds two props with the same name. Nothing downstream deduplicates them: `JsonObject::add` pushes onto a `Vec`, `eval_object` walks it, and `StructEncoder` writes every field. Raw response, before this change: "events":[{"index":0,"callAddress":null,"name":"Treasury.UpdatedInactive","callAddress":null}] RFC 8259 s4 says object names SHOULD be unique; parsers that accept a duplicate keep one of the two. Both props read the same column, so the values agree and no consumer observes a wrong value — but the response carries dead bytes and is not something a strict parser must accept. Removes the *second* occurrence rather than the first. Parsers keep the first occurrence's position, so the parsed key order is exactly what consumers already see today. This change is therefore invisible to any JSON client, and none of the 116 fixture results needed updating. Introduced in 9fcb79df (2024-11-14). Why no test caught it: `test_fixture` runs both sides through `serde_json::from_slice`, and `serde_json`'s map keeps one entry per key, so the duplicate is collapsed before any assertion sees it. The committed `result.json` files were produced the same way and contain one `callAddress` per event. The suite is structurally blind to this class of defect. So this adds two layers of test: * `substrate_event_emits_call_address_once`, a named regression test. It runs a literal query against the moonbeam fixture chunk, asserts on the raw response bytes, then asserts the query actually returned events and that each one carries the property under test — so it cannot pass vacuously, and it does not depend on any fixture's `query.json` continuing to select the field. `execute_query` is split into `execute_query_bytes` so a query can run without a file on disk. * `assert_unique_keys`, wired into `test_fixture`, so every fixture in both the parquet and storage backends now fails on any object that repeats a property name. Generic, not substrate-specific. Both use a small `Deserialize` impl rather than `serde_json::Value`, because `Value` is precisely the thing that hides the bug. Verified red and green. Re-introducing the duplicate projection entry makes the named test fail with `duplicate property "callAddress"`, and makes the corpus-wide check fail on all 18 substrate fixtures that select the field while the other 98 pass — so no other projection in the corpus has this defect. Removing it again makes all 117 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/query/src/query/substrate.rs | 1 - crates/query/tests/fixtures.rs | 138 +++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 6 deletions(-) diff --git a/crates/query/src/query/substrate.rs b/crates/query/src/query/substrate.rs index 40256ef6..a2c79ffb 100644 --- a/crates/query/src/query/substrate.rs +++ b/crates/query/src/query/substrate.rs @@ -135,7 +135,6 @@ item_field_selection! { this.call_address, this.name, this.phase, - this.call_address, this.topics, [this.args]: Json, }} diff --git a/crates/query/tests/fixtures.rs b/crates/query/tests/fixtures.rs index e7bb39ab..36501c54 100644 --- a/crates/query/tests/fixtures.rs +++ b/crates/query/tests/fixtures.rs @@ -1,12 +1,90 @@ use std::{ + collections::HashSet, + fmt, io::ErrorKind, path::{Path, PathBuf} }; +use serde::de::{self, Deserialize, Deserializer, MapAccess, SeqAccess, Visitor}; use sqd_query::{Chunk, JsonArrayWriter, Query}; -fn execute_query(chunk: &dyn Chunk, query_file: impl AsRef) -> anyhow::Result> { - let query = Query::from_json_bytes(&std::fs::read(query_file)?)?; +/// Deserialization target that accepts any JSON document, but fails if some +/// object in it repeats a property name. +/// +/// `serde_json::Value` cannot be used for this: its map keeps a single entry +/// per key, so a repeated property is silently collapsed and the defect never +/// reaches an assertion. Every fixture comparison in this file parses the +/// response, so a duplicate property is invisible to all of them. +struct UniqueKeys; + +impl<'de> Deserialize<'de> for UniqueKeys { + fn deserialize>(deserializer: D) -> Result { + deserializer.deserialize_any(UniqueKeysVisitor) + } +} + +struct UniqueKeysVisitor; + +impl<'de> Visitor<'de> for UniqueKeysVisitor { + type Value = UniqueKeys; + + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("any JSON value") + } + + fn visit_map>(self, mut map: A) -> Result { + let mut seen: HashSet = HashSet::new(); + while let Some(key) = map.next_key::()? { + map.next_value::()?; + if !seen.insert(key.clone()) { + return Err(de::Error::custom(format!("duplicate property {:?}", key))); + } + } + Ok(UniqueKeys) + } + + fn visit_seq>(self, mut seq: A) -> Result { + while seq.next_element::()?.is_some() {} + Ok(UniqueKeys) + } + + fn visit_some>(self, d: D) -> Result { + Deserialize::deserialize(d) + } + + fn visit_bool(self, _: bool) -> Result { + Ok(UniqueKeys) + } + fn visit_i64(self, _: i64) -> Result { + Ok(UniqueKeys) + } + fn visit_u64(self, _: u64) -> Result { + Ok(UniqueKeys) + } + fn visit_f64(self, _: f64) -> Result { + Ok(UniqueKeys) + } + fn visit_str(self, _: &str) -> Result { + Ok(UniqueKeys) + } + fn visit_unit(self) -> Result { + Ok(UniqueKeys) + } + fn visit_none(self) -> Result { + Ok(UniqueKeys) + } +} + +/// A response must be a valid JSON document in which no object repeats a +/// property name (RFC 8259 s4). Checked on the raw bytes, before any parse. +fn assert_unique_keys(bytes: &[u8], query_file: &Path) { + if let Err(err) = serde_json::from_slice::(bytes) { + panic!("{}: {}", query_file.display(), err) + } +} + +fn execute_query_bytes(chunk: &dyn Chunk, query_json: &[u8]) -> anyhow::Result> { + let query = Query::from_json_bytes(query_json)?; let data = Vec::with_capacity(4 * 1024 * 1024); let mut writer = JsonArrayWriter::new(data); if let Some(mut blocks) = query.compile().execute(chunk)? { @@ -15,12 +93,19 @@ fn execute_query(chunk: &dyn Chunk, query_file: impl AsRef) -> anyhow::Res Ok(writer.finish()?) } +fn execute_query(chunk: &dyn Chunk, query_file: impl AsRef) -> anyhow::Result> { + execute_query_bytes(chunk, &std::fs::read(query_file)?) +} + fn test_fixture(chunk: &dyn Chunk, query_file: PathBuf) { let case_dir = query_file.parent().unwrap(); let result_file = case_dir.join("result.json"); let actual: serde_json::Value = match execute_query(chunk, &query_file) { - Ok(bytes) => serde_json::from_slice(&bytes).unwrap(), + Ok(bytes) => { + assert_unique_keys(&bytes, &query_file); + serde_json::from_slice(&bytes).unwrap() + } Err(err) => serde_json::Value::String(err.to_string()) }; @@ -49,12 +134,12 @@ fn test_fixture(chunk: &dyn Chunk, query_file: PathBuf) { #[cfg(feature = "parquet")] mod parquet { - use std::path::PathBuf; + use std::path::{Path, PathBuf}; use rstest::rstest; use sqd_query::ParquetChunk; - use crate::test_fixture; + use crate::{assert_unique_keys, execute_query_bytes, test_fixture}; #[rstest] fn query(#[files("fixtures/*/queries/*/query.json")] query_file: PathBuf) { @@ -63,6 +148,49 @@ mod parquet { let chunk = ParquetChunk::new(chunk_dir.to_str().unwrap()); test_fixture(&chunk, query_file) } + + /// A substrate event must carry `callAddress` exactly once. + /// + /// The projection used to list the column twice, so the property was + /// written twice into every event object. No fixture could catch it: the + /// comparison parses both sides, and a JSON parser keeps one entry per + /// key, collapsing the duplicate before any assertion runs. + /// + /// This test therefore asserts on the raw response bytes, and does not + /// depend on any fixture's `query.json` continuing to select the field. + #[test] + fn substrate_event_emits_call_address_once() { + let chunk = ParquetChunk::new("fixtures/moonbeam/chunk"); + let query = br#"{ + "type": "substrate", + "fields": {"event": {"index": true, "callAddress": true, "name": true}}, + "events": [{}] + }"#; + + let bytes = execute_query_bytes(&chunk, query).unwrap(); + + // The duplicate lives in the bytes, not in the parsed document. + assert_unique_keys(&bytes, Path::new("substrate_event_emits_call_address_once")); + + // Guard against a vacuous pass: the query must actually have returned + // events, and each must actually carry the property under test. + let blocks: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + let events: Vec<&serde_json::Value> = blocks + .as_array() + .unwrap() + .iter() + .filter_map(|block| block.get("events")) + .flat_map(|events| events.as_array().unwrap()) + .collect(); + + assert!(!events.is_empty(), "fixture chunk returned no events"); + for event in &events { + assert!( + event.get("callAddress").is_some(), + "selected property is missing from {event}" + ); + } + } } #[cfg(feature = "storage")]