Re: [pbs-devel] [PATCH proxmox-backup 4/6] server/config: add ability to get mtime of files for template

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Nov 06, 2020 at 11:03:41AM +0100, Dominik Csapak wrote:
> this will help us to circumvent browser caching issues, by
> giving the server config a list of files that it will stat for their mtime
> which we can the use in the template to append a "GET" parameter
> (this lets the browser refresh the resource if the parameter is different)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/bin/proxmox-backup-proxy.rs |  4 +--
>  src/server/config.rs            | 55 +++++++++++++++++++++++----------
>  src/server/rest.rs              |  2 +-
>  3 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 3f07db89..d4425adc 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -94,8 +94,8 @@ async fn run() -> Result<(), Error> {
>  
>      let mut indexpath = PathBuf::from(buildcfg::JS_DIR);
>      indexpath.push("index.hbs");
> -    config.register_template("index", &indexpath)?;
> -    config.register_template("console", "/usr/share/pve-xtermjs/index.html.hbs")?;
> +    config.register_template("index", &indexpath, &[])?;
> +    config.register_template("console", "/usr/share/pve-xtermjs/index.html.hbs", &[])?;
>  
>      let mut commando_sock = server::CommandoSocket::new(server::our_ctrl_sock());
>  
> diff --git a/src/server/config.rs b/src/server/config.rs
> index c7300668..846725a3 100644
> --- a/src/server/config.rs
> +++ b/src/server/config.rs
> @@ -7,12 +7,12 @@ use std::sync::{Arc, Mutex, RwLock};
>  use anyhow::{bail, Error, format_err};
>  use hyper::Method;
>  use handlebars::Handlebars;
> -use serde::Serialize;
> +use serde_json::{Value, json};
>  
>  use proxmox::api::{ApiMethod, Router, RpcEnvironmentType};
>  use proxmox::tools::fs::{create_path, CreateOptions};
>  
> -use crate::tools::{FileLogger, FileLogOptions};
> +use crate::tools::{FileLogger, FileLogOptions, self};
>  
>  pub struct ApiConfig {
>      basedir: PathBuf,
> @@ -20,7 +20,7 @@ pub struct ApiConfig {
>      aliases: HashMap<String, PathBuf>,
>      env_type: RpcEnvironmentType,
>      templates: RwLock<Handlebars<'static>>,
> -    template_files: RwLock<HashMap<String, (SystemTime, PathBuf)>>,
> +    template_files: RwLock<HashMap<String, (SystemTime, PathBuf, Vec<PathBuf>)>>,

^ not very self explanatory as a tuple. If this were a struct you could
also move the 'get_timestamps' helper from inside `render_template` to
the struct as a method.

>      request_log: Option<Arc<Mutex<FileLogger>>>,
>  }
>  
> @@ -76,7 +76,7 @@ impl ApiConfig {
>          self.env_type
>      }
>  
> -    pub fn register_template<P>(&self, name: &str, path: P) -> Result<(), Error>
> +    pub fn register_template<P>(&self, name: &str, path: P, files_to_check: &[&str]) -> Result<(), Error>
>      where
>          P: Into<PathBuf>
>      {
> @@ -85,42 +85,63 @@ impl ApiConfig {
>          }
>  
>          let path: PathBuf = path.into();
> -        let metadata = metadata(&path)?;
> -        let mtime = metadata.modified()?;
> +        let mtime = metadata(&path)?.modified()?;
> +
> +        let mut files = Vec::new();
> +
> +        for file in files_to_check {
> +            let (_, components) = crate::tools::normalize_uri_path(file)?;
> +            let path = self.find_alias(&components);
> +            if path.file_name().is_none() {
> +                bail!("error registering template: has no filename: {:?}", file);
> +            }
> +            files.push(path);
> +        }
>  
>          self.templates.write().unwrap().register_template_file(name, &path)?;
> -        self.template_files.write().unwrap().insert(name.to_string(), (mtime, path));
> +        self.template_files.write().unwrap().insert(name.to_string(), (mtime, path, files));
>  
>          Ok(())
>      }
>  
>      /// Checks if the template was modified since the last rendering
>      /// if yes, it loads a the new version of the template
> -    pub fn render_template<T>(&self, name: &str, data: &T) -> Result<String, Error>
> -    where
> -        T: Serialize,
> +    pub fn render_template(&self, name: &str, mut data: Value) -> Result<String, Error>
>      {
> -        let path;
> +        fn get_timestamps(files: &Vec<PathBuf>) -> Value {
> +            let mut value = json!({});
> +
> +            for file in files {
> +                let mtime = tools::fs::get_file_mtime(file).unwrap_or_else(|_| 0);

This could just use `.unwrap_or(0)` ;-)

> +                let filename = file.file_name().unwrap().to_string_lossy();
> +                value[filename.into_owned()] = Value::from(mtime);
> +            }
> +            value
> +        }
> +
>          let mtime;
>          {
>              let template_files = self.template_files.read().unwrap();
> -            let (old_mtime, old_path) = template_files.get(name).ok_or_else(|| format_err!("template not found"))?;
> +            let (old_mtime, old_path, files) = template_files.get(name).ok_or_else(|| format_err!("template not found"))?;
>  
>              mtime = metadata(old_path)?.modified()?;
>              if mtime <= *old_mtime {
> -                return self.templates.read().unwrap().render(name, data).map_err(|err| format_err!("{}", err));
> +                data["times".to_string()] = get_timestamps(files);
> +                return self.templates.read().unwrap().render(name, &data).map_err(|err| format_err!("{}", err));

while you're at it - can this not just be `.map_err(Error::from)`?
Alternatively without mapping the error you can `Ok`-wrap it with a `?`
operator: `return Ok(foo.render()?);` that implies the `error.into()`.

>              }
> -            path = old_path.to_path_buf();
>          }
>  
>          {
>              let mut template_files = self.template_files.write().unwrap();
>              let mut templates = self.templates.write().unwrap();
>  
> -            templates.register_template_file(name, &path)?;
> -            template_files.insert(name.to_string(), (mtime, path));
> +            let (time, path, files) = template_files.get_mut(name).ok_or_else(|| format_err!("template not found"))?;
> +
> +            templates.register_template_file(name, path)?;
> +            *time = mtime;
>  
> -            templates.render(name, data).map_err(|err| format_err!("{}", err))
> +            data["times".to_string()] = get_timestamps(files);
> +            templates.render(name, &data).map_err(|err| format_err!("{}", err))

Same ^

>          }
>      }
>  
> diff --git a/src/server/rest.rs b/src/server/rest.rs
> index d0169ba5..fa01522d 100644
> --- a/src/server/rest.rs
> +++ b/src/server/rest.rs
> @@ -437,7 +437,7 @@ fn get_index(
>          "debug": debug,
>      });
>  
> -    let (ct, index) = match api.render_template(template_file, &data) {
> +    let (ct, index) = match api.render_template(template_file, data) {
>          Ok(index) => ("text/html", index),
>          Err(err) => {
>              ("text/plain", format!("Error rendering template: {}", err))
> -- 
> 2.20.1