From 3100dbdd11e127f2e599bf3b1d520d4eade439ed Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" Date: Tue, 12 Nov 2024 18:31:58 -0500 Subject: [PATCH] Add error module and wrap all valid module errors Add the `error` module, defining an Error enum using `thiserror`. Create variants for all of the errors generated by the `valid` module, and replace its custom `ValidationError` enum. Includes tests for each variant. Then teach that module to return only our errors. Note the special treatment for these error conversions: * `boon::ValidationError`s use lifetimes to hang on to values from the boon compiler and from the JSON that fails validation. This makes it quite tricky to use separate from those values. But since, so far, we just want to print JSON Schema validation errors, add a `From` trait implementation that simply copies its string value into our error type. This allows all the lifetime annotations to be removed from the `valid` crate. * `wax::GlobError` is an enum that wraps the two types that the `wax` create generates. To coerce those two types, add a `From` trait for each. While at it, improve the error generation for the custom `license` format so that the error message generated by boon is more legible and useful. Previously it just showed the value that failed --- twice --- but now it also shows the "reason" for the error. Also: change the behavior of `is_license` and `is_path` to return `Ok(())` if the value is not a string, following the example of the format validation functions in boon itself. New tests for the `license` and `path` custom formats validate the usability of the error messages. --- CHANGELOG.md | 18 +++++ Cargo.lock | 1 + Cargo.toml | 1 + src/error/mod.rs | 60 +++++++++++++++++ src/error/tests.rs | 91 ++++++++++++++++++++++++++ src/lib.rs | 1 + src/valid/compiler/mod.rs | 134 +++++++++++++++++++++++++++----------- src/valid/mod.rs | 80 +++++------------------ 8 files changed, 287 insertions(+), 99 deletions(-) create mode 100644 src/error/mod.rs create mode 100644 src/error/tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 589b987..01b7a11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ All notable changes to this project will be documented in this file. It uses the [Semantic Versioning]: https://semver.org/spec/v2.0.0.html "Semantic Versioning 2.0.0" +## [v0.5.0] — Unreleased + +### ⚡ Improvements + +* Added the [error module], which defines all the errors returned by + pgxn_meta. +* Changed the errors returned from the [valid module] from boxed + [boon] errors with lifetimes to [error module] errors with no lifetimes. + +### 📔 Notes + +* Removed the `valid::ValidationError` enum. + + [v0.5.0]: https://github.com/pgxn/meta/compare/v0.4.0...v0.5.0 + [error module]: https://docs.rs/pgxn_meta/0.5.0/pgxn_meta/error/ + [valid module]: https://docs.rs/pgxn_meta/0.5.0/pgxn_meta/valid/ + [boon]: https://github.com/santhosh-tekuri/boon + ## [v0.4.0] — 2024-10-08 The theme of this release is *JSON Web Signatures.* diff --git a/Cargo.lock b/Cargo.lock index c1054bc..a5b7a69 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -508,6 +508,7 @@ dependencies = [ "serde_with", "spdx", "tempfile", + "thiserror", "wax", ] diff --git a/Cargo.toml b/Cargo.toml index 7e88164..ac5a749 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ serde = { version = "1", features = ["derive"] } serde_json = "1.0" serde_with = { version = "3.9.0", features = ["hex"] } spdx = "0.10.6" +thiserror = "1.0" wax = "0.6.0" [build-dependencies] diff --git a/src/error/mod.rs b/src/error/mod.rs new file mode 100644 index 0000000..640e9cb --- /dev/null +++ b/src/error/mod.rs @@ -0,0 +1,60 @@ +//! PGXN Meta Errors. + +#[cfg(test)] +mod tests; + +/// Build errors. +#[derive(thiserror::Error, Debug)] +pub enum Error { + /// License Error. + #[error("{}", .0.reason)] + License(#[from] spdx::error::ParseError), + + /// Validator cannot determine the version of the meta spec. + #[error("Cannot determine meta-spec version")] + UnknownSpec, + + /// A schema file has no `$id` property. + #[error("No $id found in schema")] + UnknownSchemaId, + + /// JSON Schema compile error. + #[error(transparent)] + #[allow(clippy::enum_variant_names)] + CompileError(#[from] boon::CompileError), + + /// JSON Schema validation error. + #[error("{0}")] + #[allow(clippy::enum_variant_names)] + ValidationError(String), + + /// Serde JSON error. + #[error(transparent)] + Serde(#[from] serde_json::Error), + + /// IO error. + #[error(transparent)] + Io(#[from] std::io::Error), + + /// Glob build error. + #[error(transparent)] + Glob(#[from] wax::GlobError), +} + +impl<'s, 'v> From> for Error { + fn from(value: boon::ValidationError<'s, 'v>) -> Self { + Self::ValidationError(value.to_string()) + } +} + +impl From for Error { + fn from(value: wax::BuildError) -> Self { + wax::GlobError::Build(value).into() + } +} + +impl From for Error { + fn from(value: wax::WalkError) -> Self { + wax::GlobError::Walk(value).into() + } +} diff --git a/src/error/tests.rs b/src/error/tests.rs new file mode 100644 index 0000000..10310a8 --- /dev/null +++ b/src/error/tests.rs @@ -0,0 +1,91 @@ +use super::*; +use serde_json::json; + +#[test] +fn spdx() { + let parse_error = spdx::Expression::parse("not a license").unwrap_err(); + let exp = parse_error.reason.to_string(); + let not_exp = parse_error.to_string(); + let err: Error = parse_error.into(); + assert!(matches!(err, Error::License { .. })); + assert_eq!(exp, err.to_string()); + assert_ne!(not_exp, err.to_string()); +} + +#[test] +fn unknown_spec() { + assert_eq!( + Error::UnknownSpec.to_string(), + "Cannot determine meta-spec version" + ) +} + +#[test] +fn unknown_schema_id() { + assert_eq!(Error::UnknownSchemaId.to_string(), "No $id found in schema") +} + +#[test] +fn compile() { + let mut c = boon::Compiler::new(); + c.add_resource("foo", json!("not a schema")).unwrap(); + let mut s = boon::Schemas::new(); + let compile_err = c.compile("foo", &mut s).unwrap_err(); + let exp = compile_err.to_string(); + let err: Error = compile_err.into(); + assert!(matches!(err, Error::CompileError { .. })); + assert_eq!(exp, err.to_string(),); +} + +#[test] +fn validation() { + let mut c = boon::Compiler::new(); + c.add_resource("foo", json!({"type": "object"})).unwrap(); + let mut s = boon::Schemas::new(); + let idx = c.compile("foo", &mut s).unwrap(); + let json = json!([]); + let valid_err = s.validate(&json, idx).unwrap_err(); + let exp = valid_err.to_string(); + let err: Error = valid_err.into(); + assert!(matches!(err, Error::ValidationError { .. })); + assert_eq!(exp, err.to_string()); +} + +#[test] +fn serde() { + let serde_err = serde_json::from_str::("[]").unwrap_err(); + let exp = serde_err.to_string(); + let err: Error = serde_err.into(); + assert!(matches!(err, Error::Serde { .. })); + assert_eq!(exp, err.to_string()); +} + +#[test] +fn io() { + use std::io; + let io_error = io::Error::new(io::ErrorKind::Other, "oh no!"); + let exp = io_error.to_string(); + let err: Error = io_error.into(); + assert!(matches!(err, Error::Io { .. })); + assert_eq!(exp, err.to_string()); +} + +#[test] +fn glob() { + let build_err = wax::Glob::new("[].json").unwrap_err(); + let exp = build_err.to_string(); + let err: Error = build_err.into(); + assert!(matches!(err, Error::Glob { .. })); + assert_eq!(exp, err.to_string()); + + let glob = wax::Glob::new("*.json").unwrap(); + for path in glob.walk("nonesuch 😇") { + // Would be nice to just fetch the first, but should always be an + // error anyway. + let walk_err = path.unwrap_err(); + let exp = walk_err.to_string(); + let err: Error = walk_err.into(); + assert!(matches!(err, Error::Glob { .. })); + assert_eq!(exp, err.to_string()); + } +} diff --git a/src/lib.rs b/src/lib.rs index d1e2f6f..c3a07f9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,7 @@ files. It supports both the [v1] and [v2] specs. */ pub mod dist; +pub mod error; pub mod release; mod util; // private utilities pub mod valid; diff --git a/src/valid/compiler/mod.rs b/src/valid/compiler/mod.rs index b1c1131..e138563 100644 --- a/src/valid/compiler/mod.rs +++ b/src/valid/compiler/mod.rs @@ -1,9 +1,8 @@ -use relative_path::{Component, RelativePath}; +use crate::error::Error; /// Public but undocumented and un-exported module that creates a /// boon::Compiler for use in validation and Tests. -use std::error::Error; - use boon::Compiler; +use relative_path::{Component, RelativePath}; use serde_json::Value; /// new returns a new boon::Compiler with the schema files loaded from `dir` @@ -18,7 +17,7 @@ pub fn new() -> Compiler { let schema: Value = serde_json::from_str(line).unwrap(); let id = &schema["$id"] .as_str() - .ok_or(super::ValidationError::UnknownID) + .ok_or(Error::UnknownSchemaId) .unwrap(); compiler.add_resource(id, schema.to_owned()).unwrap(); } @@ -44,27 +43,23 @@ pub fn spec_compiler() -> Compiler { } /// Returns an error if v is not a valid path. -fn is_path(v: &Value) -> Result<(), Box> { - let Value::String(s) = v else { - Err("not a string")? - }; +fn is_path(v: &Value) -> Result<(), Box> { + let Value::String(s) = v else { return Ok(()) }; let path = RelativePath::new(s); for c in path.components() { if c == Component::ParentDir { - Err("parent dir")? + Err("references parent dir")? }; } Ok(()) } -/// Returns an error if vi is not a valid SPDX license expression. -fn is_license(v: &Value) -> Result<(), Box> { - let Value::String(s) = v else { - Err("not a string")? - }; - _ = spdx::Expression::parse(s)?; +/// Returns an error if v is not a valid SPDX license expression. +fn is_license(v: &Value) -> Result<(), Box> { + let Value::String(s) = v else { return Ok(()) }; + _ = spdx::Expression::parse(s).map_err(crate::error::Error::License)?; Ok(()) } @@ -96,17 +91,17 @@ mod tests { } // Test invalid paths. - for invalid in [ - json!("../outside/path"), - json!("thing/../other"), - json!({}), - json!([]), - json!(true), - json!(null), - json!(42), + for (name, invalid, err) in [ + ("parent", json!("../outside/path"), "references parent dir"), + ( + "sub parent", + json!("thing/../other"), + "references parent dir", + ), ] { - if is_path(&invalid).is_ok() { - panic!("{} unexpectedly passed!", invalid) + match is_path(&invalid) { + Ok(_) => panic!("{name} unexpectedly passed!"), + Err(e) => assert_eq!(err, e.to_string(), "{name}"), } } } @@ -137,24 +132,89 @@ mod tests { } // Test invalid licenses. - for invalid_license in [ - json!(""), - json!(null), - json!("0"), - json!(0), - json!("\n\t"), - json!("()"), - json!("AND"), - json!("OR"), + for (name, invalid_license, reason) in [ + ("empty string", json!(""), spdx::error::Reason::Empty), + ("zero", json!("0"), spdx::error::Reason::UnknownTerm), + ("control chars", json!("\n\t"), spdx::error::Reason::Empty), + ( + "parens", + json!("()"), + spdx::error::Reason::Unexpected(&["", "("]), + ), + ( + "and", + json!("AND"), + spdx::error::Reason::Unexpected(&["", "("]), + ), + ( + "or", + json!("OR"), + spdx::error::Reason::Unexpected(&["", "("]), + ), + ] { + match is_license(&invalid_license) { + Ok(_) => panic!("{name} unexpectedly passed!"), + Err(e) => assert_eq!(reason.to_string(), e.to_string(), "{name}"), + } + } + } + + #[test] + fn test_spec_compiler() -> Result<(), Error> { + let mut c = spec_compiler(); + let id = "format"; + // Compile simple schema to validate license and path. + c.add_resource( + id, + json!({ + "type": "object", + "properties": { + "path": { + "type": "string", + "format": "path", + }, + "license": { + "type": "string", + "format": "license", + } + } + }), + )?; + + let mut schemas = Schemas::new(); + let idx = c.compile(id, &mut schemas)?; + + for (name, json, err) in [ + ( + "empty license", + json!({"license": ""}), + "at '/license': '' is not valid license: empty expression", + ), + ( + "zero license", + json!({"license": "0"}), + "at '/license': '0' is not valid license: unknown term", + ), + ( + "parent path", + json!({"path": "../foo"}), + "'../foo' is not valid path: references parent dir", + ), ] { - if is_license(&invalid_license).is_ok() { - panic!("{} unexpectedly passed!", invalid_license) + match schemas.validate(&json, idx) { + Ok(_) => panic!("{name} unexpectedly succeeded"), + Err(e) => { + println!("{e}"); + assert!(e.to_string().contains(err), "{name}") + } } } + + Ok(()) } #[test] - fn test_new() -> Result<(), Box> { + fn test_new() -> Result<(), Error> { let mut compiler = new(); for tc in [("v1", "widget.json"), ("v2", "typical-sql.json")] { diff --git a/src/valid/mod.rs b/src/valid/mod.rs index a8a28b2..00b6f69 100644 --- a/src/valid/mod.rs +++ b/src/valid/mod.rs @@ -7,7 +7,7 @@ It supports both the [v1] and [v2] specs. # Example ``` rust -# use std::error::Error; +# use pgxn_meta::error::Error; use serde_json::json; use pgxn_meta::valid::*; @@ -30,16 +30,14 @@ let meta = json!({ let mut validator = Validator::new(); assert!(validator.validate(&meta).is_ok()); -# Ok::<(), Box>(()) +# Ok::<(), Error>(()) ``` [v1]: https://rfcs.pgxn.org/0001-meta-spec-v1.html [v2]: https://github.com/pgxn/rfcs/pull/3 */ -use std::{error::Error, fmt}; - -use crate::util; +use crate::{error::Error, util}; use boon::{Compiler, Schemas}; use serde_json::Value; @@ -56,28 +54,6 @@ pub struct Validator { schemas: Schemas, } -/// Errors returned by Validator are ValidationError objects. -#[derive(Debug, PartialEq)] -pub enum ValidationError { - /// UnknownSpec errors are returned when the validator cannot determine - /// the version of the meta spec. - UnknownSpec, - /// UnknownID errors are returned by new() when a schema file has no `$id` - /// property. - UnknownID, -} - -impl Error for ValidationError {} - -impl fmt::Display for ValidationError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - ValidationError::UnknownSpec => write!(f, "Cannot determine meta-spec version"), - ValidationError::UnknownID => write!(f, "No $id found in schema"), - } - } -} - /// The base URL for all JSON schemas. const SCHEMA_BASE: &str = "https://pgxn.org/meta/v"; @@ -111,7 +87,7 @@ impl Validator { /// success and a validation error on failure. /// /// See the [module docs](crate::valid) for an example. - pub fn validate<'a>(&'a mut self, meta: &'a Value) -> Result> { + pub fn validate(&mut self, meta: &Value) -> Result { self.validate_schema(meta, "distribution.schema.json") } @@ -134,7 +110,7 @@ impl Validator { /// [JSON Serialization]: https://datatracker.ietf.org/doc/html/rfc7515#section-7.2 /// [RFC 5]: https://github.com/pgxn/rfcs/pull/5 /// [JSON Web Signature]: https://datatracker.ietf.org/doc/html/rfc7515 - pub fn validate_release<'a>(&'a mut self, meta: &'a Value) -> Result> { + pub fn validate_release(&mut self, meta: &Value) -> Result { self.validate_schema(meta, "release.schema.json") } @@ -151,25 +127,16 @@ impl Validator { /// /// [JSON Serialization]: https://datatracker.ietf.org/doc/html/rfc7515#section-7.2 /// [RFC 5]: https://github.com/pgxn/rfcs/pull/5 - pub fn validate_payload<'a>(&'a mut self, meta: &'a Value) -> Result<(), Box> { + pub fn validate_payload(&mut self, meta: &Value) -> Result<(), Error> { self.validate_version_schema(meta, 2, "payload.schema.json") } - fn validate_schema<'a>( - &'a mut self, - meta: &'a Value, - schema: &str, - ) -> Result> { - let v = util::get_version(meta).ok_or(ValidationError::UnknownSpec)?; + fn validate_schema(&mut self, meta: &Value, schema: &str) -> Result { + let v = util::get_version(meta).ok_or(Error::UnknownSpec)?; self.validate_version_schema(meta, v, schema).map(|()| v) } - fn validate_version_schema<'a>( - &'a mut self, - meta: &'a Value, - v: u8, - schema: &str, - ) -> Result<(), Box> { + fn validate_version_schema(&mut self, meta: &Value, v: u8, schema: &str) -> Result<(), Error> { let id = format!("{SCHEMA_BASE}{v}/{schema}"); let compiler = &mut self.compiler; @@ -184,12 +151,13 @@ impl Validator { #[cfg(test)] mod tests { use super::*; + use crate::error::Error; use serde_json::{json, Value}; - use std::{error::Error, fs::File, path::PathBuf}; + use std::{fs::File, path::PathBuf}; use wax::Glob; #[test] - fn test_corpus() -> Result<(), Box> { + fn test_corpus() -> Result<(), Error> { let mut validator = Validator::default(); for (version, release_patch) in [ @@ -242,19 +210,7 @@ mod tests { } #[test] - fn test_errors() { - assert_eq!( - format!("{}", ValidationError::UnknownSpec), - "Cannot determine meta-spec version", - ); - assert_eq!( - format!("{}", ValidationError::UnknownID), - "No $id found in schema", - ); - } - - #[test] - fn test_unknown_versions() -> Result<(), Box> { + fn test_unknown_versions() -> Result<(), Error> { let mut validator = Validator::new(); for (name, json) in [ @@ -285,7 +241,7 @@ mod tests { Ok(()) } - fn load_minimal() -> Result<(Value, Value), Box> { + fn load_minimal() -> Result<(Value, Value), Error> { let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus"].iter().collect(); let file = dir.join("v1").join("howto.json"); let v1: Value = serde_json::from_reader(File::open(file)?)?; @@ -295,7 +251,7 @@ mod tests { } #[test] - fn test_invalid_distribution() -> Result<(), Box> { + fn test_invalid_distribution() -> Result<(), Error> { let mut validator = Validator::new(); let (v1, v2) = load_minimal()?; @@ -340,7 +296,7 @@ mod tests { "v2 invalid license", &v2, json!({"license": "lol no"}), - "'/license': 'lol no' is not valid license: lol no", + "'/license': 'lol no' is not valid license: unknown term", ), ( "v1 missing control", @@ -366,7 +322,7 @@ mod tests { } #[test] - fn test_invalid_release() -> Result<(), Box> { + fn test_invalid_release() -> Result<(), Error> { let mut validator = Validator::new(); let (v1, v2) = load_minimal()?; for (name, meta, patch, err) in [ @@ -427,7 +383,7 @@ mod tests { } #[test] - fn test_payload() -> Result<(), Box> { + fn test_payload() -> Result<(), Error> { let mut validator = Validator::new(); for (name, payload) in [ (