[pbs-devel] [PATCH proxmox 3/6] apt: avoid direct impl on api types (use traits instead)

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

From: Dietmar Maurer <dietmar@proxmox.com>

So that we can use api types from expternal crate proxmox-apt-api-types.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 proxmox-apt/src/repositories/file.rs          | 68 ++++++++++++-------
 .../src/repositories/file/list_parser.rs      |  4 +-
 .../src/repositories/file/sources_parser.rs   |  1 +
 proxmox-apt/src/repositories/mod.rs           |  3 +
 proxmox-apt/src/repositories/repository.rs    | 60 ++++++++++------
 proxmox-apt/src/repositories/standard.rs      | 38 +++++++----
 proxmox-apt/tests/repositories.rs             |  3 +
 7 files changed, 118 insertions(+), 59 deletions(-)

diff --git a/proxmox-apt/src/repositories/file.rs b/proxmox-apt/src/repositories/file.rs
index 0d21ce3c..086abf49 100644
--- a/proxmox-apt/src/repositories/file.rs
+++ b/proxmox-apt/src/repositories/file.rs
@@ -9,6 +9,8 @@ use crate::repositories::repository::{
     APTRepository, APTRepositoryFileType, APTRepositoryPackageType,
 };
 
+use crate::repositories::repository::APTRepositoryImpl;
+
 use proxmox_schema::api;
 
 mod list_parser;
@@ -116,13 +118,46 @@ pub struct APTRepositoryInfo {
     pub message: String,
 }
 
-impl APTRepositoryFile {
+pub trait APTRepositoryFileImpl {
     /// Creates a new `APTRepositoryFile` without parsing.
     ///
     /// If the file is hidden, the path points to a directory, or the extension
     /// is usually ignored by APT (e.g. `.orig`), `Ok(None)` is returned, while
     /// invalid file names yield an error.
-    pub fn new<P: AsRef<Path>>(path: P) -> Result<Option<Self>, APTRepositoryFileError> {
+    fn new<P: AsRef<Path>>(path: P) -> Result<Option<APTRepositoryFile>, APTRepositoryFileError>;
+
+    fn with_content(content: String, content_type: APTRepositoryFileType) -> Self;
+
+    /// Check if the file exists.
+    fn exists(&self) -> bool;
+
+    fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError>;
+
+    /// Create an `APTRepositoryFileError`.
+    fn err(&self, error: Error) -> APTRepositoryFileError;
+
+    /// Parses the APT repositories configured in the file on disk, including
+    /// disabled ones.
+    ///
+    /// Resets the current repositories and digest, even on failure.
+    fn parse(&mut self) -> Result<(), APTRepositoryFileError>;
+
+    /// Writes the repositories to the file on disk.
+    ///
+    /// If a digest is provided, checks that the current content of the file still
+    /// produces the same one.
+    fn write(&self) -> Result<(), APTRepositoryFileError>;
+
+    /// Checks if old or unstable suites are configured and that the Debian security repository
+    /// has the correct suite. Also checks that the `stable` keyword is not used.
+    fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo>;
+
+    /// Checks for official URIs.
+    fn check_uris(&self) -> Vec<APTRepositoryInfo>;
+}
+
+impl APTRepositoryFileImpl for APTRepositoryFile {
+    fn new<P: AsRef<Path>>(path: P) -> Result<Option<Self>, APTRepositoryFileError> {
         let path: PathBuf = path.as_ref().to_path_buf();
 
         let new_err = |path_string: String, err: &str| APTRepositoryFileError {
@@ -197,7 +232,7 @@ impl APTRepositoryFile {
         }))
     }
 
-    pub fn with_content(content: String, content_type: APTRepositoryFileType) -> Self {
+    fn with_content(content: String, content_type: APTRepositoryFileType) -> Self {
         Self {
             file_type: content_type,
             content: Some(content),
@@ -207,8 +242,7 @@ impl APTRepositoryFile {
         }
     }
 
-    /// Check if the file exists.
-    pub fn exists(&self) -> bool {
+    fn exists(&self) -> bool {
         if let Some(path) = &self.path {
             PathBuf::from(path).exists()
         } else {
@@ -216,7 +250,7 @@ impl APTRepositoryFile {
         }
     }
 
-    pub fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError> {
+    fn read_with_digest(&self) -> Result<(Vec<u8>, [u8; 32]), APTRepositoryFileError> {
         if let Some(path) = &self.path {
             let content = std::fs::read(path).map_err(|err| self.err(format_err!("{}", err)))?;
             let digest = openssl::sha::sha256(&content);
@@ -233,19 +267,14 @@ impl APTRepositoryFile {
         }
     }
 
-    /// Create an `APTRepositoryFileError`.
-    pub fn err(&self, error: Error) -> APTRepositoryFileError {
+    fn err(&self, error: Error) -> APTRepositoryFileError {
         APTRepositoryFileError {
             path: self.path.clone().unwrap_or_default(),
             error: error.to_string(),
         }
     }
 
-    /// Parses the APT repositories configured in the file on disk, including
-    /// disabled ones.
-    ///
-    /// Resets the current repositories and digest, even on failure.
-    pub fn parse(&mut self) -> Result<(), APTRepositoryFileError> {
+    fn parse(&mut self) -> Result<(), APTRepositoryFileError> {
         self.repositories.clear();
         self.digest = None;
 
@@ -269,11 +298,7 @@ impl APTRepositoryFile {
         Ok(())
     }
 
-    /// Writes the repositories to the file on disk.
-    ///
-    /// If a digest is provided, checks that the current content of the file still
-    /// produces the same one.
-    pub fn write(&self) -> Result<(), APTRepositoryFileError> {
+    fn write(&self) -> Result<(), APTRepositoryFileError> {
         let path = match &self.path {
             Some(path) => path,
             None => {
@@ -336,9 +361,7 @@ impl APTRepositoryFile {
         Ok(())
     }
 
-    /// Checks if old or unstable suites are configured and that the Debian security repository
-    /// has the correct suite. Also checks that the `stable` keyword is not used.
-    pub fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo> {
+    fn check_suites(&self, current_codename: DebianCodename) -> Vec<APTRepositoryInfo> {
         let mut infos = vec![];
 
         let path = match &self.path {
@@ -427,8 +450,7 @@ impl APTRepositoryFile {
         infos
     }
 
-    /// Checks for official URIs.
-    pub fn check_uris(&self) -> Vec<APTRepositoryInfo> {
+    fn check_uris(&self) -> Vec<APTRepositoryInfo> {
         let mut infos = vec![];
 
         let path = match &self.path {
diff --git a/proxmox-apt/src/repositories/file/list_parser.rs b/proxmox-apt/src/repositories/file/list_parser.rs
index 0edeea7f..93bbcc12 100644
--- a/proxmox-apt/src/repositories/file/list_parser.rs
+++ b/proxmox-apt/src/repositories/file/list_parser.rs
@@ -3,9 +3,9 @@ use std::iter::Iterator;
 
 use anyhow::{bail, format_err, Error};
 
-use crate::repositories::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
-
 use super::APTRepositoryParser;
+use crate::repositories::APTRepositoryImpl;
+use crate::repositories::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
 
 // TODO convert %-escape characters. Also adapt printing back accordingly,
 // because at least '%' needs to be re-escaped when printing.
diff --git a/proxmox-apt/src/repositories/file/sources_parser.rs b/proxmox-apt/src/repositories/file/sources_parser.rs
index 9424bbe2..213db9d6 100644
--- a/proxmox-apt/src/repositories/file/sources_parser.rs
+++ b/proxmox-apt/src/repositories/file/sources_parser.rs
@@ -3,6 +3,7 @@ use std::iter::Iterator;
 
 use anyhow::{bail, Error};
 
+use crate::repositories::APTRepositoryImpl;
 use crate::repositories::{
     APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryPackageType,
 };
diff --git a/proxmox-apt/src/repositories/mod.rs b/proxmox-apt/src/repositories/mod.rs
index 26e4192c..014f1820 100644
--- a/proxmox-apt/src/repositories/mod.rs
+++ b/proxmox-apt/src/repositories/mod.rs
@@ -4,17 +4,20 @@ use std::path::PathBuf;
 use anyhow::{bail, Error};
 
 mod repository;
+pub use repository::APTRepositoryImpl;
 pub use repository::{
     APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryPackageType,
 };
 
 mod file;
+pub use file::APTRepositoryFileImpl;
 pub use file::{APTRepositoryFile, APTRepositoryFileError, APTRepositoryInfo};
 
 mod release;
 pub use release::{get_current_release_codename, DebianCodename};
 
 mod standard;
+pub use standard::APTRepositoryHandleImpl;
 pub use standard::{APTRepositoryHandle, APTStandardRepository};
 
 const APT_SOURCES_LIST_FILENAME: &str = "/etc/apt/sources.list";
diff --git a/proxmox-apt/src/repositories/repository.rs b/proxmox-apt/src/repositories/repository.rs
index 91bd64f2..a07db7cb 100644
--- a/proxmox-apt/src/repositories/repository.rs
+++ b/proxmox-apt/src/repositories/repository.rs
@@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize};
 use proxmox_schema::api;
 
 use crate::repositories::standard::APTRepositoryHandle;
+use crate::repositories::standard::APTRepositoryHandleImpl;
 
 #[api]
 #[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq, Eq)]
@@ -188,9 +189,41 @@ pub struct APTRepository {
     pub enabled: bool,
 }
 
-impl APTRepository {
+pub trait APTRepositoryImpl {
     /// Crates an empty repository.
-    pub fn new(file_type: APTRepositoryFileType) -> Self {
+    fn new(file_type: APTRepositoryFileType) -> Self;
+
+    /// Changes the `enabled` flag and makes sure the `Enabled` option for
+    /// `APTRepositoryPackageType::Sources` repositories is updated too.
+    fn set_enabled(&mut self, enabled: bool);
+
+    /// Makes sure that all basic properties of a repository are present and not obviously invalid.
+    fn basic_check(&self) -> Result<(), Error>;
+
+    /// Checks if the repository is the one referenced by the handle.
+    fn is_referenced_repository(
+        &self,
+        handle: APTRepositoryHandle,
+        product: &str,
+        suite: &str,
+    ) -> bool;
+
+    /// Guess the origin from the repository's URIs.
+    ///
+    /// Intended to be used as a fallback for get_cached_origin.
+    fn origin_from_uris(&self) -> Option<String>;
+
+    /// Get the `Origin:` value from a cached InRelease file.
+    fn get_cached_origin(&self) -> Result<Option<String>, Error>;
+
+    /// Writes a repository in the corresponding format followed by a blank.
+    ///
+    /// Expects that `basic_check()` for the repository was successful.
+    fn write(&self, w: &mut dyn Write) -> Result<(), Error>;
+}
+
+impl APTRepositoryImpl for APTRepository {
+    fn new(file_type: APTRepositoryFileType) -> Self {
         Self {
             types: vec![],
             uris: vec![],
@@ -203,9 +236,7 @@ impl APTRepository {
         }
     }
 
-    /// Changes the `enabled` flag and makes sure the `Enabled` option for
-    /// `APTRepositoryPackageType::Sources` repositories is updated too.
-    pub fn set_enabled(&mut self, enabled: bool) {
+    fn set_enabled(&mut self, enabled: bool) {
         self.enabled = enabled;
 
         if self.file_type == APTRepositoryFileType::Sources {
@@ -226,8 +257,7 @@ impl APTRepository {
         }
     }
 
-    /// Makes sure that all basic properties of a repository are present and not obviously invalid.
-    pub fn basic_check(&self) -> Result<(), Error> {
+    fn basic_check(&self) -> Result<(), Error> {
         if self.types.is_empty() {
             bail!("missing package type(s)");
         }
@@ -267,8 +297,7 @@ impl APTRepository {
         Ok(())
     }
 
-    /// Checks if the repository is the one referenced by the handle.
-    pub fn is_referenced_repository(
+    fn is_referenced_repository(
         &self,
         handle: APTRepositoryHandle,
         product: &str,
@@ -299,10 +328,7 @@ impl APTRepository {
             && found_component
     }
 
-    /// Guess the origin from the repository's URIs.
-    ///
-    /// Intended to be used as a fallback for get_cached_origin.
-    pub fn origin_from_uris(&self) -> Option<String> {
+    fn origin_from_uris(&self) -> Option<String> {
         for uri in self.uris.iter() {
             if let Some(host) = host_from_uri(uri) {
                 if host == "proxmox.com" || host.ends_with(".proxmox.com") {
@@ -318,8 +344,7 @@ impl APTRepository {
         None
     }
 
-    /// Get the `Origin:` value from a cached InRelease file.
-    pub fn get_cached_origin(&self) -> Result<Option<String>, Error> {
+    fn get_cached_origin(&self) -> Result<Option<String>, Error> {
         for uri in self.uris.iter() {
             for suite in self.suites.iter() {
                 let mut file = release_filename(uri, suite, false);
@@ -353,10 +378,7 @@ impl APTRepository {
         Ok(None)
     }
 
-    /// Writes a repository in the corresponding format followed by a blank.
-    ///
-    /// Expects that `basic_check()` for the repository was successful.
-    pub fn write(&self, w: &mut dyn Write) -> Result<(), Error> {
+    fn write(&self, w: &mut dyn Write) -> Result<(), Error> {
         match self.file_type {
             APTRepositoryFileType::List => write_one_line(self, w),
             APTRepositoryFileType::Sources => write_stanza(self, w),
diff --git a/proxmox-apt/src/repositories/standard.rs b/proxmox-apt/src/repositories/standard.rs
index 29cc7885..7858fac4 100644
--- a/proxmox-apt/src/repositories/standard.rs
+++ b/proxmox-apt/src/repositories/standard.rs
@@ -111,9 +111,26 @@ impl Display for APTRepositoryHandle {
     }
 }
 
-impl APTRepositoryHandle {
+pub trait APTRepositoryHandleImpl {
     /// Get the description for the repository.
-    pub fn description(self) -> String {
+    fn description(self) -> String;
+    /// Get the display name of the repository.
+    fn name(self) -> String;
+    /// Get the standard file path for the repository referenced by the handle.
+    fn path(self, product: &str) -> String;
+    /// Get package type, possible URIs and the component associated with the handle.
+    ///
+    /// The first URI is the preferred one.
+    fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String);
+    /// Get the standard repository referenced by the handle.
+    ///
+    /// An URI in the result is not '/'-terminated (under the assumption that no valid
+    /// product name is).
+    fn to_repository(self, product: &str, suite: &str) -> APTRepository;
+}
+
+impl APTRepositoryHandleImpl for APTRepositoryHandle {
+    fn description(self) -> String {
         match self {
             APTRepositoryHandle::Enterprise => {
                 "This is the default, stable, and recommended repository, available for all \
@@ -155,8 +172,7 @@ impl APTRepositoryHandle {
         .to_string()
     }
 
-    /// Get the display name of the repository.
-    pub fn name(self) -> String {
+    fn name(self) -> String {
         match self {
             APTRepositoryHandle::Enterprise => "Enterprise",
             APTRepositoryHandle::NoSubscription => "No-Subscription",
@@ -171,8 +187,7 @@ impl APTRepositoryHandle {
         .to_string()
     }
 
-    /// Get the standard file path for the repository referenced by the handle.
-    pub fn path(self, product: &str) -> String {
+    fn path(self, product: &str) -> String {
         match self {
             APTRepositoryHandle::Enterprise => {
                 format!("/etc/apt/sources.list.d/{}-enterprise.list", product)
@@ -188,10 +203,7 @@ impl APTRepositoryHandle {
         }
     }
 
-    /// Get package type, possible URIs and the component associated with the handle.
-    ///
-    /// The first URI is the preferred one.
-    pub fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String) {
+    fn info(self, product: &str) -> (APTRepositoryPackageType, Vec<String>, String) {
         match self {
             APTRepositoryHandle::Enterprise => (
                 APTRepositoryPackageType::Deb,
@@ -259,11 +271,7 @@ impl APTRepositoryHandle {
         }
     }
 
-    /// Get the standard repository referenced by the handle.
-    ///
-    /// An URI in the result is not '/'-terminated (under the assumption that no valid
-    /// product name is).
-    pub fn to_repository(self, product: &str, suite: &str) -> APTRepository {
+    fn to_repository(self, product: &str, suite: &str) -> APTRepository {
         let (package_type, uris, component) = self.info(product);
 
         APTRepository {
diff --git a/proxmox-apt/tests/repositories.rs b/proxmox-apt/tests/repositories.rs
index ae92e51c..37d665bf 100644
--- a/proxmox-apt/tests/repositories.rs
+++ b/proxmox-apt/tests/repositories.rs
@@ -8,6 +8,9 @@ use proxmox_apt::repositories::{
     check_repositories, get_current_release_codename, standard_repositories, APTRepositoryFile,
     APTRepositoryHandle, APTRepositoryInfo, APTStandardRepository, DebianCodename,
 };
+use proxmox_apt::repositories::{
+    APTRepositoryFileImpl, APTRepositoryHandleImpl, APTRepositoryImpl,
+};
 
 fn create_clean_directory(path: &PathBuf) -> Result<(), Error> {
     match std::fs::remove_dir_all(path) {
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel