Skip to content

Commit

Permalink
refactor(ic-asset): Use concrete types rather than anyhow (#3191)
Browse files Browse the repository at this point in the history
Replace use of `anyhow` in the `ic-asset` crate with concrete error types.

Fixes https://dfinity.atlassian.net/browse/SDK-869


Regarding this note from  https://mmapped.blog/posts/12-rust-error-handling.html, I gave it a shot. So far I think it turned out better than I expected.
> Feel free to introduce distinct error types for each function you implement. I am still looking for Rust code that went overboard with distinct error types.
  • Loading branch information
ericswanson-dfinity authored Jun 26, 2023
1 parent 891055d commit 1a1fd8f
Show file tree
Hide file tree
Showing 39 changed files with 474 additions and 174 deletions.
3 changes: 2 additions & 1 deletion Cargo.lock

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

3 changes: 2 additions & 1 deletion src/canisters/frontend/ic-asset/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ categories = ["api-bindings", "data-structures"]
keywords = ["internet-computer", "assets", "icp", "dfinity"]

[dependencies]
anyhow.workspace = true
backoff.workspace = true
candid = { workspace = true }
derivative = "2.2.0"
dfx-core = { path = "../../../dfx-core" }
flate2.workspace = true
futures.workspace = true
futures-intrusive = "0.4.0"
Expand All @@ -32,6 +32,7 @@ serde_bytes.workspace = true
serde_json.workspace = true
sha2.workspace = true
slog = { workspace = true, features = ["max_level_trace"] }
thiserror.workspace = true
tokio.workspace = true
walkdir.workspace = true

Expand Down
123 changes: 57 additions & 66 deletions src/canisters/frontend/ic-asset/src/asset/config.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use anyhow::{bail, Context};
use crate::error::get_asset_config::GetAssetConfigError;
use crate::error::get_asset_config::GetAssetConfigError::{AssetConfigNotFound, InvalidPath};
use crate::error::load_config::AssetLoadConfigError;
use crate::error::load_config::AssetLoadConfigError::{LoadRuleFailed, MalformedAssetConfigFile};

use derivative::Derivative;
use globset::GlobMatcher;
use serde::{Deserialize, Serialize};
use std::collections::BTreeMap;
use std::{
collections::HashMap,
fs,
path::{Path, PathBuf},
sync::{Arc, Mutex},
};
Expand Down Expand Up @@ -106,9 +109,9 @@ struct AssetConfigTreeNode {

impl AssetSourceDirectoryConfiguration {
/// Constructs config tree for assets directory.
pub fn load(root_dir: &Path) -> anyhow::Result<Self> {
pub fn load(root_dir: &Path) -> Result<Self, AssetLoadConfigError> {
if !root_dir.has_root() {
bail!("root_dir paramenter is expected to be canonical path")
return Err(AssetLoadConfigError::InvalidRootDir(root_dir.to_path_buf()));
}
let mut config_map = HashMap::new();
AssetConfigTreeNode::load(None, root_dir, &mut config_map)?;
Expand All @@ -117,22 +120,15 @@ impl AssetSourceDirectoryConfiguration {
}

/// Fetches the configuration for the asset.
pub fn get_asset_config(&mut self, canonical_path: &Path) -> anyhow::Result<AssetConfig> {
let parent_dir = canonical_path.parent().with_context(|| {
format!(
"unable to get the parent directory for asset path: {:?}",
canonical_path
)
})?;
pub fn get_asset_config(
&mut self,
canonical_path: &Path,
) -> Result<AssetConfig, GetAssetConfigError> {
let parent_dir = dfx_core::fs::parent(canonical_path).map_err(InvalidPath)?;
Ok(self
.config_map
.get(parent_dir)
.with_context(|| {
format!(
"unable to find asset config for following path: {:?}",
parent_dir
)
})?
.get(&parent_dir)
.ok_or_else(|| AssetConfigNotFound(parent_dir.to_path_buf()))?
.lock()
.unwrap()
.get_config(canonical_path))
Expand Down Expand Up @@ -175,38 +171,34 @@ impl AssetSourceDirectoryConfiguration {

impl AssetConfigTreeNode {
/// Constructs config tree for assets directory in a recursive fashion.
fn load(parent: Option<ConfigNode>, dir: &Path, configs: &mut ConfigMap) -> anyhow::Result<()> {
let config_path: Option<PathBuf>;
match (
fn load(
parent: Option<ConfigNode>,
dir: &Path,
configs: &mut ConfigMap,
) -> Result<(), AssetLoadConfigError> {
let config_path = match (
dir.join(ASSETS_CONFIG_FILENAME_JSON).exists(),
dir.join(ASSETS_CONFIG_FILENAME_JSON5).exists(),
) {
(true, true) => {
return Err(anyhow::anyhow!(
"both {} and {} files exist in the same directory (dir = {:?})",
ASSETS_CONFIG_FILENAME_JSON,
ASSETS_CONFIG_FILENAME_JSON5,
dir
))
return Err(AssetLoadConfigError::MultipleConfigurationFiles(
dir.to_path_buf(),
));
}
(true, false) => config_path = Some(dir.join(ASSETS_CONFIG_FILENAME_JSON)),
(false, true) => config_path = Some(dir.join(ASSETS_CONFIG_FILENAME_JSON5)),
(false, false) => config_path = None,
}
(true, false) => Some(dir.join(ASSETS_CONFIG_FILENAME_JSON)),
(false, true) => Some(dir.join(ASSETS_CONFIG_FILENAME_JSON5)),
(false, false) => None,
};
let mut rules = vec![];
if let Some(config_path) = config_path {
let content = fs::read_to_string(&config_path).with_context(|| {
format!("unable to read config file: {}", config_path.display())
})?;
let content = dfx_core::fs::read_to_string(&config_path)?;

let interim_rules: Vec<rule_utils::InterimAssetConfigRule> = json5::from_str(&content)
.with_context(|| {
format!(
"malformed JSON asset config file: {}",
config_path.display()
)
})?;
.map_err(|e| MalformedAssetConfigFile(config_path.to_path_buf(), e))?;
for interim_rule in interim_rules {
rules.push(AssetConfigRule::from_interim(interim_rule, dir)?);
let rule = AssetConfigRule::from_interim(interim_rule, dir)
.map_err(|e| LoadRuleFailed(config_path.to_path_buf(), e))?;
rules.push(rule);
}
}

Expand All @@ -220,8 +212,7 @@ impl AssetConfigTreeNode {
};

configs.insert(dir.to_path_buf(), parent_ref.clone());
for f in std::fs::read_dir(dir)
.with_context(|| format!("Unable to read directory {}", &dir.display()))?
for f in dfx_core::fs::read_dir(dir)?
.filter_map(|x| x.ok())
.filter(|x| x.file_type().map_or_else(|_e| false, |ft| ft.is_dir()))
{
Expand Down Expand Up @@ -278,7 +269,7 @@ impl AssetConfig {
/// and pretty-printing of the `AssetConfigRule` data structure.
mod rule_utils {
use super::{AssetConfig, AssetConfigRule, CacheConfig, HeadersConfig, Maybe};
use anyhow::Context;
use crate::error::load_rule::LoadRuleError;
use globset::{Glob, GlobMatcher};
use serde::{Deserialize, Serializer};
use serde_json::Value;
Expand Down Expand Up @@ -373,23 +364,20 @@ mod rule_utils {
allow_raw_access,
}: InterimAssetConfigRule,
config_file_parent_dir: &Path,
) -> anyhow::Result<Self> {
let glob = Glob::new(
config_file_parent_dir
.join(&r#match)
.to_str()
.with_context(|| {
format!(
"cannot combine {} and {} into a string (to be later used as a glob pattern)",
config_file_parent_dir.display(),
r#match
)
})?,
)
.with_context(|| format!("{} is not a valid glob pattern", r#match))?.compile_matcher();
) -> Result<Self, LoadRuleError> {
let glob = config_file_parent_dir.join(&r#match);
let glob = glob.to_str().ok_or_else(|| {
LoadRuleError::FormGlobPatternFailed(
config_file_parent_dir.to_path_buf(),
r#match.clone(),
)
})?;
let matcher = Glob::new(glob)
.map_err(|e| LoadRuleError::InvalidGlobPattern(r#match, e))?
.compile_matcher();

Ok(Self {
r#match: glob,
r#match: matcher,
cache,
headers,
ignore,
Expand Down Expand Up @@ -807,11 +795,12 @@ mod with_tempdir {
assert_eq!(
assets_config.err().unwrap().to_string(),
format!(
"malformed JSON asset config file: {}",
"Malformed JSON asset config file '{}': {}",
assets_dir
.join(ASSETS_CONFIG_FILENAME_JSON)
.to_str()
.unwrap()
.unwrap(),
"--> 1:1\n |\n1 | \n | ^---\n |\n = expected array, boolean, null, number, object, or string"
)
);
}
Expand All @@ -825,11 +814,12 @@ mod with_tempdir {
assert_eq!(
assets_config.err().unwrap().to_string(),
format!(
"malformed JSON asset config file: {}",
"Malformed JSON asset config file '{}': {}",
assets_dir
.join(ASSETS_CONFIG_FILENAME_JSON)
.to_str()
.unwrap()
.unwrap(),
"--> 1:5\n |\n1 | [[[{{{\n | ^---\n |\n = expected identifier or string"
)
);
}
Expand All @@ -849,11 +839,12 @@ mod with_tempdir {
assert_eq!(
assets_config.err().unwrap().to_string(),
format!(
"malformed JSON asset config file: {}",
"Malformed JSON asset config file '{}': {}",
assets_dir
.join(ASSETS_CONFIG_FILENAME_JSON)
.to_str()
.unwrap()
.unwrap(),
"--> 2:19\n |\n2 | {\"match\": \"{{{\\\\\\\", \"cache\": {\"max_age\": 900}},\n | ^---\n |\n = expected boolean or null"
)
);
}
Expand Down Expand Up @@ -894,7 +885,7 @@ mod with_tempdir {
assert_eq!(
assets_config.err().unwrap().to_string(),
format!(
"unable to read config file: {}",
"Failed to read {} as string: Permission denied (os error 13)",
assets_dir
.join(ASSETS_CONFIG_FILENAME_JSON)
.as_path()
Expand Down
9 changes: 5 additions & 4 deletions src/canisters/frontend/ic-asset/src/asset/content.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::asset::content_encoder::ContentEncoder;
use dfx_core::error::fs::FsError;

use flate2::write::GzEncoder;
use flate2::Compression;
Expand All @@ -13,8 +14,8 @@ pub(crate) struct Content {
}

impl Content {
pub fn load(path: &Path) -> anyhow::Result<Content> {
let data = std::fs::read(path)?;
pub fn load(path: &Path) -> Result<Content, FsError> {
let data = dfx_core::fs::read(path)?;

// todo: check contents if mime_guess fails https://github.com/dfinity/sdk/issues/1594
let media_type = mime_guess::from_path(path)
Expand All @@ -24,13 +25,13 @@ impl Content {
Ok(Content { data, media_type })
}

pub fn encode(&self, encoder: &ContentEncoder) -> anyhow::Result<Content> {
pub fn encode(&self, encoder: &ContentEncoder) -> Result<Content, std::io::Error> {
match encoder {
ContentEncoder::Gzip => self.to_gzip(),
}
}

pub fn to_gzip(&self) -> anyhow::Result<Content> {
pub fn to_gzip(&self) -> Result<Content, std::io::Error> {
let mut e = GzEncoder::new(Vec::new(), Compression::default());
e.write_all(&self.data)?;
let data = e.finish()?;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#[derive(Clone, Debug)]
pub enum ContentEncoder {
Gzip,
}
Expand Down
Loading

0 comments on commit 1a1fd8f

Please sign in to comment.