Skip to content
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.

Sign up for GitHub

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

Add internal resolution for assets in get_url function #2726

Open
wants to merge 11 commits into
base: next
Choose a base branch
from
+579 −62
Open

Add internal resolution for assets in get_url function #2726

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9278bd1
Compute colocated path for sections
ZzMzaw Dec 6, 2024
8ff616d
Harmonize serialize assets for pages and sections
ZzMzaw Dec 6, 2024
faed94a
Add assets permalinks for pages and sections
ZzMzaw Dec 6, 2024
106c547
Add assets permalinks per language to site
ZzMzaw Dec 6, 2024
9620795
Add assets permalinks to `get_url`
ZzMzaw Dec 6, 2024
f74f551
Fix documentation about galery
ZzMzaw Dec 6, 2024
49f66b2
Fix hashmap init for assets permalink test
ZzMzaw Dec 19, 2024
334f687
Check page and section entire colocated path
ZzMzaw Jan 3, 2025
23051b8
Rework serialize_assets and get_assets_permalinks
ZzMzaw Jan 3, 2025
d213e39
Use iterator for get_asset_permalinks
ZzMzaw Jan 5, 2025
2f69aa5
Regroup assets serialization and permalinks calls
ZzMzaw Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion components/content/src/file_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,18 @@ impl FileInfo {
format!("{}.md", name)
};
let grand_parent = parent.parent().map(|p| p.to_path_buf());
let mut colocated_path = None;

// If we have a folder with an asset, don't consider it as a component
// Splitting on `.` as we might have a language so it isn't *only* index but also index.fr
// etc
if !components.is_empty() && name.split('.').collect::<Vec<_>>()[0] == "_index" {
colocated_path = Some({
let mut val = components.join("/");
val.push('/');
val
});
}

FileInfo {
filename: file_path.file_name().unwrap().to_string_lossy().to_string(),
Expand All @@ -119,7 +131,7 @@ impl FileInfo {
name,
components,
relative,
colocated_path: None,
colocated_path: colocated_path,
}
}

Expand Down Expand Up @@ -289,4 +301,41 @@ mod tests {
Path::new("/home/vincent/code/site/content/posts/tutorials/python/index")
);
}

#[test]
fn correct_colocated_path() {
struct Test<'a> {
file_info: FileInfo,
expected_colocated_path: &'a str,
}

// A colocated path:
// - MUST NOT start with a '/'
// - MUST end with a '/'
// Breaking those assumptions may have uncontrolled side effects in some other code, including but not limited to assets permalinks generation.
let tests = vec![
Test {
file_info: FileInfo::new_page(
Path::new("/home/vincent/code/site/content/posts/tutorials/python/index.md"),
&PathBuf::new(),
),
expected_colocated_path: "posts/tutorials/python/",
},
Test {
file_info: FileInfo::new_section(
Path::new("/home/vincent/code/site/content/posts/tutorials/_index.fr.md"),
&PathBuf::new(),
),
expected_colocated_path: "posts/tutorials/",
},
];

for test in tests {
assert!(test.file_info.colocated_path.is_some());
assert_eq!(
test.file_info.colocated_path.as_ref().unwrap(),
test.expected_colocated_path
)
}
}
}
92 changes: 68 additions & 24 deletions components/content/src/page.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::front_matter::{split_page_content, PageFrontMatter};
use crate::library::Library;
use crate::ser::SerializingPage;
use crate::utils::get_reading_analytics;
use crate::utils::{find_related_assets, has_anchor};
use crate::utils::{find_related_assets, get_assets_permalinks, has_anchor, serialize_assets};
use utils::anchors::has_anchor_id;
use utils::fs::read_file;

Expand All @@ -45,6 +45,8 @@ pub struct Page {
pub assets: Vec<PathBuf>,
/// All the non-md files we found next to the .md file
pub serialized_assets: Vec<String>,
/// The permalinks of all the non-md files we found next to the .md file
pub assets_permalinks: HashMap<String, String>,
/// The HTML rendered of the page
pub content: String,
/// The slug of that page.
Expand Down Expand Up @@ -195,7 +197,26 @@ impl Page {
if page.file.name == "index" {
let parent_dir = path.parent().unwrap();
page.assets = find_related_assets(parent_dir, config, true);
page.serialized_assets = page.serialize_assets(base_path);
if !page.assets.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need that if right? It will be an empty vec if page.assets is empty. Same for the one below

Copy link
Author

@ZzMzaw ZzMzaw Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I want to fail if colocated_path is none while page.assets is not empty because it should not happen (and this is covered by the expect) but no having a colocated_path is totally valid if there are no assets.
(similar reason for serialized_assets below).
In case we have no assets, serialized_assets and assets_permalinks should remain with default values.

I agree it would make more sense to regroup serialize_assets and get_assets_permalinks in the same block and retrieve colocated_path only once.
Would it be better if code was reworked as follow?

if !page.assets.is_empty() {
    let colocated_path = page.file.colocated_path.as_ref().expect("Should have a colocated path if we have assets");
    page.serialized_assets = serialize_assets(
        &page.assets,
        page.file.path.parent().unwrap(),
        colocated_path
    );
    page.assets_permalinks = get_assets_permalinks(
        &page.serialized_assets,
        &page.permalink,
        colocated_path
    );
}

I may have missed something so please don't hesitate to point it out.
Thanks for your feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's an error that can happen it should be an error and not a panic. What you propose there is better yes

Copy link
Author

Choose a reason for hiding this comment

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

We should never reach the expect within the if so keeping the panic might be ok.
Anyway, user won't be able to circumvent it so it will have to be fixed in zola itself (and it means we may have done some incompatible changes).
All changes have been pushed.

page.serialized_assets = serialize_assets(
&page.assets,
page.file.path.parent().unwrap(),
page.file
.colocated_path
.as_ref()
.expect("Should have colocated path for assets"),
);
}
if !page.serialized_assets.is_empty() {
page.assets_permalinks = get_assets_permalinks(
&page.serialized_assets,
&page.permalink,
page.file
.colocated_path
.as_ref()
.expect("Should have colocated path for assets"),
);
}
} else {
page.assets = vec![];
}
Expand Down Expand Up @@ -255,28 +276,6 @@ impl Page {
.with_context(|| format!("Failed to render page '{}'", self.file.path.display()))
}

/// Creates a vectors of asset URLs.
fn serialize_assets(&self, base_path: &Path) -> Vec<String> {
self.assets
.iter()
.filter_map(|asset| asset.strip_prefix(self.file.path.parent().unwrap()).ok())
.filter_map(|filename| filename.to_str())
.map(|filename| {
let mut path = self.file.path.clone();
// Popping the index.md from the path since file.parent would be one level too high
// for our need here
path.pop();
path.push(filename);
path = path
.strip_prefix(&base_path.join("content"))
.expect("Should be able to stripe prefix")
.to_path_buf();
path
})
.map(|path| format!("/{}", path.display()))
.collect()
}

pub fn has_anchor(&self, anchor: &str) -> bool {
has_anchor(&self.toc, anchor)
}
Expand Down Expand Up @@ -589,8 +588,19 @@ And here's another. [^3]
assert_eq!(page.file.parent, path.join("content").join("posts"));
assert_eq!(page.slug, "with-assets");
assert_eq!(page.assets.len(), 3);
assert_eq!(page.serialized_assets.len(), 3);
assert!(page.serialized_assets[0].starts_with('/'));
assert_eq!(page.permalink, "http://a-website.com/posts/with-assets/");
assert_eq!(page.assets_permalinks.len(), 3);
let random_assets_permalinks_key =
page.assets_permalinks.keys().next().expect("assets permalinks key should be present");
assert!(!random_assets_permalinks_key.starts_with('/'));
let random_assets_permalinks_value = page
.assets_permalinks
.values()
.next()
.expect("assets permalinks value should be present");
assert!(random_assets_permalinks_value.starts_with(&page.permalink));
}

#[test]
Expand All @@ -614,6 +624,13 @@ And here's another. [^3]
assert_eq!(page.slug, "hey");
assert_eq!(page.assets.len(), 3);
assert_eq!(page.permalink, "http://a-website.com/posts/hey/");
assert_eq!(page.assets_permalinks.len(), 3);
let random_assets_permalinks_value = page
.assets_permalinks
.values()
.next()
.expect("assets permalinks value should be present");
assert!(random_assets_permalinks_value.starts_with(&page.permalink));
}

// https://github.com/getzola/zola/issues/674
Expand All @@ -640,6 +657,17 @@ And here's another. [^3]
// We should not get with-assets since that's the slugified version
assert!(page.serialized_assets[0].contains("with_assets"));
assert_eq!(page.permalink, "http://a-website.com/posts/with-assets/");
assert_eq!(page.assets_permalinks.len(), 3);
let random_assets_permalinks_key =
page.assets_permalinks.keys().next().expect("assets permalinks key should be present");
// We should not get with-assets since that's the slugified version
assert!(random_assets_permalinks_key.contains("with_assets"));
let random_assets_permalinks_value = page
.assets_permalinks
.values()
.next()
.expect("assets permalinks value should be present");
assert!(random_assets_permalinks_value.starts_with(&page.permalink));
}

// https://github.com/getzola/zola/issues/607
Expand All @@ -664,7 +692,21 @@ And here's another. [^3]
assert_eq!(page.slug, "with-assets");
assert_eq!(page.meta.date, Some("2013-06-02".to_string()));
assert_eq!(page.assets.len(), 3);
assert_eq!(page.serialized_assets.len(), 3);
// We should not get with-assets since that's the slugified version
assert!(page.serialized_assets[0].contains("2013-06-02"));
assert_eq!(page.permalink, "http://a-website.com/posts/with-assets/");
assert_eq!(page.assets_permalinks.len(), 3);
let random_assets_permalinks_key =
page.assets_permalinks.keys().next().expect("assets permalinks key should be present");
// We should not get with-assets since that's the slugified version
assert!(random_assets_permalinks_key.contains("2013-06-02"));
let random_assets_permalinks_value = page
.assets_permalinks
.values()
.next()
.expect("assets permalinks value should be present");
assert!(random_assets_permalinks_value.starts_with(&page.permalink));
}

#[test]
Expand Down Expand Up @@ -692,6 +734,8 @@ And here's another. [^3]
let page = res.unwrap();
assert_eq!(page.assets.len(), 1);
assert_eq!(page.assets[0].file_name().unwrap().to_str(), Some("graph.jpg"));
assert_eq!(page.serialized_assets.len(), 1);
assert_eq!(page.assets_permalinks.len(), 1);
}

// https://github.com/getzola/zola/issues/1566
Expand Down
61 changes: 46 additions & 15 deletions components/content/src/section.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ use crate::file_info::FileInfo;
use crate::front_matter::{split_section_content, SectionFrontMatter};
use crate::library::Library;
use crate::ser::{SectionSerMode, SerializingSection};
use crate::utils::{find_related_assets, get_reading_analytics, has_anchor};
use crate::utils::{
find_related_assets, get_assets_permalinks, get_reading_analytics, has_anchor, serialize_assets,
};

// Default is used to create a default index section if there is no _index.md in the root content directory
#[derive(Clone, Debug, Default, PartialEq, Eq)]
Expand All @@ -38,6 +40,8 @@ pub struct Section {
pub assets: Vec<PathBuf>,
/// All the non-md files we found next to the .md file as string
pub serialized_assets: Vec<String>,
/// The permalinks of all the non-md files we found next to the .md file
pub assets_permalinks: HashMap<String, String>,
/// All direct pages of that section
pub pages: Vec<PathBuf>,
/// All pages that cannot be sorted in this section
Expand Down Expand Up @@ -125,7 +129,28 @@ impl Section {

let parent_dir = path.parent().unwrap();
section.assets = find_related_assets(parent_dir, config, false);
section.serialized_assets = section.serialize_assets();
if !section.assets.is_empty() {
section.serialized_assets = serialize_assets(
&section.assets,
section.file.path.parent().unwrap(),
section
.file
.colocated_path
.as_ref()
.expect("Should have colocated path for assets"),
);
}
if !section.serialized_assets.is_empty() {
section.assets_permalinks = get_assets_permalinks(
&section.serialized_assets,
&section.permalink,
section
.file
.colocated_path
.as_ref()
.expect("Should have colocated path for assets"),
);
}

Ok(section)
}
Expand Down Expand Up @@ -202,16 +227,6 @@ impl Section {
self.file.components.is_empty()
}

/// Creates a vectors of asset URLs.
fn serialize_assets(&self) -> Vec<String> {
self.assets
.iter()
.filter_map(|asset| asset.strip_prefix(self.file.path.parent().unwrap()).ok())
.filter_map(|filename| filename.to_str())
.map(|filename| format!("{}{}", self.path, filename))
.collect()
}

pub fn has_anchor(&self, anchor: &str) -> bool {
has_anchor(&self.toc, anchor)
}
Expand Down Expand Up @@ -269,8 +284,22 @@ mod tests {
assert!(res.is_ok());
let section = res.unwrap();
assert_eq!(section.assets.len(), 3);
assert_eq!(section.serialized_assets.len(), 3);
assert!(section.serialized_assets[0].starts_with('/'));
assert_eq!(section.permalink, "http://a-website.com/posts/with-assets/");
assert_eq!(section.assets_permalinks.len(), 3);
let random_assets_permalinks_key = section
.assets_permalinks
.keys()
.next()
.expect("assets permalinks key should be present");
assert!(!random_assets_permalinks_key.starts_with('/'));
let random_assets_permalinks_value = section
.assets_permalinks
.values()
.next()
.expect("assets permalinks value should be present");
assert!(random_assets_permalinks_value.starts_with(&section.permalink));
}

#[test]
Expand Down Expand Up @@ -300,9 +329,11 @@ mod tests {
Section::from_file(article_path.join("_index.md").as_path(), &config, &PathBuf::new());

assert!(res.is_ok());
let page = res.unwrap();
assert_eq!(page.assets.len(), 1);
assert_eq!(page.assets[0].file_name().unwrap().to_str(), Some("graph.jpg"));
let section = res.unwrap();
assert_eq!(section.assets.len(), 1);
assert_eq!(section.assets[0].file_name().unwrap().to_str(), Some("graph.jpg"));
assert_eq!(section.serialized_assets.len(), 1);
assert_eq!(section.assets_permalinks.len(), 1);
}

#[test]
Expand Down
Loading