fix: preserve source resources during document path saves

CHANGELOG.md

@@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Document path saves preserve the destination on write failure and retain
+  lazy resources when saving repeatedly over the input. Existing permissions
+  are preserved; temporary copies are private to their owner.
+
 - PDF calculator functions distinguish booleans, integers and reals; correct
   bit shifts and rounding; and reject invalid arithmetic and operand types.
 

docs/design/editing.md

@@ -110,8 +110,16 @@ different feature and belongs to `<text:tracked-changes>`, `w:ins` and `w:del`.
 
 The browser refuses edits for UX only. Replay validates again and throws on an
 op it cannot apply. Earlier operations remain applied when a later one fails;
-replay does not clone or roll back the document. Saving is a separate call and
-is not atomic. The envelope states a `version`, and replay refuses any other.
+replay does not clone or roll back the document. The envelope states a
+`version`, and replay refuses any other.
+
+Saving is a separate call. Path saves write a temporary file beside the
+destination and replace it after a successful close. Before that, the
+document's `release_source` moves a ZIP package that reads from the
+destination onto a private copy, so lazy resource reads and later saves keep
+working. Only the saving document is moved: a second document open on the
+same file is not, and a hard link to the destination keeps the old bytes.
+Stream saves leave partial output on failure.
 
 Open: a conformance corpus of `(base document, op log) → expected saved
 result` cases, replayed in C++ as a GoogleTest, is not built. What stands in

src/odr/document.cpp

@@ -49,22 +49,27 @@ bool Document::is_savable(const bool encrypted) const noexcept {
 // Every overload checks before it writes, so an unsavable format leaves no
 // empty file behind and no half-written stream.
 void Document::save(const std::string &path) const {
-  if (!m_impl->is_savable(false)) {
-    throw UnsupportedOperation();
-  }
-  recalculate_edits_();
-  std::ofstream out = internal::util::file::create(path);
-  m_impl->save(out);
+  save_file_(path, nullptr);
 }
 
 void Document::save(const std::string &path,
                     const std::string &password) const {
-  if (!m_impl->is_savable(true)) {
+  save_file_(path, password.c_str());
+}
+
+void Document::save_file_(const std::string &path, const char *password) const {
+  if (!m_impl->is_savable(password != nullptr)) {
     throw UnsupportedOperation();
   }
   recalculate_edits_();
-  std::ofstream out = internal::util::file::create(path);
-  m_impl->save(out, password.c_str());
+  m_impl->release_source(internal::AbsPath(std::filesystem::absolute(path)));
+  internal::util::file::write_atomic(path, [&](std::ostream &out) {
+    if (password == nullptr) {
+      m_impl->save(out);
+    } else {
+      m_impl->save(out, password);
+    }
+  });
 }
 
 void Document::save(std::ostream &out) const {

src/odr/document.hpp

@@ -61,6 +61,7 @@ class Document final {
   /// out in the clear.
   [[nodiscard]] bool is_savable(bool encrypted = false) const noexcept;
 
+  /// Replaces the destination only after writing and closing successfully.
   void save(const std::string &path) const;
   void save(const std::string &path, const std::string &password) const;
 
@@ -161,6 +162,7 @@ class Document final {
 
   /// Recalculates formulas made stale by edits before saving.
   void recalculate_edits_() const;
+  void save_file_(const std::string &path, const char *password) const;
 
   /// @p element 's identifier, checked to be one this document holds.
   [[nodiscard]] ElementIdentifier check_(const Element &element) const;

src/odr/internal/abstract/document.hpp

@@ -32,6 +32,7 @@ struct GraphicStyle;
 } // namespace odr
 
 namespace odr::internal {
+class AbsPath;
 class SheetDependencies;
 } // namespace odr::internal
 
@@ -77,6 +78,9 @@ class Document {
 
   virtual void save(std::ostream &out) const = 0;
   virtual void save(std::ostream &out, const char *password) const = 0;
+  /// Called before a save replaces the file at @p path: whatever still reads
+  /// from that file reads from a private copy from then on.
+  virtual void release_source(const AbsPath & /*path*/) const {}
 
   [[nodiscard]] virtual FileType file_type() const noexcept = 0;
   [[nodiscard]] virtual DocumentType document_type() const noexcept = 0;

src/odr/internal/common/document.cpp

@@ -5,14 +5,17 @@
 #include <odr/internal/abstract/filesystem.hpp>
 #include <odr/internal/common/sheet_dependencies.hpp>
 #include <odr/internal/formula/formula_value.hpp>
+#include <odr/internal/zip/zip_util.hpp>
 
 namespace odr::internal {
 
 Document::Document(const FileType file_type, const DocumentType document_type,
                    std::shared_ptr<abstract::ReadableFilesystem> files,
-                   const EncryptionState encryption_state)
+                   const EncryptionState encryption_state,
+                   std::shared_ptr<zip::util::Archive> archive)
     : m_file_type{file_type}, m_document_type{document_type},
-      m_encryption_state{encryption_state}, m_files{std::move(files)} {}
+      m_encryption_state{encryption_state}, m_files{std::move(files)},
+      m_archive{std::move(archive)} {}
 
 Document::~Document() = default;
 
@@ -24,6 +27,12 @@ bool Document::is_savable(const bool /*encrypted*/) const noexcept {
   return false;
 }
 
+void Document::release_source(const AbsPath &path) const {
+  if (m_archive != nullptr) {
+    m_archive->release_source(path);
+  }
+}
+
 void Document::save(std::ostream & /*out*/) const {
   throw UnsupportedOperation();
 }

src/odr/internal/common/document.hpp

@@ -11,6 +11,10 @@
 #include <utility>
 #include <vector>
 
+namespace odr::internal::zip::util {
+class Archive;
+} // namespace odr::internal::zip::util
+
 namespace odr::internal::abstract {
 class ReadableFilesystem;
 } // namespace odr::internal::abstract
@@ -21,16 +25,19 @@ class SheetDependencies;
 
 class Document : public abstract::Document {
 public:
+  /// @p archive is the package @p files reads from lazily, if any.
   Document(FileType file_type, DocumentType document_type,
            std::shared_ptr<abstract::ReadableFilesystem> files,
-           EncryptionState encryption_state = EncryptionState::not_encrypted);
+           EncryptionState encryption_state = EncryptionState::not_encrypted,
+           std::shared_ptr<zip::util::Archive> archive = nullptr);
   ~Document() override;
 
   /// Read-only, which every engine but odf and ooxml text is.
   [[nodiscard]] bool is_editable() const noexcept override;
   [[nodiscard]] bool is_savable(bool encrypted) const noexcept override;
   void save(std::ostream &out) const override;
   void save(std::ostream &out, const char *password) const override;
+  void release_source(const AbsPath &path) const final;
 
   [[nodiscard]] FileType file_type() const noexcept final;
   [[nodiscard]] DocumentType document_type() const noexcept final;
@@ -73,6 +80,7 @@ class Document : public abstract::Document {
   EncryptionState m_encryption_state{EncryptionState::not_encrypted};
 
   std::shared_ptr<abstract::ReadableFilesystem> m_files;
+  std::shared_ptr<zip::util::Archive> m_archive;
 
   ElementIdentifier m_root_element{null_element_id};
   std::unique_ptr<abstract::ElementAdapter> m_element_adapter;

src/odr/internal/common/temporary_file.cpp

@@ -6,7 +6,9 @@
 #include <odr/internal/util/stream_util.hpp>
 
 #include <cstdint>
+#include <filesystem>
 #include <fstream>
+#include <system_error>
 #include <utility>
 
 namespace odr::internal {
@@ -56,6 +58,24 @@ TemporaryDiskFile::operator=(TemporaryDiskFile &&other) noexcept {
   return *this;
 }
 
+void TemporaryDiskFile::persist(const AbsPath &target) {
+  // a missing target is no error: it has no permissions to keep
+  std::error_code missing;
+  const std::filesystem::file_status status =
+      std::filesystem::status(target.path(), missing);
+  std::error_code error;
+  if (std::filesystem::exists(status)) {
+    std::filesystem::permissions(path().path(), status.permissions(), error);
+  }
+  if (!error) {
+    std::filesystem::rename(path().path(), target.path(), error);
+  }
+  if (error) {
+    throw FileWriteError(target.string());
+  }
+  m_owns_path = false;
+}
+
 const TemporaryDiskFileFactory &TemporaryDiskFileFactory::system_default() {
   static TemporaryDiskFileFactory instance(
       AbsPath(std::filesystem::temp_directory_path()),
@@ -79,6 +99,17 @@ TemporaryDiskFileFactory::copy(const abstract::File &file) const {
 }
 
 TemporaryDiskFile TemporaryDiskFileFactory::copy(std::istream &in) const {
+  return create_([&](std::ostream &out) { util::stream::pipe(in, out); }, true);
+}
+
+TemporaryDiskFile TemporaryDiskFileFactory::create(
+    const std::function<void(std::ostream &)> &write) const {
+  return create_(write, false);
+}
+
+TemporaryDiskFile TemporaryDiskFileFactory::create_(
+    const std::function<void(std::ostream &)> &write,
+    const bool private_to_owner) const {
   std::ofstream file;
   AbsPath file_path;
   for (std::uint32_t attempt = 0; attempt < 128; ++attempt) {
@@ -98,12 +129,18 @@ TemporaryDiskFile TemporaryDiskFileFactory::copy(std::istream &in) const {
   }
 
   try {
-    util::stream::pipe(in, file);
+    if (private_to_owner) {
+      std::filesystem::permissions(file_path.path(),
+                                   std::filesystem::perms::owner_read |
+                                       std::filesystem::perms::owner_write);
+    }
+    write(file);
     file.close();
     if (!file) {
       throw FileWriteError(file_path.string());
     }
   } catch (...) {
+    file.exceptions(std::ios::goodbit);
     file.close();
     remove_quietly(file_path);
     throw;

src/odr/internal/common/temporary_file.hpp

@@ -19,6 +19,10 @@ class TemporaryDiskFile final : public DiskFile {
   TemporaryDiskFile &operator=(const TemporaryDiskFile &) = delete;
   TemporaryDiskFile &operator=(TemporaryDiskFile &&) noexcept;
 
+  /// Replaces @p target with this file, keeping the permissions @p target
+  /// has, and leaves it in place from then on.
+  void persist(const AbsPath &target);
+
 private:
   bool m_owns_path{true}; ///< cleared by a move, so only one owner removes
 };
@@ -34,10 +38,18 @@ class TemporaryDiskFileFactory final {
       AbsPath directory, RandomFileNameGenerator random_file_name_generator =
                              default_random_file_name_generator());
 
+  /// Private to the owner, because a copy can hold decrypted content.
   [[nodiscard]] TemporaryDiskFile copy(const abstract::File &file) const;
   [[nodiscard]] TemporaryDiskFile copy(std::istream &in) const;
+  /// With the permissions any new file in the directory gets.
+  [[nodiscard]] TemporaryDiskFile
+  create(const std::function<void(std::ostream &)> &write) const;
 
 private:
+  [[nodiscard]] TemporaryDiskFile
+  create_(const std::function<void(std::ostream &)> &write,
+          bool private_to_owner) const;
+
   AbsPath m_directory;
   RandomFileNameGenerator m_random_file_name_generator;
 };

src/odr/internal/odf/odf_document.cpp

@@ -53,9 +53,10 @@ create_element_adapter(Document &document, ElementRegistry &registry);
 
 Document::Document(const FileType file_type, const DocumentType document_type,
                    std::shared_ptr<abstract::ReadableFilesystem> files,
-                   const EncryptionState encryption_state)
+                   const EncryptionState encryption_state,
+                   std::shared_ptr<zip::util::Archive> archive)
     : internal::Document(file_type, document_type, std::move(files),
-                         encryption_state) {
+                         encryption_state, std::move(archive)) {
   m_content_xml = xml::parse(*m_files, AbsPath("/content.xml"));
 
   if (m_files->exists(AbsPath("/styles.xml"))) {

src/odr/internal/odf/odf_document.hpp

@@ -16,7 +16,8 @@ class Document final : public internal::Document {
 public:
   Document(FileType file_type, DocumentType document_type,
            std::shared_ptr<abstract::ReadableFilesystem> files,
-           EncryptionState encryption_state);
+           EncryptionState encryption_state,
+           std::shared_ptr<zip::util::Archive> archive = nullptr);
   /// A flat document: one tree holding both content and styles, no filesystem.
   Document(FileType file_type, DocumentType document_type,
            pugi::xml_document flat_xml);

src/odr/internal/odf/odf_file.cpp

@@ -15,8 +15,9 @@ class File;
 namespace odr::internal::odf {
 
 OpenDocumentFile::OpenDocumentFile(
-    std::shared_ptr<abstract::ReadableFilesystem> filesystem)
-    : m_filesystem{std::move(filesystem)} {
+    std::shared_ptr<abstract::ReadableFilesystem> filesystem,
+    std::shared_ptr<zip::util::Archive> archive)
+    : m_filesystem{std::move(filesystem)}, m_archive{std::move(archive)} {
   if (m_filesystem->exists(AbsPath("/META-INF/manifest.xml"))) {
     const pugi::xml_document manifest =
         xml::parse(*m_filesystem, AbsPath("/META-INF/manifest.xml"));
@@ -77,6 +78,7 @@ OpenDocumentFile::decrypt(const std::string &password) const {
   auto decrypted_filesystem = odf::decrypt(m_filesystem, m_manifest, password);
   auto decrypted_file = std::make_shared<OpenDocumentFile>(*this);
   decrypted_file->m_filesystem = std::move(decrypted_filesystem);
+  decrypted_file->m_archive = nullptr;
   decrypted_file->m_file_meta =
       parse_file_meta(*decrypted_file->m_filesystem, nullptr, true);
   decrypted_file->m_encryption_state = EncryptionState::decrypted;
@@ -95,18 +97,20 @@ std::shared_ptr<abstract::Document> OpenDocumentFile::document() const {
   switch (file_type()) {
   case FileType::opendocument_text:
     return std::make_shared<Document>(m_file_meta.type, DocumentType::text,
-                                      m_filesystem, m_encryption_state);
+                                      m_filesystem, m_encryption_state,
+                                      m_archive);
   case FileType::opendocument_presentation:
     return std::make_shared<Document>(m_file_meta.type,
                                       DocumentType::presentation, m_filesystem,
-                                      m_encryption_state);
+                                      m_encryption_state, m_archive);
   case FileType::opendocument_spreadsheet:
     return std::make_shared<Document>(m_file_meta.type,
                                       DocumentType::spreadsheet, m_filesystem,
-                                      m_encryption_state);
+                                      m_encryption_state, m_archive);
   case FileType::opendocument_graphics:
     return std::make_shared<Document>(m_file_meta.type, DocumentType::drawing,
-                                      m_filesystem, m_encryption_state);
+                                      m_filesystem, m_encryption_state,
+                                      m_archive);
   default:
     throw UnsupportedFileType(file_type());
   }

src/odr/internal/odf/odf_file.hpp

@@ -16,12 +16,18 @@ namespace odr::internal::abstract {
 class ReadableFilesystem;
 }
 
+namespace odr::internal::zip::util {
+class Archive;
+} // namespace odr::internal::zip::util
+
 namespace odr::internal::odf {
 
 class OpenDocumentFile final : public virtual abstract::DocumentFile {
 public:
+  /// @p archive is the package @p filesystem reads from lazily, if any.
   explicit OpenDocumentFile(
-      std::shared_ptr<abstract::ReadableFilesystem> filesystem);
+      std::shared_ptr<abstract::ReadableFilesystem> filesystem,
+      std::shared_ptr<zip::util::Archive> archive = nullptr);
 
   [[nodiscard]] std::shared_ptr<abstract::File> file() const noexcept override;
 
@@ -44,6 +50,7 @@ class OpenDocumentFile final : public virtual abstract::DocumentFile {
 
 private:
   std::shared_ptr<abstract::ReadableFilesystem> m_filesystem;
+  std::shared_ptr<zip::util::Archive> m_archive;
   EncryptionState m_encryption_state{EncryptionState::not_encrypted};
   FileMeta m_file_meta;
   Manifest m_manifest;