fix: handle empty public handles and related elements

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Empty file and HTML handles throw instead of dereferencing null.
+  Missing master pages and table rows/columns return empty element handles.
+
 - Copied filesystem walkers keep independent traversal positions on disk,
   matching walkers over archives and memory filesystems.
 

src/odr/document.cpp

@@ -478,6 +478,9 @@ Element Document::root_element() const {
 
 Element Document::element_by_id(const ElementIdentifier identifier) const {
   const internal::abstract::ElementAdapter *adapter = m_impl->element_adapter();
+  if (adapter == nullptr || identifier == null_element_id) {
+    return {};
+  }
   try {
     // throwing is how a registry answers an id it does not hold
     static_cast<void>(adapter->element_type(identifier));
@@ -488,7 +491,7 @@ Element Document::element_by_id(const ElementIdentifier identifier) const {
 }
 
 ElementIdentifier Document::check_(const Element &element) const {
-  if (element_by_id(element.identifier()) != element) {
+  if (!element || element_by_id(element.identifier()) != element) {
     throw std::invalid_argument("element is not this document's");
   }
   return element.identifier();

src/odr/document_element.cpp

@@ -341,8 +341,7 @@ MasterPage TextRoot::first_master_page() const {
   }
   const ElementIdentifier master_page_id =
       m_adapter2->text_root_first_master_page(m_identifier);
-  return {m_adapter, master_page_id,
-          m_adapter->master_page_adapter(master_page_id)};
+  return Element(m_adapter, master_page_id).as_master_page();
 }
 
 std::string Slide::name() const {
@@ -359,8 +358,7 @@ MasterPage Slide::master_page() const {
   }
   const ElementIdentifier master_page_id =
       m_adapter2->slide_master_page(m_identifier);
-  return {m_adapter, master_page_id,
-          m_adapter->master_page_adapter(master_page_id)};
+  return Element(m_adapter, master_page_id).as_master_page();
 }
 
 std::string Sheet::name() const {
@@ -569,8 +567,7 @@ MasterPage Page::master_page() const {
   }
   const ElementIdentifier master_page_id =
       m_adapter2->page_master_page(m_identifier);
-  return {m_adapter, master_page_id,
-          m_adapter->master_page_adapter(master_page_id)};
+  return Element(m_adapter, master_page_id).as_master_page();
 }
 
 PageLayout MasterPage::page_layout() const {
@@ -668,7 +665,7 @@ TableRow Table::first_row() const {
     return {};
   }
   const ElementIdentifier row_id = m_adapter2->table_first_row(m_identifier);
-  return {m_adapter, row_id, m_adapter->table_row_adapter(row_id)};
+  return Element(m_adapter, row_id).as_table_row();
 }
 
 TableColumn Table::first_column() const {
@@ -677,7 +674,7 @@ TableColumn Table::first_column() const {
   }
   const ElementIdentifier column_id =
       m_adapter2->table_first_column(m_identifier);
-  return {m_adapter, column_id, m_adapter->table_column_adapter(column_id)};
+  return Element(m_adapter, column_id).as_table_column();
 }
 
 ElementRange Table::columns() const {

src/odr/html.cpp

@@ -29,6 +29,13 @@ namespace odr {
 
 namespace {
 
+template <typename T> const T &deref(const std::shared_ptr<T> &impl) {
+  if (impl == nullptr) {
+    throw NullPointerError("HTML handle");
+  }
+  return *impl;
+}
+
 void bring_offline(const HtmlResources &resources,
                    const std::string &output_path) {
   for (const auto &[resource, location] : resources) {
@@ -70,32 +77,34 @@ HtmlPage::HtmlPage(std::string name, std::string path)
 HtmlService::HtmlService() = default;
 
 HtmlService::HtmlService(std::shared_ptr<abstract::HtmlService> impl)
-    : m_impl{std::move(impl)} {}
+    : m_impl{std::move(impl)} {
+  static_cast<void>(deref(m_impl));
+}
 
-const HtmlConfig &HtmlService::config() const { return m_impl->config(); }
+const HtmlConfig &HtmlService::config() const { return deref(m_impl).config(); }
 
 const HtmlViews &HtmlService::list_views() const {
-  return m_impl->list_views();
+  return deref(m_impl).list_views();
 }
 
-void HtmlService::warmup() const { m_impl->warmup(); }
+void HtmlService::warmup() const { deref(m_impl).warmup(); }
 
 bool HtmlService::exists(const std::string &path) const {
-  return m_impl->exists(path);
+  return deref(m_impl).exists(path);
 }
 
 std::string HtmlService::mimetype(const std::string &path) const {
-  return m_impl->mimetype(path);
+  return deref(m_impl).mimetype(path);
 }
 
 void HtmlService::write(const std::string &path, std::ostream &out) const {
-  m_impl->write(path, out);
+  deref(m_impl).write(path, out);
 }
 
 HtmlResources HtmlService::write_html(const std::string &path,
                                       std::ostream &out) const {
   internal::html::HtmlWriter writer(out, config());
-  return m_impl->write_html(path, writer);
+  return deref(m_impl).write_html(path, writer);
 }
 
 Html HtmlService::bring_offline(const std::string &output_path) const {
@@ -144,23 +153,25 @@ const std::shared_ptr<abstract::HtmlService> &HtmlService::impl() const {
 HtmlView::HtmlView() = default;
 
 HtmlView::HtmlView(std::shared_ptr<abstract::HtmlView> impl)
-    : m_impl{std::move(impl)} {}
+    : m_impl{std::move(impl)} {
+  static_cast<void>(deref(m_impl));
+}
 
-const std::string &HtmlView::name() const { return m_impl->name(); }
+const std::string &HtmlView::name() const { return deref(m_impl).name(); }
 
-std::size_t HtmlView::index() const { return m_impl->index(); }
+std::size_t HtmlView::index() const { return deref(m_impl).index(); }
 
-const std::string &HtmlView::path() const { return m_impl->path(); }
+const std::string &HtmlView::path() const { return deref(m_impl).path(); }
 
-const HtmlConfig &HtmlView::config() const { return m_impl->config(); }
+const HtmlConfig &HtmlView::config() const { return deref(m_impl).config(); }
 
 const std::optional<HtmlSheetCut> &HtmlView::sheet_cut() const {
-  return m_impl->sheet_cut();
+  return deref(m_impl).sheet_cut();
 }
 
 HtmlResources HtmlView::write_html(std::ostream &out) const {
   internal::html::HtmlWriter writer(out, config());
-  return m_impl->write_html(writer);
+  return deref(m_impl).write_html(writer);
 }
 
 Html HtmlView::bring_offline(const std::string &output_path) const {
@@ -186,28 +197,34 @@ const std::shared_ptr<abstract::HtmlView> &HtmlView::impl() const {
 HtmlResource::HtmlResource() = default;
 
 HtmlResource::HtmlResource(std::shared_ptr<abstract::HtmlResource> impl)
-    : m_impl{std::move(impl)} {}
+    : m_impl{std::move(impl)} {
+  static_cast<void>(deref(m_impl));
+}
 
-HtmlResourceType HtmlResource::type() const { return m_impl->type(); }
+HtmlResourceType HtmlResource::type() const { return deref(m_impl).type(); }
 
 const std::string &HtmlResource::mime_type() const {
-  return m_impl->mime_type();
+  return deref(m_impl).mime_type();
 }
 
-const std::string &HtmlResource::name() const { return m_impl->name(); }
+const std::string &HtmlResource::name() const { return deref(m_impl).name(); }
 
-const std::string &HtmlResource::path() const { return m_impl->path(); }
+const std::string &HtmlResource::path() const { return deref(m_impl).path(); }
 
-const std::optional<File> &HtmlResource::file() const { return m_impl->file(); }
+const std::optional<File> &HtmlResource::file() const {
+  return deref(m_impl).file();
+}
 
-bool HtmlResource::is_shipped() const { return m_impl->is_shipped(); }
+bool HtmlResource::is_shipped() const { return deref(m_impl).is_shipped(); }
 
-bool HtmlResource::is_external() const { return m_impl->is_external(); }
+bool HtmlResource::is_external() const { return deref(m_impl).is_external(); }
 
-bool HtmlResource::is_accessible() const { return m_impl->is_accessible(); }
+bool HtmlResource::is_accessible() const {
+  return deref(m_impl).is_accessible();
+}
 
 void HtmlResource::write_resource(std::ostream &os) const {
-  m_impl->write_resource(os);
+  deref(m_impl).write_resource(os);
 }
 
 namespace {

src/odr/html.hpp

@@ -43,6 +43,7 @@ enum class HtmlResourceType {
 /// font. @ref HtmlConfig decides which are embedded and which are linked.
 class HtmlResource final {
 public:
+  /// Empty handle; accessing it throws `NullPointerError`.
   HtmlResource();
   explicit HtmlResource(std::shared_ptr<internal::abstract::HtmlResource> impl);
 
@@ -276,6 +277,7 @@ struct HtmlSheetCut final {
 /// needs.
 class HtmlView final {
 public:
+  /// Empty handle; accessing it throws `NullPointerError`.
   HtmlView();
   explicit HtmlView(std::shared_ptr<internal::abstract::HtmlView> impl);
 
@@ -306,6 +308,7 @@ using HtmlViews = std::vector<HtmlView>;
 /// puts on a socket.
 class HtmlService final {
 public:
+  /// Empty handle; accessing it throws `NullPointerError`.
   HtmlService();
   explicit HtmlService(std::shared_ptr<internal::abstract::HtmlService> impl);
 

src/odr/internal/open_strategy.cpp

@@ -326,6 +326,9 @@ open_file_as(const std::shared_ptr<abstract::File> &file, const FileType as,
 std::vector<FileType>
 open_strategy::list_file_types(const std::shared_ptr<abstract::File> &file,
                                const Logger &logger) {
+  if (file == nullptr) {
+    throw NullPointerError("file");
+  }
   std::vector<FileType> result;
 
   auto file_type = magic::file_type(*file);
@@ -659,6 +662,9 @@ open_by_probe(const std::shared_ptr<abstract::File> &file,
 std::unique_ptr<abstract::DecodedFile>
 open_strategy::open_file(const std::shared_ptr<abstract::File> &file,
                          const DecodeOptions &options, const Logger &logger) {
+  if (file == nullptr) {
+    throw NullPointerError("file");
+  }
   // A named type is not a probe with one candidate: there is nothing to move
   // on to, so the format's own "not a ..." reaches the caller rather than
   // being collapsed into UnknownFileType.

test/src/file_test.cpp

@@ -250,6 +250,15 @@ TEST(File, default_constructed_reports_unknown_location) {
   EXPECT_THROW(std::ignore = file.stream(), NullPointerError);
 }
 
+TEST(File, opening_or_probing_an_empty_handle_throws) {
+  const File file;
+  EXPECT_THROW(std::ignore = open(file), NullPointerError);
+  EXPECT_THROW(std::ignore = open(file, DecodeOptions::as(FileType::text_file)),
+               NullPointerError);
+  EXPECT_THROW(std::ignore = list_file_types(file), NullPointerError);
+  EXPECT_THROW(std::ignore = mimetype(file), NullPointerError);
+}
+
 TEST(File, disk_file_has_no_memory_data) {
   const File file(std::make_shared<internal::DiskFile>(
       TestData::test_file_path("odr-public/odt/about.odt")));

test/src/html_test.cpp

@@ -26,6 +26,15 @@ using namespace odr;
 using namespace odr::internal;
 using namespace odr::test;
 
+TEST(html, empty_handles_throw_on_access) {
+  EXPECT_THROW(std::ignore = HtmlService().config(), NullPointerError);
+  EXPECT_THROW(std::ignore = HtmlView().name(), NullPointerError);
+  EXPECT_THROW(std::ignore = HtmlResource().type(), NullPointerError);
+  EXPECT_THROW(HtmlService(nullptr), NullPointerError);
+  EXPECT_THROW(HtmlView(nullptr), NullPointerError);
+  EXPECT_THROW(HtmlResource(nullptr), NullPointerError);
+}
+
 // A linked stylesheet is of no use to a host serving the service over http if
 // the service cannot answer for the path the markup names.
 TEST(html, linked_resources_are_served) {

test/src/internal/odf/odf_flat_file_test.cpp

@@ -82,6 +82,20 @@ std::string packaged_text(const std::string &name, const std::string &body) {
 
 } // namespace
 
+TEST(FlatOdf, missing_related_elements_return_empty_handles) {
+  const Document document = open(File::from_memory(flat_text("<table:table/>")))
+                                .as_document_file()
+                                .document();
+  EXPECT_FALSE(document.root_element().as_text_root().first_master_page());
+  const Table table =
+      first_of_type(document.root_element(), ElementType::table).as_table();
+  ASSERT_TRUE(table);
+  EXPECT_FALSE(table.first_row());
+  EXPECT_FALSE(table.first_column());
+  EXPECT_FALSE(document.element_by_id(null_element_id));
+  EXPECT_THROW(document.remove(Element()), std::invalid_argument);
+}
+
 TEST(FlatOpenDocumentFile, the_root_mimetype_names_the_document_type) {
   const struct {
     const char *mimetype;