Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: custom error types for cargo-util-schemas #13186

Merged
merged 10 commits into from
Dec 20, 2023
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ cargo-platform = { path = "crates/cargo-platform", version = "0.1.4" }
cargo-test-macro = { path = "crates/cargo-test-macro" }
cargo-test-support = { path = "crates/cargo-test-support" }
cargo-util = { version = "0.2.6", path = "crates/cargo-util" }
cargo-util-schemas = { version = "0.1.0", path = "crates/cargo-util-schemas" }
cargo-util-schemas = { version = "0.2.0", path = "crates/cargo-util-schemas" }
cargo_metadata = "0.18.1"
clap = "4.4.10"
color-print = "0.3.5"
Expand Down
4 changes: 2 additions & 2 deletions crates/cargo-util-schemas/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "cargo-util-schemas"
version = "0.1.1"
version = "0.2.0"
rust-version.workspace = true
edition.workspace = true
license.workspace = true
Expand All @@ -9,11 +9,11 @@ repository.workspace = true
description = "Deserialization schemas for Cargo"

[dependencies]
anyhow.workspace = true
semver.workspace = true
serde = { workspace = true, features = ["derive"] }
serde-untagged.workspace = true
serde-value.workspace = true
thiserror.workspace = true
toml.workspace = true
unicode-xid.workspace = true
url.workspace = true
Expand Down
2 changes: 2 additions & 0 deletions crates/cargo-util-schemas/src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ mod partial_version;
mod source_kind;

pub use package_id_spec::PackageIdSpec;
pub use package_id_spec::PackageIdSpecError;
pub use partial_version::PartialVersion;
pub use partial_version::PartialVersionError;
pub use source_kind::GitReference;
pub use source_kind::SourceKind;
89 changes: 63 additions & 26 deletions crates/cargo-util-schemas/src/core/package_id_spec.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
use std::fmt;

use anyhow::bail;
use anyhow::Result;
use semver::Version;
use serde::{de, ser};
use url::Url;

use crate::core::GitReference;
use crate::core::PartialVersion;
use crate::core::PartialVersionError;
use crate::core::SourceKind;
use crate::manifest::PackageName;
use crate::restricted_names::NameValidationError;

type Result<T> = std::result::Result<T, PackageIdSpecError>;
weihanglo marked this conversation as resolved.
Show resolved Hide resolved

/// Some or all of the data required to identify a package:
///
Expand Down Expand Up @@ -83,12 +85,11 @@ impl PackageIdSpec {
if abs.exists() {
let maybe_url = Url::from_file_path(abs)
.map_or_else(|_| "a file:// URL".to_string(), |url| url.to_string());
bail!(
"package ID specification `{}` looks like a file path, \
maybe try {}",
spec,
maybe_url
);
return Err(ErrorKind::MaybeFilePath {
spec: spec.into(),
maybe_url,
}
.into());
}
}
let mut parts = spec.splitn(2, [':', '@']);
Expand Down Expand Up @@ -119,51 +120,44 @@ impl PackageIdSpec {
}
"registry" => {
if url.query().is_some() {
bail!("cannot have a query string in a pkgid: {url}")
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
kind = Some(SourceKind::Registry);
url = strip_url_protocol(&url);
}
"sparse" => {
if url.query().is_some() {
bail!("cannot have a query string in a pkgid: {url}")
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
kind = Some(SourceKind::SparseRegistry);
// Leave `sparse` as part of URL, see `SourceId::new`
// url = strip_url_protocol(&url);
}
"path" => {
if url.query().is_some() {
bail!("cannot have a query string in a pkgid: {url}")
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
if scheme != "file" {
anyhow::bail!("`path+{scheme}` is unsupported; `path+file` and `file` schemes are supported");
return Err(ErrorKind::UnsupportedPathPlusScheme(scheme.into()).into());
}
kind = Some(SourceKind::Path);
url = strip_url_protocol(&url);
}
kind => anyhow::bail!("unsupported source protocol: {kind}"),
kind => return Err(ErrorKind::UnsupportedProtocol(kind.into()).into()),
}
} else {
if url.query().is_some() {
bail!("cannot have a query string in a pkgid: {url}")
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
}

let frag = url.fragment().map(|s| s.to_owned());
url.set_fragment(None);

let (name, version) = {
let mut path = url
.path_segments()
.ok_or_else(|| anyhow::format_err!("pkgid urls must have a path: {}", url))?;
let path_name = path.next_back().ok_or_else(|| {
anyhow::format_err!(
"pkgid urls must have at least one path \
component: {}",
url
)
})?;
let Some(path_name) = url.path_segments().and_then(|mut p| p.next_back()) else {
return Err(ErrorKind::MissingUrlPath(url).into());
};
match frag {
Some(fragment) => match fragment.split_once([':', '@']) {
Some((name, part)) => {
Expand Down Expand Up @@ -259,7 +253,7 @@ impl fmt::Display for PackageIdSpec {
}

impl ser::Serialize for PackageIdSpec {
fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
fn serialize<S>(&self, s: S) -> std::result::Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
Expand All @@ -268,7 +262,7 @@ impl ser::Serialize for PackageIdSpec {
}

impl<'de> de::Deserialize<'de> for PackageIdSpec {
fn deserialize<D>(d: D) -> Result<PackageIdSpec, D::Error>
fn deserialize<D>(d: D) -> std::result::Result<PackageIdSpec, D::Error>
where
D: de::Deserializer<'de>,
{
Expand All @@ -277,6 +271,49 @@ impl<'de> de::Deserialize<'de> for PackageIdSpec {
}
}

/// Error parsing a [`PackageIdSpec`].
#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct PackageIdSpecError(#[from] ErrorKind);

impl From<PartialVersionError> for PackageIdSpecError {
fn from(value: PartialVersionError) -> Self {
ErrorKind::PartialVersion(value).into()
}
}

impl From<NameValidationError> for PackageIdSpecError {
fn from(value: NameValidationError) -> Self {
ErrorKind::NameValidation(value).into()
}
}

/// Non-public error kind for [`PackageIdSpecError`].
#[non_exhaustive]
#[derive(Debug, thiserror::Error)]
enum ErrorKind {
#[error("unsupported source protocol: {0}")]
UnsupportedProtocol(String),

#[error("`path+{0}` is unsupported; `path+file` and `file` schemes are supported")]
UnsupportedPathPlusScheme(String),

#[error("cannot have a query string in a pkgid: {0}")]
UnexpectedQueryString(Url),

#[error("pkgid urls must have at least one path component: {0}")]
MissingUrlPath(Url),

#[error("package ID specification `{spec}` looks like a file path, maybe try {maybe_url}")]
MaybeFilePath { spec: String, maybe_url: String },

#[error(transparent)]
NameValidation(#[from] crate::restricted_names::NameValidationError),

#[error(transparent)]
PartialVersion(#[from] crate::core::PartialVersionError),
}

#[cfg(test)]
mod tests {
use super::PackageIdSpec;
Expand Down
38 changes: 27 additions & 11 deletions crates/cargo-util-schemas/src/core/partial_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,27 +80,21 @@ impl From<semver::Version> for PartialVersion {
}

impl std::str::FromStr for PartialVersion {
type Err = anyhow::Error;
type Err = PartialVersionError;

fn from_str(value: &str) -> Result<Self, Self::Err> {
if is_req(value) {
anyhow::bail!("unexpected version requirement, expected a version like \"1.32\"")
return Err(ErrorKind::VersionReq.into());
}
match semver::Version::parse(value) {
Ok(ver) => Ok(ver.into()),
Err(_) => {
// HACK: Leverage `VersionReq` for partial version parsing
let mut version_req = match semver::VersionReq::parse(value) {
Ok(req) => req,
Err(_) if value.contains('-') => {
anyhow::bail!(
"unexpected prerelease field, expected a version like \"1.32\""
)
}
Err(_) if value.contains('+') => {
anyhow::bail!("unexpected build field, expected a version like \"1.32\"")
}
Err(_) => anyhow::bail!("expected a version like \"1.32\""),
Err(_) if value.contains('-') => return Err(ErrorKind::Prerelease.into()),
Err(_) if value.contains('+') => return Err(ErrorKind::BuildMetadata.into()),
Err(_) => return Err(ErrorKind::Unexpected.into()),
};
assert_eq!(version_req.comparators.len(), 1, "guaranteed by is_req");
let comp = version_req.comparators.pop().unwrap();
Expand Down Expand Up @@ -163,6 +157,28 @@ impl<'de> serde::Deserialize<'de> for PartialVersion {
}
}

/// Error parsing a [`PartialVersion`].
#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct PartialVersionError(#[from] ErrorKind);

/// Non-public error kind for [`PartialVersionError`].
#[non_exhaustive]
#[derive(Debug, thiserror::Error)]
enum ErrorKind {
#[error("unexpected version requirement, expected a version like \"1.32\"")]
VersionReq,

#[error("unexpected prerelease field, expected a version like \"1.32\"")]
Prerelease,

#[error("unexpected build field, expected a version like \"1.32\"")]
BuildMetadata,
epage marked this conversation as resolved.
Show resolved Hide resolved

#[error("expected a version like \"1.32\"")]
Unexpected,
}

fn is_req(value: &str) -> bool {
let Some(first) = value.chars().next() else {
return false;
Expand Down
46 changes: 34 additions & 12 deletions crates/cargo-util-schemas/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@ use std::fmt::{self, Display, Write};
use std::path::PathBuf;
use std::str;

use anyhow::Result;
use serde::de::{self, IntoDeserializer as _, Unexpected};
use serde::ser;
use serde::{Deserialize, Serialize};
use serde_untagged::UntaggedEnumVisitor;

use crate::core::PackageIdSpec;
use crate::core::PartialVersion;
use crate::core::PartialVersionError;
use crate::restricted_names;

pub use crate::restricted_names::NameValidationError;

/// This type is used to deserialize `Cargo.toml` files.
#[derive(Debug, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
Expand Down Expand Up @@ -1144,7 +1146,7 @@ macro_rules! str_newtype {
}

impl<'a> std::str::FromStr for $name<String> {
type Err = anyhow::Error;
type Err = restricted_names::NameValidationError;
weihanglo marked this conversation as resolved.
Show resolved Hide resolved

fn from_str(value: &str) -> Result<Self, Self::Err> {
Self::new(value.to_owned())
Expand Down Expand Up @@ -1173,8 +1175,8 @@ str_newtype!(PackageName);

impl<T: AsRef<str>> PackageName<T> {
/// Validated package name
pub fn new(name: T) -> Result<Self> {
restricted_names::validate_package_name(name.as_ref(), "package name", "")?;
pub fn new(name: T) -> Result<Self, NameValidationError> {
restricted_names::validate_package_name(name.as_ref(), "package name")?;
Ok(Self(name))
}
}
Expand All @@ -1195,8 +1197,8 @@ str_newtype!(RegistryName);

impl<T: AsRef<str>> RegistryName<T> {
/// Validated registry name
pub fn new(name: T) -> Result<Self> {
restricted_names::validate_package_name(name.as_ref(), "registry name", "")?;
pub fn new(name: T) -> Result<Self, NameValidationError> {
restricted_names::validate_package_name(name.as_ref(), "registry name")?;
Ok(Self(name))
}
}
Expand All @@ -1205,7 +1207,7 @@ str_newtype!(ProfileName);

impl<T: AsRef<str>> ProfileName<T> {
/// Validated profile name
pub fn new(name: T) -> Result<Self> {
pub fn new(name: T) -> Result<Self, NameValidationError> {
restricted_names::validate_profile_name(name.as_ref())?;
Ok(Self(name))
}
Expand All @@ -1215,7 +1217,7 @@ str_newtype!(FeatureName);

impl<T: AsRef<str>> FeatureName<T> {
/// Validated feature name
pub fn new(name: T) -> Result<Self> {
pub fn new(name: T) -> Result<Self, NameValidationError> {
restricted_names::validate_feature_name(name.as_ref())?;
Ok(Self(name))
}
Expand Down Expand Up @@ -1334,15 +1336,16 @@ impl std::ops::Deref for RustVersion {
}

impl std::str::FromStr for RustVersion {
type Err = anyhow::Error;
type Err = RustVersionError;

fn from_str(value: &str) -> Result<Self, Self::Err> {
let partial = value.parse::<PartialVersion>()?;
let partial = value.parse::<PartialVersion>();
let partial = partial.map_err(RustVersionErrorKind::PartialVersion)?;
if partial.pre.is_some() {
anyhow::bail!("unexpected prerelease field, expected a version like \"1.32\"")
return Err(RustVersionErrorKind::Prerelease.into());
}
if partial.build.is_some() {
anyhow::bail!("unexpected prerelease field, expected a version like \"1.32\"")
return Err(RustVersionErrorKind::BuildMetadata.into());
}
Ok(Self(partial))
}
Expand All @@ -1366,6 +1369,25 @@ impl Display for RustVersion {
}
}

/// Error parsing a [`RustVersion`].
#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct RustVersionError(#[from] RustVersionErrorKind);

/// Non-public error kind for [`RustVersionError`].
#[non_exhaustive]
#[derive(Debug, thiserror::Error)]
enum RustVersionErrorKind {
#[error("unexpected prerelease field, expected a version like \"1.32\"")]
Prerelease,

#[error("unexpected build field, expected a version like \"1.32\"")]
BuildMetadata,

#[error(transparent)]
PartialVersion(#[from] PartialVersionError),
}

#[derive(Copy, Clone, Debug)]
pub struct InvalidCargoFeatures {}

Expand Down
Loading