Skip to content

Commit

Permalink
AssetPath source parse fix (#11543)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
thepackett committed Jan 26, 2024
1 parent 7ae36a9 commit 182c21d
Showing 1 changed file with 85 additions and 22 deletions.
107 changes: 85 additions & 22 deletions crates/bevy_asset/src/path.rs
Expand Up @@ -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,
}
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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)));

Expand All @@ -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]
Expand Down

0 comments on commit 182c21d

Please sign in to comment.