Skip to content

Canonicalize paths to ensure includes behave as expected when folders are involved#289

Merged
Kijewski merged 3 commits intoaskama-rs:masterfrom
LukeMathWalker:includes
Dec 16, 2024
Merged

Canonicalize paths to ensure includes behave as expected when folders are involved#289
Kijewski merged 3 commits intoaskama-rs:masterfrom
LukeMathWalker:includes

Conversation

@LukeMathWalker
Copy link
Copy Markdown
Contributor

Fixes #288

@LukeMathWalker
Copy link
Copy Markdown
Contributor Author

I'm panicking if the canonicalization fails—an alternative would be to return a compilation error. Tell me what you'd prefer.

Comment thread rinja_derive/src/config.rs Outdated
Comment on lines 186 to 191
let relative = root.with_file_name(path);
if relative.exists() {
return Ok(relative.into());
return Ok(relative.canonicalize().unwrap().into());
}
Copy link
Copy Markdown
Member

@Kijewski Kijewski Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for finding the issue, finding the source of the error, and presenting a solution all in time-span of less than an hour!

I wonder if it would better to change the order of the test (untested pseudo-code):

if let Ok(absolute) = root.with_file_name(path).canonicalize() {
    if absolute.exists() {
        return Ok(absolute.into());
    }
}

Then we don't have to care if and why canonicalizing the path didn't succeed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on rinja!

Re: order—we definitely could, but I wonder if that may lead to confusion in case the canonicalization fails but the file exists? No error would be reported, the user would see the file being where it is expected to be yet rinja would behave as if it isn't there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you're right. It's better to tell the user that there is a problem instead of ignoring it. I like explicit error messages better than a panic, because then fallback Template implementation with an empty body is generated to suppress subsequent error messages. Could you please add a check like:

relative.canonicalize().map_err(|err| CompileError::no_file_info(format_args!("could not canonicalize path {relative:?}: {err}"), None))?

I don't know when the canonicalization could fail, though, when it was successfully deemed to exists. If the resolved path is excessively long? Well, in the end it does not matter, I guess. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Kijewski Kijewski merged commit 1e66204 into askama-rs:master Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] #[derive(Template)] panics when using include with nested templates

2 participants