From 4ea0f42af787bfb9bf51452afb031344128a49dd Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 15:23:06 -0600 Subject: [PATCH] AssetPath source parse fix (#11543) # Objective Fixes #11533 When `AssetPath`s are created from a string type, they are parsed into an `AssetSource`, a `Path`, and a `Label`. The current method of parsing has some unnecessary quirks: - The presence of a `:` character is assumed to be the start of an asset source indicator. - This is not necessarily true. There are valid uses of a `:` character in an asset path, for example an http source's port such as `localhost:80`. - If there are multiple instances of `://`, the last one is assumed to be the asset source deliminator. - This has some unexpected behavior. Even in a fully formed path, such as `http://localhost:80`, the `:` between `localhost` and `80` is assumed to be the start of an asset source, causing an error since it does not form the full sequence `://`. ## Solution Changes the `AssetPath`'s `parse_internal` method to be more permissive. - Only the exact sequence `://` is taken to be the asset source deliminator, and only the first one if there are multiple. - As a consequence, it is no longer possible to detect a malformed asset source deliminator, and so the corresponding error was removed. --- crates/bevy_asset/src/path.rs | 107 +++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 22 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 68991316913db9..77039880fb9cea 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -78,12 +78,19 @@ impl<'a> Display for AssetPath<'a> { } } +/// An error that occurs when parsing a string type to create an [`AssetPath`] fails, such as during [`AssetPath::parse`] or [`AssetPath::from<'static str>`]. #[derive(Error, Debug, PartialEq, Eq)] pub enum ParseAssetPathError { - #[error("Asset source must be followed by '://'")] + /// Error that occurs when the [`AssetPath::source`] section of a path string contains the [`AssetPath::label`] delimiter `#`. E.g. `bad#source://file.test`. + #[error("Asset source must not contain a `#` character")] InvalidSourceSyntax, + /// Error that occurs when the [`AssetPath::label`] section of a path string contains the [`AssetPath::source`] delimiter `://`. E.g. `source://file.test#bad://label`. + #[error("Asset label must not contain a `://` substring")] + InvalidLabelSyntax, + /// Error that occurs when a path string has an [`AssetPath::source`] delimiter `://` with no characters preceeding it. E.g. `://file.test`. #[error("Asset source must be at least one character. Either specify the source before the '://' or remove the `://`")] MissingSource, + /// Error that occurs when a path string has an [`AssetPath::label`] delimiter `#` with no characters succeeding it. E.g. `file.test#` #[error("Asset label must be at least one character. Either specify the label after the '#' or remove the '#'")] MissingLabel, } @@ -126,40 +133,70 @@ impl<'a> AssetPath<'a> { }) } + // Attempts to Parse a &str into an `AssetPath`'s `AssetPath::source`, `AssetPath::path`, and `AssetPath::label` components. fn parse_internal( asset_path: &str, ) -> Result<(Option<&str>, &Path, Option<&str>), ParseAssetPathError> { - let mut chars = asset_path.char_indices(); + let chars = asset_path.char_indices(); let mut source_range = None; let mut path_range = 0..asset_path.len(); let mut label_range = None; - while let Some((index, char)) = chars.next() { + + // Loop through the characters of the passed in &str to accomplish the following: + // 1. Seach for the first instance of the `://` substring. If the `://` substring is found, + // store the range of indices representing everything before the `://` substring as the `source_range`. + // 2. Seach for the last instance of the `#` character. If the `#` character is found, + // store the range of indices representing everything after the `#` character as the `label_range` + // 3. Set the `path_range` to be everything in between the `source_range` and `label_range`, + // excluding the `://` substring and `#` character. + // 4. Verify that there are no `#` characters in the `AssetPath::source` and no `://` substrings in the `AssetPath::label` + let mut source_delimiter_chars_matched = 0; + let mut last_found_source_index = 0; + for (index, char) in chars { match char { ':' => { - let (_, char) = chars - .next() - .ok_or(ParseAssetPathError::InvalidSourceSyntax)?; - if char != '/' { - return Err(ParseAssetPathError::InvalidSourceSyntax); - } - let (index, char) = chars - .next() - .ok_or(ParseAssetPathError::InvalidSourceSyntax)?; - if char != '/' { - return Err(ParseAssetPathError::InvalidSourceSyntax); + source_delimiter_chars_matched = 1; + } + '/' => { + match source_delimiter_chars_matched { + 1 => { + source_delimiter_chars_matched = 2; + } + 2 => { + // If we haven't found our first `AssetPath::source` yet, check to make sure it is valid and then store it. + if source_range.is_none() { + // If the `AssetPath::source` containes a `#` character, it is invalid. + if label_range.is_some() { + return Err(ParseAssetPathError::InvalidSourceSyntax); + } + source_range = Some(0..index - 2); + path_range.start = index + 1; + } + last_found_source_index = index - 2; + source_delimiter_chars_matched = 0; + } + _ => {} } - source_range = Some(0..index - 2); - path_range.start = index + 1; } '#' => { path_range.end = index; label_range = Some(index + 1..asset_path.len()); - break; + source_delimiter_chars_matched = 0; + } + _ => { + source_delimiter_chars_matched = 0; } - _ => {} } } - + // If we found an `AssetPath::label` + if let Some(range) = label_range.clone() { + // If the `AssetPath::label` contained a `://` substring, it is invalid. + if range.start <= last_found_source_index { + return Err(ParseAssetPathError::InvalidLabelSyntax); + } + } + // Try to parse the range of indices that represents the `AssetPath::source` portion of the `AssetPath` to make sure it is not empty. + // This would be the case if the input &str was something like `://some/file.test` let source = match source_range { Some(source_range) => { if source_range.is_empty() { @@ -169,6 +206,8 @@ impl<'a> AssetPath<'a> { } None => None, }; + // Try to parse the range of indices that represents the `AssetPath::label` portion of the `AssetPath` to make sure it is not empty. + // This would be the case if the input &str was something like `some/file.test#`. let label = match label_range { Some(label_range) => { if label_range.is_empty() { @@ -700,6 +739,33 @@ mod tests { Ok((Some("http"), Path::new("a/b.test"), Some("Foo"))) ); + let result = AssetPath::parse_internal("localhost:80/b.test"); + assert_eq!(result, Ok((None, Path::new("localhost:80/b.test"), None))); + + let result = AssetPath::parse_internal("http://localhost:80/b.test"); + assert_eq!( + result, + Ok((Some("http"), Path::new("localhost:80/b.test"), None)) + ); + + let result = AssetPath::parse_internal("http://localhost:80/b.test#Foo"); + assert_eq!( + result, + Ok((Some("http"), Path::new("localhost:80/b.test"), Some("Foo"))) + ); + + let result = AssetPath::parse_internal("#insource://a/b.test"); + assert_eq!(result, Err(crate::ParseAssetPathError::InvalidSourceSyntax)); + + let result = AssetPath::parse_internal("source://a/b.test#://inlabel"); + assert_eq!(result, Err(crate::ParseAssetPathError::InvalidLabelSyntax)); + + let result = AssetPath::parse_internal("#insource://a/b.test#://inlabel"); + assert!( + result == Err(crate::ParseAssetPathError::InvalidSourceSyntax) + || result == Err(crate::ParseAssetPathError::InvalidLabelSyntax) + ); + let result = AssetPath::parse_internal("http://"); assert_eq!(result, Ok((Some("http"), Path::new(""), None))); @@ -708,9 +774,6 @@ mod tests { let result = AssetPath::parse_internal("a/b.test#"); assert_eq!(result, Err(crate::ParseAssetPathError::MissingLabel)); - - let result = AssetPath::parse_internal("http:/"); - assert_eq!(result, Err(crate::ParseAssetPathError::InvalidSourceSyntax)); } #[test]