-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Non existent path causes panic instead of Error result #819
Comments
That's not good, I'll fix it today unless someone beats me to it
…On Wed, 15 Mar 2023, 02:23 chupaty, ***@***.***> wrote:
New versions of tera panic instead of returning Error when passed an
invalid glob path. For example:
rsgen-avro
<https://github.com/lerouxrgd/rsgen-avro/blob/9e3eb0bfd0f7ba7a583bb7f3166fd67c41f4e708/src/templates.rs#L338>
let mut tera = Tera::new("/dev/null/*")?;
Problem caused by use of unwrap() instead of ? operator, for example:
tera.rs
<https://github.com/Keats/tera/blob/226d0108cdb64c8d056e46c5c9a67a4a4e8549ea/src/tera.rs#L135>
let parent_dir = std::fs::canonicalize(parent_dir).unwrap();
—
Reply to this email directly, view it on GitHub
<#819>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFGDI2UPDC2CXBK4QZ25KLW4EKXVANCNFSM6AAAAAAV3FAIN4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Your fix caused a regression in behaviour (well to be fair, the behaviour regression was already present in #799). This feature was useful, because in zola it was possible to remove the Now with this change, this is no longer possible and zola always requires the To me this change doesn't make any sense, because it is inconsistent:
In my opinion both cases should have the same behaviour, i.e. both cases should expand to the empty set, and not return an error just because an internal implementation detail ( Please see the following patch for a simple fix, that will return to the old behaviour but still with the fix from #799 present. If you agree, I can also make a PR. diff --git a/src/tera.rs b/src/tera.rs
index bf29f6d..1aece99 100644
--- a/src/tera.rs
+++ b/src/tera.rs
@@ -132,7 +132,10 @@ impl Tera {
// See https://github.com/Keats/tera/issues/574 for the Tera discussion
// and https://github.com/Gilnaa/globwalk/issues/28 for the upstream issue.
let (parent_dir, glob_end) = glob.split_at(glob.find('*').unwrap());
- let parent_dir = std::fs::canonicalize(parent_dir)?;
+ let parent_dir = match std::fs::canonicalize(parent_dir) {
+ Ok(d) => d,
+ Err(_) => std::path::PathBuf::from(parent_dir),
+ };
let dir = parent_dir.join(glob_end).into_os_string().into_string().unwrap();
// We are parsing all the templates on instantiation
@@ -1225,6 +1228,6 @@ mod tests {
fn doesnt_panic_on_invalid_glob() {
let tera = Tera::new("\\dev/null/*");
println!("{:?}", tera);
- assert!(tera.is_err());
+ assert!(tera.is_ok());
}
} Otherwise if you insist this is the correct behaviour in tera, this can also be fixed from zola's side. |
I think it makes sense and is consistent for me to return an error if a path doesn't exist? |
Your opinion is perfectly reasonable, it just doesn't match what most other languages do. In Bash it can be controlled with the Maybe it's better to fix the problem in zola. |
As mentioned in Keats/tera#819, there was a regression in how non-existent glob paths are handled. Previously, invalid globs would be expanded to the empty set, but now they lead to an error. Since this implementation just wants a bogus template directory, we just load the `Tera::default()` template. Signed off by: Nick Groszewski <[email protected]>
As mentioned in Keats/tera#819, there was a regression in how non-existent glob paths are handled. Previously, invalid globs would be expanded to the empty set, but now they lead to an error. Since this implementation just wants a bogus template directory, we just load the `Tera::default()` template. This should address google#545 Signed off by: Nick Groszewski <[email protected]>
Regression was introduced in 9479c28. Since that commit providing an invalid glob to tera will make it fail to create an instance, first (in that commit) by making it panic and since 1f95878 by making it return an error. While returning an error is not entirely unlogical, it doesn't match what most languages do with invalid globs. - Bash will by default return an empty set on invalid globs, as the `failglob` option is off by default - Python will likewise return the empty set instead of throwing an exception, when doing something like `glob.glob("/dev/null/*")` - Rust's `globwalk` will also not error, but return an empty set In fact, we use globwalk in tera and the only reason we panic is by accident, because `std::fs::canonicalize()` checks the path. It is better to match other language's glob behaviour, therefore we resort back to the original path if `canonicalize()` fails to return the empty set again when encountering an invalid glob. Even more so, since this caused a lot of regressions already, including in zola. Fixes getzola/zola#2150 ref: Keats#819 ref: Keats#820 ref: Keats#799
Regression was introduced in 9479c28. Since that commit, providing an invalid glob to tera will make it fail to create an instance, first (in that commit) by making it panic and since 1f95878 by making it return an error. While returning an error is not entirely unlogical, it doesn't match what most languages do with invalid globs. - Bash will by default return an empty set on invalid globs, as the `failglob` option is off by default - Python will likewise return the empty set instead of throwing an exception, when doing something like `glob.glob("/dev/null/*")` - Rust's `globwalk` will also not error, but return an empty set In fact, we use globwalk in tera and the only reason we panic is by accident, because `std::fs::canonicalize()` checks the path. It is better to match other language's glob behaviour, therefore we resort back to the original path if `canonicalize()` fails to return the empty set again when encountering an invalid glob. We should especially restore the previous behaviour, as this caused a lot of regressions already, including in zola. Fixes getzola/zola#2150 ref: Keats#819 ref: Keats#820 ref: Keats#799
Regression was introduced in 9479c28. Since that commit, providing an invalid glob to tera will make it fail to create an instance, first (in that commit) by making it panic and since 1f95878 by making it return an error. While returning an error is not entirely unlogical, it doesn't match what most languages do with invalid globs. - Bash will by default return an empty set on invalid globs, as the `failglob` option is off by default - Python will likewise return the empty set instead of throwing an exception, when doing something like `glob.glob("/dev/null/*")` - Rust's `globwalk` will also not error, but return an empty set In fact, we use globwalk in tera and the only reason we panic is by accident, because `std::fs::canonicalize()` checks the path. It is better to match other language's glob behaviour, therefore we resort back to the original path if `canonicalize()` fails. We should especially restore the previous behaviour, as this caused a lot of regressions already, including in zola. Fixes getzola/zola#2150 ref: Keats#819 ref: Keats#820 ref: Keats#799
Regression was introduced in 9479c28. Since that commit, providing an invalid glob to tera will make it fail to create an instance, first (in that commit) by making it panic and since 1f95878 by making it return an error. While returning an error is not entirely unlogical, it doesn't match what most languages do with invalid globs. - Bash will by default return an empty set on invalid globs, as the `failglob` option is off by default - Python will likewise return the empty set instead of throwing an exception, when doing something like `glob.glob("/dev/null/*")` - Rust's `globwalk` will also not error, but return an empty set In fact, we use globwalk in tera and the only reason we panic is by accident, because `std::fs::canonicalize()` checks the path. It is better to match other language's glob behaviour, therefore we resort back to the original path if `canonicalize()` fails. We should especially restore the previous behaviour, as this caused a lot of regressions already, including in zola. Fixes getzola/zola#2150 ref: Keats#819 ref: Keats#820 ref: Keats#799
Regression was introduced in 9479c28. Since that commit, providing an invalid glob to tera will make it fail to create an instance, first (in that commit) by making it panic and since 1f95878 by making it return an error. While returning an error is not entirely unlogical, it doesn't match what most languages do with invalid globs. - Bash will by default return an empty set on invalid globs, as the `failglob` option is off by default - Python will likewise return the empty set instead of throwing an exception, when doing something like `glob.glob("/dev/null/*")` - Rust's `globwalk` will also not error, but return an empty set In fact, we use globwalk in tera and the only reason we panic is by accident, because `std::fs::canonicalize()` checks the path. It is better to match other language's glob behaviour, therefore we resort back to the original path if `canonicalize()` fails. We should especially restore the previous behaviour, as this caused a lot of regressions already, including in zola. Fixes getzola/zola#2150 ref: #819 ref: #820 ref: #799
I have spent 2 hours searching why the hell my Zola project wasn't building to GitHub Pages. Turns git wasn't pushing the empty templates folder. |
New versions of tera panic instead of returning Error when passed an invalid glob path. For example:
rsgen-avro
let mut tera = Tera::new("/dev/null/*")?;
Problem caused by use of unwrap() instead of ? operator, for example:
tera.rs
let parent_dir = std::fs::canonicalize(parent_dir).unwrap();
The text was updated successfully, but these errors were encountered: