From fb35238499e93ba0a17bbd95c16f432cbf0bce6b Mon Sep 17 00:00:00 2001 From: thepackett Date: Thu, 25 Jan 2024 19:40:35 -0600 Subject: [PATCH 1/9] Changed AssetPath parsing behavior. --- crates/bevy_asset/src/path.rs | 42 ++++++++++++----------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 68991316913db..0bb14e5c20d78 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -129,36 +129,22 @@ impl<'a> AssetPath<'a> { fn parse_internal( asset_path: &str, ) -> Result<(Option<&str>, &Path, Option<&str>), ParseAssetPathError> { - let mut 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() { - 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_range = Some(0..index - 2); - path_range.start = index + 1; - } - '#' => { - path_range.end = index; - label_range = Some(index + 1..asset_path.len()); - break; - } - _ => {} + + let source_range = match asset_path.find("://") { + Some(index) => { + path_range.start = index + 3; + Some(0..index) } - } + None => None, + }; + let label_range = match asset_path.rfind('#') { + Some(index) => { + path_range.end = index; + Some(index + 1..asset_path.len()) + } + None => None, + }; let source = match source_range { Some(source_range) => { From 9bd0bbd6b66396796819985dd24122f81a211665 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 05:41:29 -0600 Subject: [PATCH 2/9] Removed redundant error type, added new test. --- crates/bevy_asset/src/path.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 0bb14e5c20d78..8c220b56a176c 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -686,6 +686,12 @@ mod tests { Ok((Some("http"), Path::new("a/b.test"), Some("Foo"))) ); + 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://"); assert_eq!(result, Ok((Some("http"), Path::new(""), None))); @@ -694,9 +700,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] From 544d53085e57c15e599626c6d9a31c175846fdf4 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 08:00:03 -0600 Subject: [PATCH 3/9] Actually remove ParseAssetPathError variant for InvalidSourceSyntax. --- crates/bevy_asset/src/path.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 8c220b56a176c..a45064849908b 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -80,8 +80,6 @@ impl<'a> Display for AssetPath<'a> { #[derive(Error, Debug, PartialEq, Eq)] pub enum ParseAssetPathError { - #[error("Asset source must be followed by '://'")] - InvalidSourceSyntax, #[error("Asset source must be at least one character. Either specify the source before the '://' or remove the `://`")] MissingSource, #[error("Asset label must be at least one character. Either specify the label after the '#' or remove the '#'")] From d655c4bd47b3761cafda3e7b6201a4b5b205f493 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 11:40:23 -0600 Subject: [PATCH 4/9] Changed to one pass algorithm. Added comments. Discovered new error case and added appropriate error types and tests. --- crates/bevy_asset/src/path.rs | 95 +++++++++++++++++++++++++++++------ 1 file changed, 81 insertions(+), 14 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index a45064849908b..37d37eab359a5 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -80,6 +80,10 @@ impl<'a> Display for AssetPath<'a> { #[derive(Error, Debug, PartialEq, Eq)] pub enum ParseAssetPathError { + #[error("Asset source must not contain a `#` character")] + InvalidSourceSyntax, + #[error("Asset label must not contain a `://` substring")] + InvalidLabelSyntax, #[error("Asset source must be at least one character. Either specify the source before the '://' or remove the `://`")] MissingSource, #[error("Asset label must be at least one character. Either specify the label after the '#' or remove the '#'")] @@ -124,26 +128,69 @@ 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 mut source_range = None; let mut path_range = 0..asset_path.len(); - - let source_range = match asset_path.find("://") { - Some(index) => { - path_range.start = index + 3; - Some(0..index) + let mut label_range = None; + + // 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; + while let Some((index, char)) = chars.next() { + match char { + ':' => { + source_delimiter_chars_matched = 1; + } + '/' => { + match source_delimiter_chars_matched { + 1 => { + source_delimiter_chars_matched = 2; + } + 2 => { + if source_range == None { + // If the `AssetPath::source` contained a `#` character, it is invalid. + if label_range != None { + 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; + } + _ => {} + } + } + '#' => { + path_range.end = index; + label_range = Some(index + 1..asset_path.len()); + source_delimiter_chars_matched = 0; + } + _ => { + source_delimiter_chars_matched = 0; + } } - None => None, - }; - let label_range = match asset_path.rfind('#') { - Some(index) => { - path_range.end = index; - Some(index + 1..asset_path.len()) + } + // 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); } - None => None, - }; - + } + // Try to parse the range of indices that represents the `AssetPath::source` portion of the `AssetPath`. + // The only invalid `AssetPath::source` is an empty &str, representing an input such as `://some/file.test` let source = match source_range { Some(source_range) => { if source_range.is_empty() { @@ -153,6 +200,8 @@ impl<'a> AssetPath<'a> { } None => None, }; + // Try to parse the range of indices that represents the `AssetPath::label` portion of the `AssetPath`. + // The only invalid `AssetPath::label` is an empty &str, representing an input such as `some/file.test#`. let label = match label_range { Some(label_range) => { if label_range.is_empty() { @@ -684,12 +733,30 @@ 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))); From fe6669552a4ef2d285da8ba6ba56257d27f36355 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 11:41:21 -0600 Subject: [PATCH 5/9] Cargo format --- crates/bevy_asset/src/path.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 37d37eab359a5..96fdc3e76cc14 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -755,7 +755,10 @@ mod tests { 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)); + 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))); From 7694302428ecf3f2c40b418a41e94bf2cf1c85c7 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 11:48:20 -0600 Subject: [PATCH 6/9] Added additional comments. --- crates/bevy_asset/src/path.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 96fdc3e76cc14..1ff5ea901f656 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -158,8 +158,9 @@ impl<'a> AssetPath<'a> { 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 == None { - // If the `AssetPath::source` contained a `#` character, it is invalid. + // If the `AssetPath::source` containes a `#` character, it is invalid. if label_range != None { return Err(ParseAssetPathError::InvalidSourceSyntax); } @@ -189,8 +190,8 @@ impl<'a> AssetPath<'a> { return Err(ParseAssetPathError::InvalidLabelSyntax); } } - // Try to parse the range of indices that represents the `AssetPath::source` portion of the `AssetPath`. - // The only invalid `AssetPath::source` is an empty &str, representing an input such as `://some/file.test` + // 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() { @@ -200,8 +201,8 @@ impl<'a> AssetPath<'a> { } None => None, }; - // Try to parse the range of indices that represents the `AssetPath::label` portion of the `AssetPath`. - // The only invalid `AssetPath::label` is an empty &str, representing an input such as `some/file.test#`. + // 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() { From 89262cb36a414267c2e41493f833b087f8b8af3b Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 12:05:05 -0600 Subject: [PATCH 7/9] Make CI Happy --- crates/bevy_asset/src/path.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 1ff5ea901f656..e7a928dec8ed4 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -147,7 +147,7 @@ impl<'a> AssetPath<'a> { // 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; - while let Some((index, char)) = chars.next() { + for (index, char) in chars { match char { ':' => { source_delimiter_chars_matched = 1; @@ -159,9 +159,9 @@ impl<'a> AssetPath<'a> { } 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 == None { + if source_range.is_none() { // If the `AssetPath::source` containes a `#` character, it is invalid. - if label_range != None { + if label_range.is_some() { return Err(ParseAssetPathError::InvalidSourceSyntax); } source_range = Some(0..index - 2); From dbabbe4a1f4e2879ca5b971c71e2760568e5db5f Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 12:07:46 -0600 Subject: [PATCH 8/9] Remove unused mut --- crates/bevy_asset/src/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index e7a928dec8ed4..f844a76321cfd 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -132,7 +132,7 @@ impl<'a> AssetPath<'a> { 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; From 3db15256b39545869d45bb221d858b880d056c30 Mon Sep 17 00:00:00 2001 From: thepackett Date: Fri, 26 Jan 2024 15:08:26 -0600 Subject: [PATCH 9/9] Added ParseAssetPathError documentation. --- crates/bevy_asset/src/path.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index f844a76321cfd..77039880fb9ce 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -78,14 +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 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, }