Fix OPC part naming, leniency, and preserved relationships

Author: guseggert

crates/duke-sheets-chart/src/drawing_part/write.rs

@@ -133,6 +133,9 @@ pub struct PlannedRel {
     pub rel_type: String,
     pub target: String,
     pub external: bool,
+    /// Round-tripped from a raw entry rather than generated here, so its
+    /// target part may be absent if the source package lacked it.
+    pub preserved: bool,
 }
 
 /// Relationship-id assignments for a sheet's drawing part. Writer
@@ -233,6 +236,7 @@ pub fn plan_drawing_rels(
             rel_type: RT_IMAGE.to_string(),
             target: format!("../media/image{global_num}.{ext}"),
             external: false,
+            preserved: false,
         });
         plan.image_rids.push(rid);
     }
@@ -255,6 +259,7 @@ pub fn plan_drawing_rels(
                         rel_type: payload.0.clone(),
                         target: payload.1.clone(),
                         external: rel.external,
+                        preserved: true,
                     });
                     seen_raw.insert(fresh.clone(), payload);
                     remap.push((rel.id.to_string(), fresh));
@@ -265,6 +270,7 @@ pub fn plan_drawing_rels(
                         rel_type: payload.0.clone(),
                         target: payload.1.clone(),
                         external: rel.external,
+                        preserved: true,
                     });
                     seen_raw.insert(rel.id.to_string(), payload);
                 }
@@ -302,6 +308,7 @@ pub fn plan_drawing_rels(
                     rel_type: RT_CHART.to_string(),
                     target: format!("../charts/chart{}.xml", chart_globals[chart_i]),
                     external: false,
+                    preserved: false,
                 });
                 chart_i += 1;
                 plan.chart_rids.push(rid);
@@ -313,6 +320,7 @@ pub fn plan_drawing_rels(
                     rel_type: RT_CHART_EX.to_string(),
                     target: format!("../charts/chartEx{}.xml", chartex_globals[chartex_i]),
                     external: false,
+                    preserved: false,
                 });
                 chartex_i += 1;
                 plan.chartex_rids.push(rid);

crates/duke-sheets-xlsx/src/opc/diagnostics.rs

@@ -28,36 +28,62 @@ pub enum XlsxDiagnosticSeverity {
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 #[non_exhaustive]
 pub enum XlsxDiagnosticCode {
+    /// A ZIP entry or target breaks the OPC part-name rules.
     InvalidPartName,
+    /// Two entries differ only by ASCII case, so they name one part.
     EquivalentPartName,
+    /// One part name is a prefix of another, which OPC forbids.
     DerivablePartName,
+    /// A part has no `Default` or `Override` content type.
     MissingContentType,
+    /// An extension or part name is declared more than once.
     DuplicateContentType,
+    /// `[Content_Types].xml` is unreadable or does not match its schema.
     MalformedContentType,
+    /// A part that owns relationships has no `.rels` part.
     MissingRelationshipsPart,
+    /// A `Relationship` element is malformed or wrongly namespaced.
     MalformedRelationship,
+    /// Two relationships from one source share an `Id`.
     DuplicateRelationshipId,
+    /// `TargetMode` is neither `Internal` nor `External`, or is miscased.
     UnknownTargetMode,
+    /// An internal target could not be resolved to a part name.
     UnresolvedRelationshipTarget,
+    /// An internal target resolved but names no part in the package.
     MissingRelationshipTarget,
+    /// The package has no `/_rels/.rels`.
     MissingPackageRelationships,
+    /// Package relationships contain no `officeDocument` relationship.
     MissingOfficeDocumentRelationship,
+    /// More than one candidate Workbook part was found.
     AmbiguousOfficeDocumentRelationship,
+    /// The Workbook part's content type is not a SpreadsheetML workbook.
     WorkbookContentTypeMismatch,
+    /// A part's content type does not match its relationship type.
     PartContentTypeMismatch,
+    /// A part was located by convention because relationships were unusable.
     CanonicalPartFallback,
+    /// A valid sheet kind this library does not model, such as a dialog
+    /// or macro sheet, was skipped.
     UnsupportedSheetType,
 }
 
 /// A recoverable OPC package problem observed while reading an XLSX file.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
 pub struct XlsxDiagnostic {
+    /// Stable category, suitable for matching.
     pub code: XlsxDiagnosticCode,
+    /// Whether data was skipped or a fallback was applied.
     pub severity: XlsxDiagnosticSeverity,
+    /// Part that owns the problem, as an OPC part name.
     pub source_part: Option<String>,
+    /// Relationship id, when the problem is relationship-scoped.
     pub relationship_id: Option<String>,
+    /// Relationship target or part name the problem refers to.
     pub target: Option<String>,
+    /// Human-readable explanation; not stable across versions.
     pub message: String,
 }
 

crates/duke-sheets-xlsx/src/opc/package.rs

@@ -89,6 +89,15 @@ impl<R: Read + Seek> OpcPackage<R> {
                     continue;
                 }
             };
+            if let Some(problem) = part_name.conformance_error() {
+                diagnostics.warning(
+                    XlsxDiagnosticCode::InvalidPartName,
+                    problem,
+                    None,
+                    None,
+                    Some(&raw_name),
+                );
+            }
             if let Some(existing) = entries.get_mut(&part_name) {
                 diagnostics.violation(
                     XlsxDiagnosticCode::EquivalentPartName,
@@ -237,6 +246,14 @@ impl<R: Read + Seek> OpcPackage<R> {
                 .or_else(|| targets.iter().position(|target| target == &canonical))
                 .unwrap_or(0);
             let workbook = targets.remove(preferred);
+            // A relationship pointing at a non-workbook part is more
+            // likely wrong than the content types are, so let the
+            // content-type scan have the last word before giving up.
+            if !self.has_workbook_content_type(&workbook) {
+                if let Ok(fallback) = self.discover_workbook_fallback() {
+                    return Ok(fallback);
+                }
+            }
             self.validate_workbook_content_type(&workbook)?;
             return Ok(workbook);
         }

crates/duke-sheets-xlsx/src/opc/part_name.rs

@@ -9,7 +9,18 @@ pub(crate) struct PartName(String);
 impl PartName {
     pub(crate) fn new(value: impl Into<String>) -> XlsxResult<Self> {
         let value = value.into();
-        validate_part_name(&value)?;
+        validate_structure(&value)?;
+        validate_conformance(&value)?;
+        Ok(Self(value))
+    }
+
+    /// Accept a name that addresses a part unambiguously but breaks
+    /// ECMA-376 Part 2 §6.2.2.2 character rules, so Compatible mode can
+    /// still read parts that non-conforming producers emit (spaces,
+    /// brackets, redundant percent-encoding).
+    pub(crate) fn new_lenient(value: impl Into<String>) -> XlsxResult<Self> {
+        let value = value.into();
+        validate_structure(&value)?;
         Ok(Self(value))
     }
 
@@ -24,7 +35,18 @@ impl PartName {
             )));
         }
         let normalized = value.replace('\\', "/");
-        Self::new(format!("/{}", normalized.trim_start_matches('/')))
+        let name = format!("/{}", normalized.trim_start_matches('/'));
+        if compatible {
+            Self::new_lenient(name)
+        } else {
+            Self::new(name)
+        }
+    }
+
+    /// Describe how this name breaks the OPC character rules, for
+    /// diagnostics when it was accepted leniently.
+    pub(crate) fn conformance_error(&self) -> Option<String> {
+        validate_conformance(&self.0).err().map(|e| e.to_string())
     }
 
     pub(crate) fn as_str(&self) -> &str {
@@ -57,7 +79,9 @@ impl PartName {
         } else {
             format!("{parent}/_rels/{file}.rels")
         };
-        Self::new(path)
+        // Conformance was already decided for the owner; the added
+        // segments are always valid, so never re-reject here.
+        Self::new_lenient(path)
     }
 }
 
@@ -83,19 +107,33 @@ impl std::fmt::Display for PartName {
     }
 }
 
-fn validate_part_name(value: &str) -> XlsxResult<()> {
+/// Rules that must hold for a name to address one part unambiguously.
+/// Violating these is unrecoverable in any policy.
+fn validate_structure(value: &str) -> XlsxResult<()> {
     if value == "/" || !value.starts_with('/') || value.contains(['?', '#', '\\']) {
         return Err(XlsxError::InvalidFormat(format!(
             "invalid OPC part name: {value}"
         )));
     }
-
     for segment in value[1..].split('/') {
-        if segment.is_empty() || matches!(segment, "." | "..") || segment.ends_with('.') {
+        if segment.is_empty() || matches!(segment, "." | "..") {
             return Err(XlsxError::InvalidFormat(format!(
                 "invalid OPC part name segment in {value}"
             )));
         }
+    }
+    Ok(())
+}
+
+/// ECMA-376 Part 2 §6.2.2.2 character and encoding rules. Breaking these
+/// makes a package non-conforming without making the name ambiguous.
+fn validate_conformance(value: &str) -> XlsxResult<()> {
+    for segment in value[1..].split('/') {
+        if segment.ends_with('.') {
+            return Err(XlsxError::InvalidFormat(format!(
+                "OPC part name segment ends with a dot in {value}"
+            )));
+        }
         if segment.chars().any(|character| {
             character.is_control()
                 || character == ' '
@@ -236,7 +274,12 @@ pub(crate) fn resolve_internal_target_with_policy(
             "relationship target does not name a package part: target={target}"
         )));
     }
-    PartName::new(format!("/{}", parts.join("/")))
+    let resolved = format!("/{}", parts.join("/"));
+    if compatible {
+        PartName::new_lenient(resolved)
+    } else {
+        PartName::new(resolved)
+    }
 }
 
 fn decode_percent_encoded_unreserved(value: &str) -> String {

crates/duke-sheets-xlsx/src/reader/mod.rs

@@ -11,7 +11,8 @@ use quick_xml::reader::Reader;
 use crate::error::{XlsxError, XlsxResult};
 use crate::opc::{
     resolve_internal_target, ContentTypeExpectation, OpcPackage, PartName, Relationship,
-    RelationshipKind, RelationshipSet, XlsxDiagnosticCode, XlsxPackagePolicy,
+    RelationshipKind, RelationshipSet, XlsxDiagnosticCode, XlsxDiagnosticSeverity,
+    XlsxPackagePolicy,
 };
 use crate::styles::{
     read_styles_xml, register_roundtrip_style_data, register_roundtrip_theme_data, ParsedStyles,
@@ -215,9 +216,9 @@ fn resolve_pic_image<R: Read + Seek>(
     };
     let image_rel_id = image.media_path.clone();
     if let Some(rel) = drawing_rels.get(&image_rel_id) {
-        let Some(path) = rel.internal_path() else {
-            return Ok(image);
-        };
+        // An external or unresolvable target keeps its raw URI so the
+        // model carries the link rather than a stale relationship id.
+        let path = rel.internal_path().unwrap_or_else(|| rel.target());
         let ext = path.rsplit('.').next().unwrap_or("");
         if let Some(fmt) = duke_sheets_chart::ImageFormat::from_extension(ext) {
             image.format = fmt;
@@ -232,9 +233,7 @@ fn resolve_pic_image<R: Read + Seek>(
     }
     if let Some(svg_rel_id) = image.svg_media_path.clone() {
         if let Some(rel) = drawing_rels.get(svg_rel_id.as_str()) {
-            let Some(path) = rel.internal_path() else {
-                return Ok(image);
-            };
+            let path = rel.internal_path().unwrap_or_else(|| rel.target());
             image.svg_media_path = Some(path.to_string());
             if let Some(mut f) = package.open_related_part(rel)? {
                 let mut buf = Vec::new();
@@ -487,7 +486,17 @@ pub struct XlsxReader;
 
 fn log_package_diagnostics(diagnostics: &[crate::opc::XlsxDiagnostic]) {
     for diagnostic in diagnostics {
-        log::warn!("{}", diagnostic.message);
+        let code = diagnostic.code;
+        let source = diagnostic.source_part.as_deref().unwrap_or("/");
+        let message = &diagnostic.message;
+        match diagnostic.severity {
+            XlsxDiagnosticSeverity::Recovery => {
+                log::info!("{code:?} in {source}: {message}");
+            }
+            XlsxDiagnosticSeverity::Warning => {
+                log::warn!("{code:?} in {source}: {message}");
+            }
+        }
     }
 }