From 1036b7e33a4360df8b99f81ef03492fee8328bd0 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Mon, 29 Jun 2026 01:30:42 +0900 Subject: refactor(json): replace seld/jsonlint with serde_json Validate JSON syntax with serde_json's parse errors in JsonFile, and detect duplicate keys in ConfigValidator with a hand-written serde visitor, dropping the now-unused JsonParser/Lexer/DuplicateKeyException ports. ParsingException is kept as the thrown error type and downcast signal. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/shirabe/src/json/json_file.rs | 59 ++++++----- crates/shirabe/src/util/config_validator.rs | 150 +++++++++++++++++++++++++--- crates/shirabe/tests/json/json_file_test.rs | 34 ++++--- 3 files changed, 183 insertions(+), 60 deletions(-) (limited to 'crates/shirabe') diff --git a/crates/shirabe/src/json/json_file.rs b/crates/shirabe/src/json/json_file.rs index 0f609f2..b95732f 100644 --- a/crates/shirabe/src/json/json_file.rs +++ b/crates/shirabe/src/json/json_file.rs @@ -10,8 +10,7 @@ use crate::util::HttpDownloader; use crate::util::Silencer; use indexmap::IndexMap; use shirabe_external_packages::composer::pcre::{CaptureKey, Preg}; -use shirabe_external_packages::seld::json_lint::JsonParser; -use shirabe_external_packages::seld::json_lint::ParsingException; +use shirabe_external_packages::seld::json_lint::{ParsingException, ParsingExceptionDetails}; use shirabe_php_shim::{ InvalidArgumentException, JSON_PRETTY_PRINT, JSON_UNESCAPED_SLASHES, JSON_UNESCAPED_UNICODE, PhpMixed, RuntimeException, UnexpectedValueException, dirname, file_exists, file_get_contents, @@ -537,39 +536,37 @@ impl JsonFile { /// @throws ParsingException /// @return bool true on success pub(crate) fn validate_syntax(json: &str, file: Option<&str>) -> anyhow::Result { - let mut parser = JsonParser::new(); - let result = parser.lint(json); - if result.is_none() { - // TODO(phase-c): Rust's &str is guaranteed as UTF-8, but PHP string is not. Change `json` - // to &[u8] and check UTF-8 validity here. - - // if (defined('JSON_ERROR_UTF8') && JSON_ERROR_UTF8 === json_last_error()) { - // if ($file === null) { - // throw new \UnexpectedValueException('The input is not UTF-8, could not parse as JSON'); - // } else { - // throw new \UnexpectedValueException('"' . $file . '" is not UTF-8, could not parse as JSON'); - // } - // } - - return Ok(true); - } + // TODO(phase-d): make json_decode() returns an error object with details. + let error = match serde_json::from_str::(json) { + Ok(_) => { + // TODO(phase-c): Rust's &str is guaranteed as UTF-8, but PHP string is not. Change `json` + // to &[u8] and check UTF-8 validity here. + + // if (defined('JSON_ERROR_UTF8') && JSON_ERROR_UTF8 === json_last_error()) { + // if ($file === null) { + // throw new \UnexpectedValueException('The input is not UTF-8, could not parse as JSON'); + // } else { + // throw new \UnexpectedValueException('"' . $file . '" is not UTF-8, could not parse as JSON'); + // } + // } + + return Ok(true); + } + Err(e) => e, + }; - let result = result.unwrap(); + let details = ParsingExceptionDetails { + line: Some(error.line() as i64), + ..Default::default() + }; Err(match file { None => ParsingException::new( - format!( - "The input does not contain valid JSON\n{}", - result.get_message() - ), - result.get_details().clone(), + format!("The input does not contain valid JSON\n{error}"), + details, ), - Some(f) => ParsingException::new( - format!( - "\"{}\" does not contain valid JSON\n{}", - f, - result.get_message() - ), - result.get_details().clone(), + Some(file) => ParsingException::new( + format!("\"{file}\" does not contain valid JSON\n{error}"), + details, ), } .into()) diff --git a/crates/shirabe/src/util/config_validator.rs b/crates/shirabe/src/util/config_validator.rs index 689b107..849521f 100644 --- a/crates/shirabe/src/util/config_validator.rs +++ b/crates/shirabe/src/util/config_validator.rs @@ -7,9 +7,8 @@ use crate::package::loader::ArrayLoader; use crate::package::loader::InvalidPackageException; use crate::package::loader::ValidatingArrayLoader; use indexmap::IndexMap; +use serde::de::Error as _; use shirabe_external_packages::composer::pcre::Preg; -use shirabe_external_packages::seld::json_lint::DuplicateKeyException; -use shirabe_external_packages::seld::json_lint::JsonParser; use shirabe_php_shim::PhpMixed; use shirabe_spdx_licenses::SpdxLicenses; @@ -72,20 +71,9 @@ impl ConfigValidator { } if manifest.is_some() { - let json_parser = JsonParser::new(); let contents = shirabe_php_shim::file_get_contents(file).unwrap_or_default(); - let parse_result = json_parser.parse(&contents, JsonParser::DETECT_KEY_CONFLICTS); - match parse_result { - Ok(_) => {} - Err(e) => { - if let Some(dup_e) = e.downcast_ref::() { - let details = dup_e.get_details(); - warnings.push(format!( - "Key {} is a duplicate in {} at line {}", - details["key"], file, details["line"] - )); - } - } + if let Some((key, line)) = detect_duplicate_keys(&contents) { + warnings.push(format!("Key {key} is a duplicate in {file} at line {line}")); } } @@ -324,3 +312,135 @@ impl ConfigValidator { (errors, publish_errors, warnings) } } + +/// Detects the first duplicate object key in a JSON text, returning its key name and line number. +/// Replaces `Seld\JsonLint\JsonParser` with `DETECT_KEY_CONFLICTS`. +/// Returns `None` on no collision, even if any other error occurs. +fn detect_duplicate_keys(json: &str) -> Option<(String, usize)> { + let duplicate_key: std::cell::Cell> = std::cell::Cell::new(None); + + let mut deserializer = serde_json::Deserializer::from_str(json); + let seed = DuplicateKeyScanSeed { + duplicate_key: &duplicate_key, + }; + match serde::de::DeserializeSeed::deserialize(seed, &mut deserializer) { + Ok(()) => None, + Err(e) => duplicate_key.take().map(|key| (key, e.line())), + } +} + +/// Carries the cell that records the first duplicate key found while recursing into a value. +struct DuplicateKeyScanSeed<'a> { + duplicate_key: &'a std::cell::Cell>, +} + +impl<'de> serde::de::DeserializeSeed<'de> for DuplicateKeyScanSeed<'_> { + type Value = (); + + fn deserialize(self, deserializer: D) -> Result<(), D::Error> + where + D: serde::Deserializer<'de>, + { + deserializer.deserialize_any(DuplicateKeyScanVisitor { + duplicate_key: self.duplicate_key, + }) + } +} + +struct DuplicateKeyScanVisitor<'a> { + duplicate_key: &'a std::cell::Cell>, +} + +impl<'de> serde::de::Visitor<'de> for DuplicateKeyScanVisitor<'_> { + type Value = (); + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("any JSON value") + } + + fn visit_map(self, mut map: A) -> Result<(), A::Error> + where + A: serde::de::MapAccess<'de>, + { + let mut seen: indexmap::IndexSet = indexmap::IndexSet::new(); + while let Some(key) = map.next_key::()? { + if !seen.insert(key.clone()) { + self.duplicate_key.set(Some(key)); + return Err(A::Error::custom("found duplicate key")); + } + map.next_value_seed(DuplicateKeyScanSeed { + duplicate_key: self.duplicate_key, + })?; + } + Ok(()) + } + + fn visit_seq(self, mut seq: A) -> Result<(), A::Error> + where + A: serde::de::SeqAccess<'de>, + { + while seq + .next_element_seed(DuplicateKeyScanSeed { + duplicate_key: self.duplicate_key, + })? + .is_some() + {} + Ok(()) + } + + fn visit_bool(self, _v: bool) -> Result<(), E> { + Ok(()) + } + + fn visit_i64(self, _v: i64) -> Result<(), E> { + Ok(()) + } + + fn visit_u64(self, _v: u64) -> Result<(), E> { + Ok(()) + } + + fn visit_f64(self, _v: f64) -> Result<(), E> { + Ok(()) + } + + fn visit_str(self, _v: &str) -> Result<(), E> { + Ok(()) + } + + fn visit_unit(self) -> Result<(), E> { + Ok(()) + } + + fn visit_none(self) -> Result<(), E> { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_duplicates() { + assert_eq!(detect_duplicate_keys("{\"a\": 1, \"b\": {\"c\": 2}}"), None); + } + + #[test] + fn top_level_duplicate_reports_key_and_line() { + let json = "{\n \"name\": \"a\",\n \"name\": \"b\"\n}"; + assert_eq!(detect_duplicate_keys(json), Some(("name".to_string(), 3))); + } + + #[test] + fn nested_duplicate_is_detected() { + let json = + "{\n \"require\": {\n \"x/y\": \"1\",\n \"x/y\": \"2\"\n }\n}"; + assert_eq!(detect_duplicate_keys(json), Some(("x/y".to_string(), 4))); + } + + #[test] + fn syntax_error_is_not_reported_as_duplicate() { + assert_eq!(detect_duplicate_keys("{ not json"), None); + } +} diff --git a/crates/shirabe/tests/json/json_file_test.rs b/crates/shirabe/tests/json/json_file_test.rs index 3b56d35..217592f 100644 --- a/crates/shirabe/tests/json/json_file_test.rs +++ b/crates/shirabe/tests/json/json_file_test.rs @@ -42,64 +42,70 @@ fn create_temp_file() -> tempfile::NamedTempFile { tempfile::NamedTempFile::new().expect("failed to create a temporary file") } +// INCOMPATIBILITY NOTE: the test_parse_error_* cases below assert serde_json's parse-error wording +// instead of upstream's. Composer (via seld/jsonlint) reports "Parse error on line N:" with +// jsonlint's own line numbering and hint messages; shirabe surfaces serde_json's errors, which flag +// the same syntax errors with different wording and sometimes a different line/column (serde_json +// points at where parsing fails, jsonlint at the offending token). This is a deliberate, permanent +// incompatibility: the seld/jsonlint parser is no longer used for JSON syntax validation. #[test] fn test_parse_error_detect_extra_comma() { let json = "{\n \"foo\": \"bar\",\n}"; - expect_parse_exception("Parse error on line 2", json); + expect_parse_exception("trailing comma at line 3 column 1", json); } #[test] fn test_parse_error_detect_extra_comma_in_array() { let json = "{\n \"foo\": [\n \"bar\",\n ]\n}"; - expect_parse_exception("Parse error on line 3", json); + expect_parse_exception("trailing comma at line 4 column 9", json); } #[test] fn test_parse_error_detect_unescaped_backslash() { let json = "{\n \"fo\\o\": \"bar\"\n}"; - expect_parse_exception("Parse error on line 1", json); + expect_parse_exception("invalid escape at line 2 column 13", json); } #[test] fn test_parse_error_skips_escaped_backslash() { let json = "{\n \"fo\\\\o\": \"bar\"\n \"a\": \"b\"\n}"; - expect_parse_exception("Parse error on line 2", json); + expect_parse_exception("expected `,` or `}` at line 3 column 9", json); } #[test] fn test_parse_error_detect_single_quotes() { let json = "{\n 'foo': \"bar\"\n}"; - expect_parse_exception("Parse error on line 1", json); + expect_parse_exception("key must be a string at line 2 column 9", json); } #[test] fn test_parse_error_detect_missing_quotes() { let json = "{\n foo: \"bar\"\n}"; - expect_parse_exception("Parse error on line 1", json); + expect_parse_exception("key must be a string at line 2 column 9", json); } #[test] fn test_parse_error_detect_array_as_hash() { let json = "{\n \"foo\": [\"bar\": \"baz\"]\n}"; - expect_parse_exception("Parse error on line 2", json); + expect_parse_exception("expected `,` or `]` at line 2 column 22", json); } #[test] fn test_parse_error_detect_missing_comma() { let json = "{\n \"foo\": \"bar\"\n \"bar\": \"foo\"\n}"; - expect_parse_exception("Parse error on line 2", json); + expect_parse_exception("expected `,` or `}` at line 3 column 9", json); } #[test] fn test_parse_error_detect_missing_comma_multiline() { let json = "{\n \"foo\": \"barbar\"\n\n \"bar\": \"foo\"\n}"; - expect_parse_exception("Parse error on line 2", json); + expect_parse_exception("expected `,` or `}` at line 4 column 9", json); } #[test] fn test_parse_error_detect_missing_colon() { let json = "{\n \"foo\": \"bar\",\n \"bar\" \"foo\"\n}"; - expect_parse_exception("Parse error on line 3", json); + expect_parse_exception("expected `:` at line 3 column 15", json); } #[test] @@ -247,7 +253,7 @@ fn test_schema_validation() { #[test] fn test_schema_validation_error() { - // WORDING NOTE: upstream asserts justinrainbow's "name : NULL value found, but a string is + // INCOMPATIBILITY NOTE: upstream asserts justinrainbow's "name : NULL value found, but a string is // required". The jsonschema crate reports the same violation with different wording. let file_tmp = create_temp_file(); let file = file_tmp.path().to_str().unwrap().to_string(); @@ -273,7 +279,7 @@ fn test_schema_validation_error() { #[test] fn test_schema_validation_lax_additional_properties() { - // WORDING NOTE: upstream asserts justinrainbow's "The property foo is not defined and the + // INCOMPATIBILITY NOTE: upstream asserts justinrainbow's "The property foo is not defined and the // definition does not allow additional properties". The jsonschema crate reports the same // additionalProperties violation with different wording. let file_tmp = create_temp_file(); @@ -303,7 +309,7 @@ fn test_schema_validation_lax_additional_properties() { #[test] fn test_schema_validation_lax_required() { - // WORDING NOTE: upstream asserts justinrainbow's property-prefixed strings such as + // INCOMPATIBILITY NOTE: upstream asserts justinrainbow's property-prefixed strings such as // "name : The property name is required". The jsonschema crate phrases the message // differently ("name : \"name\" is a required property"), but the property prefix is reconstructed, // so the "PROPERTY : MESSAGE" shape matches: "name : \"name\" is a required property". @@ -425,7 +431,7 @@ fn test_custom_schema_validation_strict() { #[test] fn test_auth_schema_validation_with_custom_data_source() { - // WORDING NOTE: upstream asserts justinrainbow's "github-oauth : String value found, but an + // INCOMPATIBILITY NOTE: upstream asserts justinrainbow's "github-oauth : String value found, but an // object is required". The jsonschema crate reports the same type violation with different // wording. let json = shirabe_php_shim::json_decode("{\"github-oauth\": \"foo\"}", false).unwrap(); -- cgit v1.3.1