feat: parse workbook properties (core + extended) - #685
Conversation
Adds WorkbookProperties to Metadata, exposing XLSX docProps/core.xml and docProps/app.xml fields such as creator, last_modified_by, application, company, etc. Closes tafia#677
|
Overall this is a good (and useful) PR. I would have probably mapped some of the elements to different types but I think having everything as Could you also add an example to the examples directory in the format of some of the other examples. Also add an entry to There is also the possibility to have user defined custom fields but that can be left for another PR if it is requested. In that case the field could be |
jmcnamara
added
the
awaiting user changes
|
thanks @jmcnamara yeah I can do custom props as well potentially - i.e. there's a lot of prior art as well:
I think we may as well add it in here too, whilst you are I are both in this context etc. I'll do this, no worries:
|
- Close the unclosed delimiter in test_xlsx_workbook_properties_missing. - Add blank lines between WorkbookProperties fields for readability. - Rename PropField to DocProperty.
|
@jmcnamara for the We'd need to support:
so would you still want the type as |
|
Added
|
That is good work. :-)
In https://github.com/jmcnamara/rust_xlsxwriter/blob/main/src/properties.rs#L613-L621 However that is probably overkill. Custom properties will not be a requirement for most users and anyone who did need them probably wouldn't be concerned with the type or could probably figure it out from the field name. Hence my suggestion to just use However, even at that I don't think it is worth the effort of adding custom property support, at this time. A better use of your/my time would be to add property support for the other file types: xlsb, xls, and ods. I never insist that features are rolled out across all file types but it seems important that users be able to access the author and creation/modification data, at a minimum. Maybe you could look at xlsb. I can look at ODS (or vice-versa). @sftse would it be possible to extract basic property information like author and creation/modification from xls using the current cfb support? |
dca97b7 to
7442f0d
Compare
|
Updated
|
Alex-ley-scrub
changed the title
380f322 to
beb3e1e
Compare
Alex-ley-scrub
changed the title
|
Thanks. Your additions for custom property handling came at more or less the same time as my comments about omitting it. If I had seen the work first I would have left that go. However, splitting them out into a separate PR is good too. It makes this review a bit simpler. I'll review the current code but one high level comment. The current approach reads (or tries to read) the document properties for every xlsx/xlsb file (and later others) even if the user doesn't need them. That is a performance overhead, even if (relative to the data in the worksheets) it isn't a big one. A better approach might be to load the properties lazily when requested and cache them for repeated lookups (unlikely). Something like this: )$ git diff props-eager props-lazy src
diff --git a/src/lib.rs b/src/lib.rs
index fac7b06..40262de 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -226,8 +226,6 @@ pub struct Metadata {
sheets: Vec<Sheet>,
/// Map of sheet names/sheet path within zip archive
names: Vec<(String, String)>,
- /// Workbook properties (core and extended)
- pub(crate) workbook_properties: WorkbookProperties,
}
/// Workbook document properties.
@@ -293,13 +291,6 @@ pub struct WorkbookProperties {
pub manager: Option<String>,
}
-impl Metadata {
- /// Returns the workbook document properties.
- pub fn workbook_properties(&self) -> &WorkbookProperties {
- &self.workbook_properties
- }
-}
-
/// Type of sheet.
///
/// Only Excel formats support this. Default value for ODS is
diff --git a/src/ods.rs b/src/ods.rs
index dd98f74..05a2304 100644
--- a/src/ods.rs
+++ b/src/ods.rs
@@ -206,7 +206,6 @@ where
let metadata = Metadata {
sheets: sheets_metadata,
names: defined_names,
- workbook_properties: crate::WorkbookProperties::default(),
};
Ok(Ods {
diff --git a/src/xlsb/mod.rs b/src/xlsb/mod.rs
index dfb85a2..6faa72a 100644
--- a/src/xlsb/mod.rs
+++ b/src/xlsb/mod.rs
@@ -162,6 +162,8 @@ pub struct Xlsb<RS> {
formats: Vec<CellFormat>,
is_1904: bool,
metadata: Metadata,
+ /// Workbook document properties, parsed lazily on first access
+ workbook_properties: Option<Box<WorkbookProperties>>,
#[cfg(feature = "picture")]
pictures: Option<Vec<(String, Vec<u8>)>>,
options: XlsbOptions,
@@ -440,14 +442,28 @@ impl<RS: Read + Seek> Xlsb<RS> {
)
}
- fn read_properties(&mut self) -> Result<(), XlsbError> {
- let mut props = WorkbookProperties::default();
- read_core_properties(&mut self.zip, &mut props, &self.zip_path_cache)
- .map_err(xlsx_error_to_xlsb)?;
- read_app_properties(&mut self.zip, &mut props, &self.zip_path_cache)
- .map_err(xlsx_error_to_xlsb)?;
- self.metadata.workbook_properties = props;
- Ok(())
+ /// Get the workbook document properties.
+ ///
+ /// Reads the core [`WorkbookProperties`] document properties, such as file
+ /// author and creation date/time. Missing fields are set as `None`.
+ ///
+ /// # Errors
+ ///
+ /// - [`XlsbError::Xml`] if a `docProps` part is present but malformed.
+ ///
+ pub fn workbook_properties(&mut self) -> Result<&WorkbookProperties, XlsbError> {
+ if self.workbook_properties.is_none() {
+ let mut props = WorkbookProperties::default();
+ read_core_properties(&mut self.zip, &mut props, &self.zip_path_cache)
+ .map_err(xlsx_error_to_xlsb)?;
+ read_app_properties(&mut self.zip, &mut props, &self.zip_path_cache)
+ .map_err(xlsx_error_to_xlsb)?;
+ self.workbook_properties = Some(Box::new(props));
+ }
+ Ok(self
+ .workbook_properties
+ .as_deref()
+ .expect("populated immediately above"))
}
#[cfg(feature = "picture")]
@@ -495,6 +511,7 @@ impl<RS: Read + Seek> Reader<RS> for Xlsb<RS> {
formats: Vec::new(),
is_1904: false,
metadata: Metadata::default(),
+ workbook_properties: None,
#[cfg(feature = "picture")]
pictures: None,
options: XlsbOptions::default(),
@@ -504,7 +521,6 @@ impl<RS: Read + Seek> Reader<RS> for Xlsb<RS> {
xlsb.read_styles()?;
let relationships = xlsb.read_relationships()?;
xlsb.read_workbook(&relationships)?;
- xlsb.read_properties()?;
#[cfg(feature = "picture")]
xlsb.read_pictures()?;
diff --git a/src/xlsx/mod.rs b/src/xlsx/mod.rs
index c2728ea..d87d12e 100644
--- a/src/xlsx/mod.rs
+++ b/src/xlsx/mod.rs
@@ -265,6 +265,8 @@ pub struct Xlsx<RS> {
is_1904: bool,
/// Metadata
metadata: Metadata,
+ /// Workbook document properties, parsed lazily on first access.
+ workbook_properties: Option<Box<WorkbookProperties>>,
/// Pictures
#[cfg(feature = "picture")]
pictures: Option<Vec<Picture>>,
@@ -547,12 +549,26 @@ impl<RS: Read + Seek> Xlsx<RS> {
Ok(())
}
- fn read_properties(&mut self) -> Result<(), XlsxError> {
- let mut core = WorkbookProperties::default();
- read_core_properties(&mut self.zip, &mut core, &self.zip_path_cache)?;
- read_app_properties(&mut self.zip, &mut core, &self.zip_path_cache)?;
- self.metadata.workbook_properties = core;
- Ok(())
+ /// Get the workbook document properties.
+ ///
+ /// Reads the core [`WorkbookProperties`] document properties, such as file
+ /// author and creation date/time. Missing fields are set as `None`.
+ ///
+ /// # Errors
+ ///
+ /// - [`XlsxError::Xml`] if a `docProps` part is present but malformed.
+ ///
+ pub fn workbook_properties(&mut self) -> Result<&WorkbookProperties, XlsxError> {
+ if self.workbook_properties.is_none() {
+ let mut props = WorkbookProperties::default();
+ read_core_properties(&mut self.zip, &mut props, &self.zip_path_cache)?;
+ read_app_properties(&mut self.zip, &mut props, &self.zip_path_cache)?;
+ self.workbook_properties = Some(Box::new(props));
+ }
+ Ok(self
+ .workbook_properties
+ .as_deref()
+ .expect("populated immediately above"))
}
fn read_relationships(&mut self) -> Result<HashMap<Vec<u8>, (String, String)>, XlsxError> {
@@ -2560,6 +2576,7 @@ impl<RS: Read + Seek> Reader<RS> for Xlsx<RS> {
sheets: Vec::new(),
tables: None,
metadata: Metadata::default(),
+ workbook_properties: None,
#[cfg(feature = "picture")]
pictures: None,
merged_regions: None,
@@ -2571,7 +2588,6 @@ impl<RS: Read + Seek> Reader<RS> for Xlsx<RS> {
xlsx.read_styles()?;
let relationships = xlsx.read_relationships()?;
xlsx.read_workbook(&relationships)?;
- xlsx.read_properties()?;
#[cfg(feature = "picture")]
xlsx.read_pictures()?;
@@ -4577,6 +4593,7 @@ mod tests {
formats: vec![],
is_1904: false,
metadata: Metadata::default(),
+ workbook_properties: None,
#[cfg(feature = "picture")]
pictures: None,
merged_regions: None, |
Reuses the existing XLSX property parsers (now pub(crate)) since XLSB uses the same OPC XML package layout for document properties.
beb3e1e to
af315b4
Compare
|
I've addressed the lazy-loading feedback from #685 (comment) @jmcnamara Workbook properties are now loaded lazily on first access (and cached) instead of being parsed eagerly during reader initialization, and the access pattern is now I also opened the deferred custom-properties follow-up as a stacked PR on my fork: Alex-ley-scrub#1 - I can then open a PR from my fork into this repo once this PR here merges. Want/need me to do anything else @jmcnamara? |
| //! This example reads the core and extended document properties (such as | ||
| //! creator, application, and company) from a workbook. |
There was a problem hiding this comment.
Omit the phrase "core and extended" here and on any public docs. They won't mean anything to most end users.
| - `read_hyperlinks.rs`: Reads the hyperlinks defined in an XLSX worksheet, | ||
| either by sheet name or by sheet index. | ||
| - `read_picture_data.rs`: Reads pictures and their metadata from an XLSX file. | ||
| - `read_properties.rs`: Reads the workbook properties (core and extended) |
There was a problem hiding this comment.
Omit the phrase "core and extended" here.
|
@Alex-ley-scrub Thanks for the work here. Just one minor change required. |
| /// Get workbook document properties. | ||
| /// | ||
| /// Missing fields are returned as `None`. | ||
| pub fn workbook_properties(&mut self) -> Result<&WorkbookProperties, XlsbError> { |
There was a problem hiding this comment.
Thinking about this again, it is probably better to return WorkbookProperties via a clone() rather than the reference &WorkbookProperties which could keep the workbook owned and could prevent other method calls.
WorkbookProperties isn't one of the performance critical or heavyweight internal structures that needs to be returned as a ref. Side note, there are probably inconsistencies of this ref/value return usage in other places in the code but that is a separate issue.
|
@Alex-ley-scrub Ping on this. There are some requested changes outstanding. |
2 participants
Closes #677.
This PR adds support for reading workbook document properties from XLSX and XLSB files:
Core properties from
docProps/core.xml:creatorlast_modified_bycreatedmodifiedtitlesubjectdescriptionkeywordscategorycontent_statusrevisionversionExtended properties from
docProps/app.xml:applicationapp_versioncompanytemplatemanagerUsage:
If a file does not contain
docProps/core.xmlordocProps/app.xml, all properties remainNoneand parsing continues normally.Tests and fixtures covering both populated and missing properties are included.