Add internal resolution for assets in `get_url` function #2726

ZzMzaw · 2024-12-06T10:57:53Z

This PR solves issue #2598 through internal resolution for assets (i.e. using @ in front of the asset path).

It allow to retrieve an assets permalink based on its associated page and according to the language.
Documentation regarding the galery shortcode has been updated accordingly.

Beware supports for resolving assets internally has only been implemented for the get_url function.
I am not sure if it should be generalized to links as well (as get_url can and should be used to feed the href of a link).

This PR could maybe provide an answer to the issue described by the PR #2146 (a not to-be-merged PR for demo purpose only) even if it doesn't fix it where the PR points out (not sure about this at all).

Sanity check:

Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

Have you created/updated the relevant documentation page(s)?

Colocated path is required for computing serialized assets and assets permalinks. Test have been added to check colocated path format as expected by those computations.

ZzMzaw · 2024-12-07T04:43:58Z

There is something wrong with serialization of assets in sub-directories on windows.
I'll look into it but as I don't have windows to reproduce the issue locally, I'll use this PR for testing.
I may force-push several time until I'm able to fix the issue.
I'll keep you posted once it is done.

Fix assets serialization behavior which was relying on the output path instead of the source path like for pages. This behavior was making assets permalinks computation fail.

Compute assets permalinks for serialized assets based on associated page or section permalink.

Assets are duplicated for every language so permalinks should be releated to it in order to be language-aware.

Assets permalinks allows to resolve internally colocated assets as well as pages and sections.

Galery can be fixed by using internal resolution. Adding the lang as well allows to make the target URL lang-aware even if the image is just duplicated. Example file has been fixed as well even if it might not be used.

ZzMzaw · 2024-12-07T06:22:07Z

Tests are successful now, including on Windows.
It would be great if someone could test the galery shortcode on Windows.

Anyway, PR can be reviewed now.

Keats · 2024-12-18T13:40:06Z

components/content/src/file_info.rs

+            assert!(file.colocated_path.is_some());
+            if let Some(colocated_path) = file.colocated_path {
+                assert!(!colocated_path.starts_with('/'));
+                assert!(colocated_path.ends_with('/'));


Can you assert what the actual path it?

I just asserted what is really important for a colocated path which is not starting with a '/' and ending with a '/'.
I wanted to emphasize that because if you change the way you compute colocated path in the future but don't respect those two assertions, you'll break the current feature.

I definitely can split the check (one for the page and one for the section as they have different colocated path) in this test while adding a comment on what is really important to keep through time.
Would it be ok for you?

Yes that would be fine

Keats · 2024-12-18T14:14:58Z

components/content/src/utils.rs

+        .map(|asset_relative_path_as_string| {
+            format!(
+                "/{}{}",
+                colocated_path.expect("Should have colocated path for assets"),


If it should have it, just pass &String for colocated_path instead of the Option

If assets is empty, I don't have any guarantee that colocated_path will be Some.
I tried to reuse the same structure as the existing code but I can add an if !assets.is_empty() to unwrap this variable once instead of doing it in the iteration.
Doing it outside of serialize_assets to get rid of the Option in the signature would require to wrap the call of this function in the if statement mentionned above.
This would result in code duplication as I would have to do it twice, one for the pages and one for the sections.

What would you prefer me to implement?

Keep it as it is

Add the if statement in serialize_assets function to avoid unwrapping the option in the iteration

Add the if statement around the serialize_assets

Keats · 2024-12-18T14:15:46Z

components/content/src/utils.rs

+) -> Vec<String> {
+    assets
+        .iter()
+        .filter_map(|asset| asset.strip_prefix(parent_path.unwrap()).ok())


Pass parent_path: &Path rather than the option if all we're doing is unwrapping it

If assets is empty, I don't have any guarantee that parent_path will be Some.
I tried to reuse the same structure as the existing code but I can add an if !assets.is_empty() to unwrap this variable once instead of doing it in the iteration.
Doing it outside of serialize_assets to get rid of the Option in the signature would require to wrap the call of this function in the if statement mentionned above.
This would result in code duplication as I would have to do it twice, one for the pages and one for the sections.

What would you prefer me to implement?

Keep it as it is

Add the if statement in serialize_assets function to avoid unwrapping the option in the iteration

Add the if statement around the serialize_assets

How much duplication are we talking about? I'm leaning towards 3

We are just talking about a few lines (if !assets.is_empty() statement and of parent_path and colocated_path options unwrapping) duplicated twice, for pages and sections (where serialize_assets is currently used).

If we go for 3., we should apply the same logic to the get_assets_permalinks function by moving l.95-96 (if !serialized_assets.is_empty() statement and colocated_path option unwraping) around the function call.

Is it ok if I proceed that way?

I think it's fine. Eventually page/sections will converge probably so it's going to reduce the duplication

components/content/src/utils.rs

Keats · 2024-12-18T14:18:31Z

components/site/src/lib.rs

    /// We need that if there are relative links in the content that need to be resolved
    pub permalinks: HashMap<String, String>,
+    /// A map of all assets (non .md files colocated to sections and pages) and their permalink, distributed per lang
+    /// We need that if there are relative links in the content that need to be resolved
+    pub assets_permalinks: HashMap<String, HashMap<String, String>>,


what's the reason for separating by lang? Otherwise we could just add them to the permalinks maps
Are the assets copied for each language? I can't remember

Yes, assets are copied for each language.

This will allow to have different permalink depending on the lang for the same asset:

For the default lang, path/to/the/asset.jpg would become https://yourwebsite.com/slug/to/the/asset.jpg

For the non-default lang lang, path/to/the/asset.jpg would become https://yourwebsite.com/lang/slug/to/the/asset.jpg

I tested several things such as always defaulting to the default lang but I find this solution to be more clean and future proof.
When a lang is selected, your assets point to the same base URL than your page.
In addition, even if today it is not the case, if we introduce per-lang assets, it will just require minor adaptations.

Please let me know if you prefer to have something more simple.

Yes, assets are copied for each language.

Ah that's not ideal from content duplication/processing I guess. We can't even define a language specific asset and I don't think it's something that will be added.

I think it makes more sense for assets to be shared across languages and be located once in the folder of the main language page

Ah that's not ideal from content duplication/processing I guess.

This is mandatory and may remain mandatory in the future from my understanding.
We'll break any relative links in non-default language pages otherwise.
Let's imagine we have a test.file colocated file and a link defined as follow: <a href="test.file">, it would work in default language (resolved as https://yourwebsite.com/slug/to/the/test.file by the browser) but would break in non-default ones in case content was not duplicated (resolved as https://yourwebsite.com/lang/slug/to/the/test.file by the browser).

I think it makes more sense for assets to be shared across languages and be located once in the folder of the main language page

If it is still the case, I'll revert my changes to ignore the lang for assets in get_url function.
Is it ok for you?

Keep like you have right now. In an ideal world, we would serve the assets from a single place and rewrite the links to assets to point to the right thing but that's not for now.

As discussed during PR review, it is better to check for the entire path than just the required assumptions. Assumptions have been written as a comment.

As discussed during PR review, move option unwrapping logic outside of each function

ZzMzaw · 2025-01-03T08:32:44Z

Everything may have been fixed as per our discussion.
I let you check and resolve comments if it fits your expectations.
Feel free to let me know if there is still something to adapt.
Many thanks for your review.

Keats · 2025-01-04T21:55:40Z

components/content/src/page.rs

@@ -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() {


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

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.

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

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.

Keats · 2025-01-04T21:56:35Z

components/content/src/utils.rs

+) -> HashMap<String, String> {
+    let mut permalinks = HashMap::new();
+    if !serialized_assets.is_empty() {
+        for asset in serialized_assets {


If you make it into an iterator, you don't need that check

As per PR discussion, align get_asset_permalinks with serialize_assets.

As per PR discussion, it makes more sense to have both asset serialization and permalinks generation in the same logical unit. Change has been made for both page and section.

Compute colocated path for sections

9278bd1

Colocated path is required for computing serialized assets and assets permalinks. Test have been added to check colocated path format as expected by those computations.

ZzMzaw force-pushed the feat/resolve-internal-assets branch from 2115e03 to 3fb51eb Compare December 7, 2024 05:13

ZzMzaw added 5 commits December 7, 2024 06:23

Harmonize serialize assets for pages and sections

8ff616d

Fix assets serialization behavior which was relying on the output path instead of the source path like for pages. This behavior was making assets permalinks computation fail.

Add assets permalinks for pages and sections

faed94a

Compute assets permalinks for serialized assets based on associated page or section permalink.

Add assets permalinks per language to site

106c547

Assets are duplicated for every language so permalinks should be releated to it in order to be language-aware.

Add assets permalinks to get_url

9620795

Assets permalinks allows to resolve internally colocated assets as well as pages and sections.

Fix documentation about galery

f74f551

Galery can be fixed by using internal resolution. Adding the lang as well allows to make the target URL lang-aware even if the image is just duplicated. Example file has been fixed as well even if it might not be used.

ZzMzaw force-pushed the feat/resolve-internal-assets branch from 3fb51eb to f74f551 Compare December 7, 2024 05:31

Keats reviewed Dec 18, 2024

View reviewed changes

ZzMzaw added 3 commits December 19, 2024 06:38

Fix hashmap init for assets permalink test

49f66b2

Check page and section entire colocated path

334f687

As discussed during PR review, it is better to check for the entire path than just the required assumptions. Assumptions have been written as a comment.

Rework serialize_assets and get_assets_permalinks

23051b8

As discussed during PR review, move option unwrapping logic outside of each function

Keats reviewed Jan 4, 2025

View reviewed changes

ZzMzaw added 2 commits January 5, 2025 07:43

Use iterator for get_asset_permalinks

d213e39

As per PR discussion, align get_asset_permalinks with serialize_assets.

Regroup assets serialization and permalinks calls

2f69aa5

As per PR discussion, it makes more sense to have both asset serialization and permalinks generation in the same logical unit. Change has been made for both page and section.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal resolution for assets in `get_url` function #2726

Add internal resolution for assets in `get_url` function #2726

ZzMzaw commented Dec 6, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 26, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 26, 2024

ZzMzaw Dec 28, 2024

Keats Dec 29, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024 •

edited

Loading

Keats Dec 26, 2024

ZzMzaw Dec 28, 2024

Keats Dec 29, 2024

ZzMzaw commented Jan 3, 2025

Keats Jan 4, 2025

ZzMzaw Jan 5, 2025 •

edited

Loading

Keats Jan 7, 2025

ZzMzaw Jan 8, 2025

Keats Jan 4, 2025

Add internal resolution for assets in get_url function #2726

Are you sure you want to change the base?

Add internal resolution for assets in get_url function #2726

Conversation

ZzMzaw commented Dec 6, 2024 • edited Loading

Code changes

ZzMzaw commented Dec 7, 2024 • edited Loading

ZzMzaw commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZzMzaw Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZzMzaw commented Jan 3, 2025

Choose a reason for hiding this comment

ZzMzaw Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add internal resolution for assets in `get_url` function #2726

Add internal resolution for assets in `get_url` function #2726

ZzMzaw commented Dec 6, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024 •

edited

Loading

ZzMzaw Dec 19, 2024 •

edited

Loading

ZzMzaw Jan 5, 2025 •

edited

Loading