From fb02ea2f47e72243a9f4726d34d4e5989cd49402 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" Date: Wed, 13 Nov 2024 14:36:26 -0500 Subject: [PATCH] Replace dist errors Add property validation and parameter errors to the error module and replace all `Box`s in the dist module with `error:Error`s. Also, make the first word of all error messages consistently lowercase. --- src/dist/mod.rs | 37 ++++++++++++++++--------------------- src/dist/tests.rs | 35 +++++++++++++++++------------------ src/dist/v1/mod.rs | 44 ++++++++++++++++++++------------------------ src/dist/v1/tests.rs | 34 +++++++++++++++++++++------------- src/dist/v2/mod.rs | 9 +++------ src/error/mod.rs | 16 ++++++++++++++-- src/error/tests.rs | 35 +++++++++++++++++++++++++++++++++-- src/release/tests.rs | 2 +- src/valid/mod.rs | 4 ++-- 9 files changed, 127 insertions(+), 89 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 06f06ff..b37b93a 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -8,9 +8,9 @@ distribution `META.json` files. It supports both the [v1] and [v2] specs. [v2]: https://github.com/pgxn/rfcs/pull/3 */ -use std::{borrow::Borrow, collections::HashMap, error::Error, fs::File, path::Path}; +use std::{borrow::Borrow, collections::HashMap, fs::File, path::Path}; -use crate::util; +use crate::{error::Error, util}; use relative_path::{RelativePath, RelativePathBuf}; use semver::Version; use serde::{Deserialize, Deserializer, Serialize}; @@ -828,18 +828,18 @@ where impl Distribution { /// Deserializes `meta`, which contains PGXN `version` metadata, into a /// [`Distribution`]. - fn from_version(version: u8, meta: Value) -> Result> { + fn from_version(version: u8, meta: Value) -> Result { match version { 1 => v1::from_value(meta), 2 => v2::from_value(meta), - _ => Err(Box::from(format!("Unknown meta version {version}"))), + _ => Err(Error::UnknownSpec), } } /// Loads the release `META.json` data from `file` then converts into a /// [`Distribution`]. Returns an error on file error or if the content of /// `file` is not valid PGXN `META.json` data. - pub fn load>(file: P) -> Result> { + pub fn load>(file: P) -> Result { let meta: Value = serde_json::from_reader(File::open(file)?)?; meta.try_into() } @@ -922,7 +922,7 @@ impl Distribution { } impl TryFrom for Distribution { - type Error = Box; + type Error = Error; /// Converts the PGXN `META.json` data from `meta` into a /// [`Distribution`]. Returns an error if `meta` is invalid. /// @@ -959,16 +959,13 @@ impl TryFrom for Distribution { fn try_from(meta: Value) -> Result { // Make sure it's valid. let mut validator = crate::valid::Validator::new(); - let version = match validator.validate(&meta) { - Err(e) => return Err(Box::from(e.to_string())), - Ok(v) => v, - }; + let version = validator.validate(&meta)?; Distribution::from_version(version, meta) } } impl TryFrom<&[&Value]> for Distribution { - type Error = Box; + type Error = Error; /// Merge multiple PGXN `META.json` data from `meta` into a /// [`Distribution`]. Returns an error if `meta` is invalid. /// @@ -1013,12 +1010,11 @@ impl TryFrom<&[&Value]> for Distribution { /// [RFC 7396]: https:///www.rfc-editor.org/rfc/rfc7396.html fn try_from(meta: &[&Value]) -> Result { if meta.is_empty() { - return Err(Box::from("meta contains no values")); + return Err(Error::Param("meta contains no values")); } // Find the version of the first doc. - let version = - util::get_version(meta[0]).ok_or("No spec version found in first meta value")?; + let version = util::get_version(meta[0]).ok_or(Error::UnknownSpec)?; // Convert the first doc to v2 if necessary. let mut v2 = match version { @@ -1034,21 +1030,20 @@ impl TryFrom<&[&Value]> for Distribution { // Validate the patched doc and return. let mut validator = crate::valid::Validator::new(); - validator.validate(&v2).map_err(|e| e.to_string())?; + validator.validate(&v2)?; Distribution::from_version(2, v2) } } impl TryFrom for Value { - type Error = Box; + type Error = Error; /// Converts `meta` into a [serde_json::Value]. /// /// # Example /// /// ``` rust - /// # use std::error::Error; /// use serde_json::{json, Value}; - /// use pgxn_meta::dist::*; + /// use pgxn_meta::{error::Error, dist::*}; /// /// let meta_json = json!({ /// "name": "pair", @@ -1072,7 +1067,7 @@ impl TryFrom for Value { /// /// let meta = Distribution::try_from(meta_json); /// assert!(meta.is_ok()); - /// let val: Result> = meta.unwrap().try_into(); + /// let val: Result = meta.unwrap().try_into(); /// assert!(val.is_ok()); /// ``` fn try_from(meta: Distribution) -> Result { @@ -1082,7 +1077,7 @@ impl TryFrom for Value { } impl TryFrom<&String> for Distribution { - type Error = Box; + type Error = Error; /// Converts `str` into JSON and then into a [`Distribution`]. Returns an /// error if the content of `str` is not valid PGXN `META.json` data. fn try_from(str: &String) -> Result { @@ -1092,7 +1087,7 @@ impl TryFrom<&String> for Distribution { } impl TryFrom for String { - type Error = Box; + type Error = Error; /// Converts `meta` into a JSON String. fn try_from(meta: Distribution) -> Result { let val = serde_json::to_string(&meta)?; diff --git a/src/dist/tests.rs b/src/dist/tests.rs index 99d3add..4cca3b8 100644 --- a/src/dist/tests.rs +++ b/src/dist/tests.rs @@ -1,14 +1,13 @@ use super::*; use serde_json::{json, Value}; use std::{ - error::Error, fs::{self, File}, path::PathBuf, }; use wax::Glob; #[test] -fn test_corpus() -> Result<(), Box> { +fn test_corpus() -> Result<(), Error> { for v_dir in ["v1", "v2"] { let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus", v_dir] .iter() @@ -32,10 +31,11 @@ fn test_corpus() -> Result<(), Box> { if v_dir == "v2" { assert_eq!(contents.get("license").unwrap(), dist.license()); // Make sure round-trip produces the same JSON. - let output: Result> = dist.try_into(); + let output: Result = dist.try_into(); match output { Err(e) => panic!("{v_dir}/{:?} failed: {e}", path.file_name().unwrap()), Ok(val) => { + let val: Value = serde_json::from_str(&val)?; assert_json_diff::assert_json_eq!(&contents, &val); } }; @@ -49,11 +49,10 @@ fn test_corpus() -> Result<(), Box> { Ok(dist) => { if v_dir == "v2" { // Make sure value round-trip produces the same JSON. - let output: Result> = dist.try_into(); + let output: Result = dist.try_into(); match output { Err(e) => panic!("{v_dir}/{:?} failed: {e}", path.file_name().unwrap()), Ok(val) => { - let val: Value = serde_json::from_str(&val)?; assert_json_diff::assert_json_eq!(&contents, &val); } }; @@ -68,7 +67,7 @@ fn test_corpus() -> Result<(), Box> { } #[test] -fn test_bad_corpus() -> Result<(), Box> { +fn test_bad_corpus() -> Result<(), Error> { let file: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus", "invalid.json"] .iter() .collect(); @@ -89,7 +88,7 @@ fn test_bad_corpus() -> Result<(), Box> { "Should have failed on {:?} but did not", file.file_name().unwrap() ), - Err(e) => assert_eq!("Unknown meta version 99", e.to_string()), + Err(e) => assert_eq!("cannot determine meta-spec version", e.to_string()), } // Should fail when no meta-spec. @@ -99,7 +98,7 @@ fn test_bad_corpus() -> Result<(), Box> { "Should have failed on {:?} but did not", file.file_name().unwrap() ), - Err(e) => assert_eq!("Cannot determine meta-spec version", e.to_string()), + Err(e) => assert_eq!("cannot determine meta-spec version", e.to_string()), } // Make sure we catch a failure parsing into a Distribution struct. @@ -112,7 +111,7 @@ fn test_bad_corpus() -> Result<(), Box> { } #[test] -fn test_try_merge_v1() -> Result<(), Box> { +fn test_try_merge_v1() -> Result<(), Error> { // Load a v1 META file. let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus"].iter().collect(); let widget_file = dir.join("v1").join("widget.json"); @@ -169,7 +168,7 @@ fn test_try_merge_v1() -> Result<(), Box> { } #[test] -fn test_try_merge_v2() -> Result<(), Box> { +fn test_try_merge_v2() -> Result<(), Error> { // Load a v2 META file. let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus"].iter().collect(); let widget_file = dir.join("v2").join("minimal.json"); @@ -214,7 +213,7 @@ fn run_merge_case( orig: &Value, patches: &[Value], expect: &Value, -) -> Result<(), Box> { +) -> Result<(), Error> { let mut meta = vec![orig]; for p in patches { meta.push(p); @@ -223,7 +222,7 @@ fn run_merge_case( Err(e) => panic!("patching {name} failed: {e}"), Ok(dist) => { // Convert the Distribution object to JSON. - let output: Result> = dist.try_into(); + let output: Result = dist.try_into(); match output { Err(e) => panic!("{name} serialization failed: {e}"), Ok(val) => { @@ -240,7 +239,7 @@ fn run_merge_case( } #[test] -fn test_try_merge_err() -> Result<(), Box> { +fn test_try_merge_err() -> Result<(), Error> { // Load invalid meta. let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus"].iter().collect(); let widget_file = dir.join("invalid.json"); @@ -254,12 +253,12 @@ fn test_try_merge_err() -> Result<(), Box> { ( "no version", vec![&empty], - "No spec version found in first meta value", + "cannot determine meta-spec version", ), ( "bad version", vec![&bad_version], - "No spec version found in first meta value", + "cannot determine meta-spec version", ), ( "invalid", @@ -277,7 +276,7 @@ fn test_try_merge_err() -> Result<(), Box> { } #[test] -fn test_try_merge_partman() -> Result<(), Box> { +fn test_try_merge_partman() -> Result<(), Error> { // Test the real-world pg_partman JSON with a patch to build the expected // v2 JSON. First, load the original metadata. let original_meta = json!({ @@ -410,7 +409,7 @@ fn test_try_merge_partman() -> Result<(), Box> { Err(e) => panic!("patching part man failed: {e}"), Ok(dist) => { // Convert the Distributions object to JSON. - let output: Result> = dist.try_into(); + let output: Result = dist.try_into(); match output { Err(e) => panic!("partman serialization failed: {e}"), Ok(val) => { @@ -1336,7 +1335,7 @@ fn test_artifact() { } #[test] -fn test_distribution() -> Result<(), Box> { +fn test_distribution() -> Result<(), Error> { let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus", "v2"] .iter() .collect(); diff --git a/src/dist/v1/mod.rs b/src/dist/v1/mod.rs index 8a75a33..dd9392c 100644 --- a/src/dist/v1/mod.rs +++ b/src/dist/v1/mod.rs @@ -1,11 +1,12 @@ use super::Distribution; +use crate::error::Error; use email_address::EmailAddress; use serde_json::{json, Map, Value}; -use std::{error::Error, str::FromStr}; +use std::str::FromStr; /// to_v2 parses v1, which contains PGXN v1 metadata, into a JSON object /// containing valid PGXN v2 metadata. -pub fn to_v2(v1: &Value) -> Result> { +pub fn to_v2(v1: &Value) -> Result { // Copy common fields. let mut v2 = v1_to_v2_common(v1); @@ -43,7 +44,7 @@ pub fn to_v2(v1: &Value) -> Result> { /// from_value parses v1, which contains PGXN v1 metadata, into a /// [`Distribution`] object containing valid PGXN v2 metadata. -pub fn from_value(v1: Value) -> Result> { +pub fn from_value(v1: Value) -> Result { to_v2(&v1)?.try_into() } @@ -104,7 +105,7 @@ fn v1_to_v2_custom_props(v1: &Map, v2: &mut Map) { /// attempts to parse an email address from each maintainer in v1; if there is /// no email address, it sets `url` the value in `resources.homepage`, if /// present, and otherwise to `https://pgxn.org`. -fn v1_to_v2_maintainers(v1: &Value) -> Result> { +fn v1_to_v2_maintainers(v1: &Value) -> Result { if let Some(maintainer) = v1.get("maintainer") { return match maintainer { Value::Array(list) => parse_v1_maintainers(v1, list), @@ -112,10 +113,10 @@ fn v1_to_v2_maintainers(v1: &Value) -> Result> { let list = vec![maintainer.clone()]; parse_v1_maintainers(v1, &list) } - _ => Err(Box::from(format!("Invalid v1 maintainer: {maintainer}"))), + _ => Err(Error::Invalid("maintainer", 1, maintainer.clone())), }; } - Err(Box::from("maintainer property missing")) + Err(Error::Missing("maintainer")) } /// parse_v1_maintainers parses list for a list of v1 maintainer strings and @@ -125,7 +126,7 @@ fn v1_to_v2_maintainers(v1: &Value) -> Result> { /// Otherwise the string will be saved as the maintainer `name` and the `url` /// set to either the `homepage` in the `resources` object in `v1`, or else /// `https://pgxn.org`. -fn parse_v1_maintainers(v1: &Value, list: &[Value]) -> Result> { +fn parse_v1_maintainers(v1: &Value, list: &[Value]) -> Result { let mut new_list: Vec = Vec::with_capacity(list.len()); for v in list { if let Some(str) = v.as_str() { @@ -150,7 +151,7 @@ fn parse_v1_maintainers(v1: &Value, list: &[Value]) -> Result Result Result> { +fn v1_to_v2_license(v1: &Value) -> Result { if let Some(license) = v1.get("license") { return match license { Value::String(l) => { if let Some(name) = license_expression_for(l.as_str()) { return Ok(Value::String(name.to_string())); } - Err(Box::from(format!("Invalid v1 license: {license}"))) + Err(Error::Invalid("license", 1, license.clone())) } Value::Array(list) => { // https://users.rust-lang.org/t/replace-elements-of-a-vector-as-a-function-of-previous-values/101618/6 @@ -189,10 +190,10 @@ fn v1_to_v2_license(v1: &Value) -> Result> { Value::String(s) => { match license_expression_for(s.as_str()) { Some(name) => v.push(name.to_string()), - None => return Err(Box::from(format!("Invalid v1 license: {ln}"))), + None => return Err(Error::Invalid("license", 1, ln.clone())), }; } - _ => return Err(Box::from(format!("Invalid v1 license: {ln}"))), + _ => return Err(Error::Invalid("license", 1, ln.clone())), }; } return Ok(Value::String(v.join(" OR "))); @@ -219,15 +220,15 @@ fn v1_to_v2_license(v1: &Value) -> Result> { "restricted", Some("https://github.com/diffix/pg_diffix/blob/master/LICENSE.md"), ) => list.push("BUSL-1.1".to_string()), - _ => return Err(Box::from(format!("Unknown v1 license: {k}: {v}"))), + _ => return Err(Error::Invalid("license", 1, v.clone())), } } return Ok(Value::String(list.join(" OR "))); } - _ => Err(Box::from(format!("Invalid v1 license: {license}"))), + _ => Err(Error::Invalid("license", 1, license.clone())), }; } - Err(Box::from("license property missing")) + Err(Error::Missing("license")) } /// license_expression_for maps the list of v1 open source license names to @@ -276,7 +277,7 @@ fn license_expression_for(name: &str) -> Option<&str> { /// /// Returns the resulting object in a valid `contents` object with /// `extensions` as the sole property. -fn v1_to_v2_contents(v1: &Value) -> Result> { +fn v1_to_v2_contents(v1: &Value) -> Result { if let Some(provides) = v1.get("provides") { // Assume everything is an extension. It's not true, but most common. let mut extensions = Map::new(); @@ -310,21 +311,16 @@ fn v1_to_v2_contents(v1: &Value) -> Result> { extensions.insert(ext.to_string(), Value::Object(v2_spec)); } - _ => { - return Err(Box::from(format!( - "Invalid v1 {:?} extension value: {spec}", - ext, - ))) - } + _ => return Err(Error::Invalid("extension", 1, spec.clone())), } } } else { - return Err(Box::from(format!("Invalid v1 provides value: {provides}"))); + return Err(Error::Invalid("provides", 1, provides.clone())); } return Ok(json!({"extensions": extensions})); } - Err(Box::from("provides property missing")) + Err(Error::Missing("provides")) } /// v1_to_v2_classifications clones the tags array in v1 into an object with diff --git a/src/dist/v1/tests.rs b/src/dist/v1/tests.rs index 631d6b9..5d67f9c 100644 --- a/src/dist/v1/tests.rs +++ b/src/dist/v1/tests.rs @@ -116,22 +116,22 @@ fn test_v1_to_v2_maintainers_errors() { ( "null maintainer", json!({"maintainer": null}), - "Invalid v1 maintainer: null", + "invalid v1 maintainer value: null", ), ( "object maintainer", json!({"maintainer": {"name": "hi"}}), - r#"Invalid v1 maintainer: {"name":"hi"}"#, + r#"invalid v1 maintainer value: {"name":"hi"}"#, ), ( "null maintainer item", json!({"maintainer": [null]}), - "Invalid v1 maintainer: null", + "invalid v1 maintainer value: null", ), ( "true maintainer item", json!({"maintainer": [true]}), - "Invalid v1 maintainer: true", + "invalid v1 maintainer value: true", ), ] { match v1_to_v2_maintainers(&input) { @@ -249,25 +249,33 @@ fn test_v1_v2_licenses_error() { ( "unknown string", json!({"license": "nonesuch"}), - "Invalid v1 license: \"nonesuch\"", + "invalid v1 license value: \"nonesuch\"", ), ( "unknown array", json!({"license": ["nonesuch"]}), - "Invalid v1 license: \"nonesuch\"", + "invalid v1 license value: \"nonesuch\"", ), ( "array non-string item", json!({"license": [{"x": "y"}]}), - "Invalid v1 license: {\"x\":\"y\"}", + "invalid v1 license value: {\"x\":\"y\"}", ), ( "unknown object", json!({"license": {"nonesuch": "http://example.com"}}), - "Unknown v1 license: nonesuch: \"http://example.com\"", + "invalid v1 license value: \"http://example.com\"", + ), + ( + "number", + json!({"license": 42}), + "invalid v1 license value: 42", + ), + ( + "null", + json!({"license": null}), + "invalid v1 license value: null", ), - ("number", json!({"license": 42}), "Invalid v1 license: 42"), - ("null", json!({"license": null}), "Invalid v1 license: null"), ("nonexistent", json!({}), "license property missing"), ] { match v1_to_v2_license(&input) { @@ -333,12 +341,12 @@ fn test_v1_v2_contents_err() { ( "provides null", json!({"provides": null}), - "Invalid v1 provides value: null", + "invalid v1 provides value: null", ), ( "extension not object", json!({"provides": {"foo": []}}), - "Invalid v1 \"foo\" extension value: []", + "invalid v1 extension value: []", ), ] { match v1_to_v2_contents(&input) { @@ -678,7 +686,7 @@ fn test_v1_v2_resources() { } #[test] -fn test_from_value() -> Result<(), Box> { +fn test_from_value() -> Result<(), Error> { use wax::Glob; let dir: PathBuf = [env!("CARGO_MANIFEST_DIR"), "corpus", "v1"] .iter() diff --git a/src/dist/v2/mod.rs b/src/dist/v2/mod.rs index 97abf0c..531ed1f 100644 --- a/src/dist/v2/mod.rs +++ b/src/dist/v2/mod.rs @@ -1,10 +1,7 @@ use super::Distribution; +use crate::error::Error; use serde_json::Value; -use std::error::Error; -pub fn from_value(meta: Value) -> Result> { - match serde_json::from_value(meta) { - Ok(m) => Ok(m), - Err(e) => Err(Box::from(e)), - } +pub fn from_value(meta: Value) -> Result { + Ok(serde_json::from_value(meta)?) } diff --git a/src/error/mod.rs b/src/error/mod.rs index 640e9cb..36a0c98 100644 --- a/src/error/mod.rs +++ b/src/error/mod.rs @@ -11,11 +11,11 @@ pub enum Error { License(#[from] spdx::error::ParseError), /// Validator cannot determine the version of the meta spec. - #[error("Cannot determine meta-spec version")] + #[error("cannot determine meta-spec version")] UnknownSpec, /// A schema file has no `$id` property. - #[error("No $id found in schema")] + #[error("no $id found in schema")] UnknownSchemaId, /// JSON Schema compile error. @@ -39,6 +39,18 @@ pub enum Error { /// Glob build error. #[error(transparent)] Glob(#[from] wax::GlobError), + + /// Parameter errors. + #[error("{0}")] + Param(&'static str), + + /// Invalid property value. + #[error("invalid v{1} {0} value: {2}")] + Invalid(&'static str, u8, serde_json::Value), + + /// Missing property value. + #[error("{0} property missing")] + Missing(&'static str), } impl<'s, 'v> From> for Error { diff --git a/src/error/tests.rs b/src/error/tests.rs index 10310a8..d504a7c 100644 --- a/src/error/tests.rs +++ b/src/error/tests.rs @@ -16,13 +16,13 @@ fn spdx() { fn unknown_spec() { assert_eq!( Error::UnknownSpec.to_string(), - "Cannot determine meta-spec version" + "cannot determine meta-spec version" ) } #[test] fn unknown_schema_id() { - assert_eq!(Error::UnknownSchemaId.to_string(), "No $id found in schema") + assert_eq!(Error::UnknownSchemaId.to_string(), "no $id found in schema") } #[test] @@ -89,3 +89,34 @@ fn glob() { assert_eq!(exp, err.to_string()); } } + +#[test] +fn parameter() { + assert_eq!("invalid hi", Error::Param("invalid hi").to_string()) +} + +#[test] +fn invalid() { + for (name, err, exp) in [ + ( + "v1 thing", + Error::Invalid("thing", 1, json!("hi")), + "invalid v1 thing value: \"hi\"", + ), + ( + "v2 bag array", + Error::Invalid("bag", 2, json!([])), + "invalid v2 bag value: []", + ), + ] { + assert_eq!(exp, err.to_string(), "{name}"); + } +} + +#[test] +fn missing() { + assert_eq!( + "thing property missing", + Error::Missing("thing").to_string() + ) +} diff --git a/src/release/tests.rs b/src/release/tests.rs index eeb68f8..4e7eb90 100644 --- a/src/release/tests.rs +++ b/src/release/tests.rs @@ -177,7 +177,7 @@ fn test_bad_corpus() -> Result<(), Box> { meta.as_object_mut().unwrap().remove("meta-spec"); match Release::try_from(meta.clone()) { Ok(_) => panic!("Unexpected success with no meta-spec"), - Err(e) => assert_eq!("Cannot determine meta-spec version", e.to_string()), + Err(e) => assert_eq!("cannot determine meta-spec version", e.to_string()), } // Should fail on missing certs object. diff --git a/src/valid/mod.rs b/src/valid/mod.rs index 00b6f69..78f46e5 100644 --- a/src/valid/mod.rs +++ b/src/valid/mod.rs @@ -222,7 +222,7 @@ mod tests { ] { match validator.validate(&json) { Err(e) => assert_eq!( - "Cannot determine meta-spec version", + "cannot determine meta-spec version", e.to_string(), "{name} validate" ), @@ -230,7 +230,7 @@ mod tests { } match validator.validate_release(&json) { Err(e) => assert_eq!( - "Cannot determine meta-spec version", + "cannot determine meta-spec version", e.to_string(), "{name} validate_release" ),