From 6c73dbcda13708a18f77167fb4e474af60328438 Mon Sep 17 00:00:00 2001 From: Robert Bastian Date: Fri, 14 Jul 2023 14:59:02 +0200 Subject: [PATCH 1/2] errs --- provider/adapters/src/fork/by_error.rs | 4 +-- provider/adapters/src/fork/predicates.rs | 7 +++++ provider/fs/Cargo.toml | 2 +- provider/fs/src/export/fs_exporter.rs | 37 +++++++++++++++++------- provider/fs/src/fs_data_provider.rs | 18 +++++++----- 5 files changed, 47 insertions(+), 21 deletions(-) diff --git a/provider/adapters/src/fork/by_error.rs b/provider/adapters/src/fork/by_error.rs index aae9967619b..2cde1aaaf74 100644 --- a/provider/adapters/src/fork/by_error.rs +++ b/provider/adapters/src/fork/by_error.rs @@ -234,7 +234,7 @@ where F: ForkByErrorPredicate, { fn supported_locales_for_key(&self, key: DataKey) -> Result, DataError> { - let mut last_error = DataErrorKind::MissingDataKey.with_key(key); + let mut last_error = F::ERROR.with_key(key); for provider in self.providers.iter() { let result = provider.supported_locales_for_key(key); match result { @@ -260,7 +260,7 @@ where key: DataKey, mut from: DataPayload, ) -> Result, (DataPayload, DataError)> { - let mut last_error = DataErrorKind::MissingDataKey.with_key(key); + let mut last_error = F::ERROR.with_key(key); for provider in self.providers.iter() { let result = provider.convert(key, from); match result { diff --git a/provider/adapters/src/fork/predicates.rs b/provider/adapters/src/fork/predicates.rs index 0fefe57043e..c499bb364bf 100644 --- a/provider/adapters/src/fork/predicates.rs +++ b/provider/adapters/src/fork/predicates.rs @@ -10,6 +10,9 @@ use icu_provider::prelude::*; /// /// [`ForkByErrorProvider`]: super::ForkByErrorProvider pub trait ForkByErrorPredicate { + /// The error to return if there are zero providers. + const ERROR: DataErrorKind = DataErrorKind::MissingDataKey; + /// This function is called when a data request fails and there are additional providers /// that could possibly fulfill the request. /// @@ -43,6 +46,8 @@ pub trait ForkByErrorPredicate { pub struct MissingDataKeyPredicate; impl ForkByErrorPredicate for MissingDataKeyPredicate { + const ERROR: DataErrorKind = DataErrorKind::MissingDataKey; + #[inline] fn test(&self, _: DataKey, _: Option, err: DataError) -> bool { matches!( @@ -125,6 +130,8 @@ impl ForkByErrorPredicate for MissingDataKeyPredicate { pub struct MissingLocalePredicate; impl ForkByErrorPredicate for MissingLocalePredicate { + const ERROR: DataErrorKind = DataErrorKind::MissingLocale; + #[inline] fn test(&self, _: DataKey, _: Option, err: DataError) -> bool { matches!( diff --git a/provider/fs/Cargo.toml b/provider/fs/Cargo.toml index 77ee6605ada..c01d4532982 100644 --- a/provider/fs/Cargo.toml +++ b/provider/fs/Cargo.toml @@ -33,7 +33,6 @@ displaydoc = { version = "0.2.3", default-features = false } icu_provider = { version = "1.2.0", path = "../core", features = ["serde", "std"] } serde = { version = "1.0", default-features = false, features = ["derive", "alloc"] } serde-json-core = { version = "0.4", default-features = false, features = ["std"] } -writeable = { version = "0.5.1", path = "../../utils/writeable" } # Dependencies for the export feature bincode = { version = "1.3", optional = true } @@ -48,6 +47,7 @@ icu_benchmark_macros = { path = "../../tools/benchmark/macros" } icu_locid = { path = "../../components/locid", features = ["serde"] } icu_provider = { path = "../core", features = ["deserialize_json", "deserialize_bincode_1", "deserialize_postcard_1", "datagen"] } icu_datagen = { path = "../datagen" } +writeable = { path = "../../utils/writeable" } [features] # Enables the "export" module and FilesystemExporter diff --git a/provider/fs/src/export/fs_exporter.rs b/provider/fs/src/export/fs_exporter.rs index 3b6eaef71e9..ec3d77421e7 100644 --- a/provider/fs/src/export/fs_exporter.rs +++ b/provider/fs/src/export/fs_exporter.rs @@ -7,14 +7,14 @@ use crate::manifest::Manifest; use icu_provider::datagen::*; use icu_provider::prelude::*; use serde::{Deserialize, Serialize}; +use std::fmt::Write; use std::fs; #[allow(deprecated)] // We're using SipHash, which is deprecated, but we want a stable hasher // (we're fine with it not being cryptographically secure since we're just using it to track diffs) use std::hash::{Hasher, SipHasher}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Mutex; -use writeable::Writeable; /// Choices of what to do if [`FilesystemExporter`] tries to write to a pre-existing directory. #[non_exhaustive] @@ -119,15 +119,20 @@ impl DataExporter for FilesystemExporter { locale: &DataLocale, obj: &DataPayload, ) -> Result<(), DataError> { - let mut path_buf = self.root.clone(); - path_buf.push(&*key.write_to_string()); - path_buf.push(&*locale.write_to_string()); - path_buf.set_extension(self.manifest.file_extension); - - if let Some(parent_dir) = path_buf.parent() { - fs::create_dir_all(parent_dir) - .map_err(|e| DataError::from(e).with_path_context(&parent_dir))?; - } + let mut path_buf = self.root.clone().into_os_string(); + write!( + &mut path_buf, + "/{key}/{locale}.{}", + self.manifest.file_extension + ) + .expect("infallible"); + + #[allow(clippy::unwrap_used)] // has parent by construction + let parent_dir = Path::new(&path_buf) + .parent().unwrap(); + + fs::create_dir_all(parent_dir) + .map_err(|e| DataError::from(e).with_path_context(&parent_dir))?; let mut file = HashingFile { file: if self.serializer.is_text_format() { @@ -163,6 +168,16 @@ impl DataExporter for FilesystemExporter { Ok(()) } + fn flush_with_fallback(&self, key: DataKey, _: FallbackMode) -> Result<(), DataError> { + let mut path_buf = self.root.clone().into_os_string(); + write!(&mut path_buf, "/{key}").expect("infallible"); + + fs::create_dir_all(&path_buf) + .map_err(|e| DataError::from(e).with_path_context(&path_buf))?; + + Ok(()) + } + fn close(&mut self) -> Result<(), DataError> { if let Some(fingerprints) = self.fingerprints.as_mut() { let fingerprints = fingerprints.get_mut().expect("poison"); diff --git a/provider/fs/src/fs_data_provider.rs b/provider/fs/src/fs_data_provider.rs index 5d6b279bb1e..fdd7d6ab629 100644 --- a/provider/fs/src/fs_data_provider.rs +++ b/provider/fs/src/fs_data_provider.rs @@ -7,7 +7,8 @@ use icu_provider::prelude::*; use std::fmt::Debug; use std::fs; use std::path::PathBuf; -use writeable::Writeable; +use std::fmt::Write; +use std::path::Path; /// A data provider that reads ICU4X data from a filesystem directory. /// @@ -70,17 +71,20 @@ impl BufferProvider for FsDataProvider { key: DataKey, req: DataRequest, ) -> Result, DataError> { - let mut path_buf = self.root.join(&*key.write_to_string()); - if !path_buf.exists() { + if key.metadata().singleton && !req.locale.is_empty() { + return Err(DataErrorKind::ExtraneousLocale.with_req(key, req)); + } + let mut path = self.root.clone().into_os_string(); + write!(&mut path, "/{key}").expect("infallible"); + if !Path::new(&path).exists() { return Err(DataErrorKind::MissingDataKey.with_req(key, req)); } - path_buf.push(&*req.locale.write_to_string()); - path_buf.set_extension(self.manifest.file_extension); - if !path_buf.exists() { + write!(&mut path, "/{}.{}", req.locale, self.manifest.file_extension).expect("infallible"); + if !Path::new(&path).exists() { return Err(DataErrorKind::MissingLocale.with_req(key, req)); } let buffer = - fs::read(&path_buf).map_err(|e| DataError::from(e).with_path_context(&path_buf))?; + fs::read(&path).map_err(|e| DataError::from(e).with_path_context(&path))?; let mut metadata = DataResponseMetadata::default(); metadata.buffer_format = Some(self.manifest.buffer_format); Ok(DataResponse { From 35655872b5ee58f6122e64ba0ac1e5f0bea2de47 Mon Sep 17 00:00:00 2001 From: Robert Bastian Date: Wed, 19 Jul 2023 17:16:36 +0200 Subject: [PATCH 2/2] fmt --- provider/adapters/src/fork/predicates.rs | 4 ++-- provider/fs/src/export/fs_exporter.rs | 3 +-- provider/fs/src/fs_data_provider.rs | 14 +++++++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/provider/adapters/src/fork/predicates.rs b/provider/adapters/src/fork/predicates.rs index c499bb364bf..2905d63bc77 100644 --- a/provider/adapters/src/fork/predicates.rs +++ b/provider/adapters/src/fork/predicates.rs @@ -10,7 +10,7 @@ use icu_provider::prelude::*; /// /// [`ForkByErrorProvider`]: super::ForkByErrorProvider pub trait ForkByErrorPredicate { - /// The error to return if there are zero providers. + /// The error to return if there are zero providers. const ERROR: DataErrorKind = DataErrorKind::MissingDataKey; /// This function is called when a data request fails and there are additional providers @@ -131,7 +131,7 @@ pub struct MissingLocalePredicate; impl ForkByErrorPredicate for MissingLocalePredicate { const ERROR: DataErrorKind = DataErrorKind::MissingLocale; - + #[inline] fn test(&self, _: DataKey, _: Option, err: DataError) -> bool { matches!( diff --git a/provider/fs/src/export/fs_exporter.rs b/provider/fs/src/export/fs_exporter.rs index ec3d77421e7..6037805fe17 100644 --- a/provider/fs/src/export/fs_exporter.rs +++ b/provider/fs/src/export/fs_exporter.rs @@ -128,8 +128,7 @@ impl DataExporter for FilesystemExporter { .expect("infallible"); #[allow(clippy::unwrap_used)] // has parent by construction - let parent_dir = Path::new(&path_buf) - .parent().unwrap(); + let parent_dir = Path::new(&path_buf).parent().unwrap(); fs::create_dir_all(parent_dir) .map_err(|e| DataError::from(e).with_path_context(&parent_dir))?; diff --git a/provider/fs/src/fs_data_provider.rs b/provider/fs/src/fs_data_provider.rs index fdd7d6ab629..b300fde7f83 100644 --- a/provider/fs/src/fs_data_provider.rs +++ b/provider/fs/src/fs_data_provider.rs @@ -5,10 +5,10 @@ use crate::manifest::Manifest; use icu_provider::prelude::*; use std::fmt::Debug; -use std::fs; -use std::path::PathBuf; use std::fmt::Write; +use std::fs; use std::path::Path; +use std::path::PathBuf; /// A data provider that reads ICU4X data from a filesystem directory. /// @@ -79,12 +79,16 @@ impl BufferProvider for FsDataProvider { if !Path::new(&path).exists() { return Err(DataErrorKind::MissingDataKey.with_req(key, req)); } - write!(&mut path, "/{}.{}", req.locale, self.manifest.file_extension).expect("infallible"); + write!( + &mut path, + "/{}.{}", + req.locale, self.manifest.file_extension + ) + .expect("infallible"); if !Path::new(&path).exists() { return Err(DataErrorKind::MissingLocale.with_req(key, req)); } - let buffer = - fs::read(&path).map_err(|e| DataError::from(e).with_path_context(&path))?; + let buffer = fs::read(&path).map_err(|e| DataError::from(e).with_path_context(&path))?; let mut metadata = DataResponseMetadata::default(); metadata.buffer_format = Some(self.manifest.buffer_format); Ok(DataResponse {