Merge pull request #1585 from m-holger/dict

Refactor private-API dictionary methods

Merge pull request #1585 from m-holger/dict
Refactor private-API dictionary methods
m-holger · GitHub
2 parents c290a82f fcc7f702
Showing 14 changed files with 202 additions and 52 deletions
include/qpdf/ObjectHandle.hh
libqpdf/NNTree.cc
libqpdf/QPDF.cc
libqpdf/QPDFAcroFormDocumentHelper.cc
libqpdf/QPDFAnnotationObjectHelper.cc
libqpdf/QPDFEFStreamObjectHelper.cc
libqpdf/QPDFFileSpecObjectHelper.cc
libqpdf/QPDFFormFieldObjectHelper.cc
libqpdf/QPDFOutlineObjectHelper.cc
libqpdf/QPDF_Dictionary.cc
libqpdf/qpdf/QPDFObjectHandle_private.hh
libtests/objects.cc
libtests/qtest/objects.test
qpdf/qpdf.testcov
@@ -83,8 +83,11 @@ namespace qpdf
         QPDFObjectHandle operator[](size_t n) const;
         QPDFObjectHandle operator[](int n) const;
+        QPDFObjectHandle& at(std::string const& key) const;
         bool contains(std::string const& key) const;
         size_t erase(std::string const& key);
+        QPDFObjectHandle& find(std::string const& key) const;
+        bool replace(std::string const& key, QPDFObjectHandle value);
         QPDFObjectHandle const& operator[](std::string const& key) const;
         std::shared_ptr<QPDFObject> copy(bool shallow = false) const;
@@ -134,6 +137,8 @@ namespace qpdf
         std::string description() const;
+        inline QPDFObjectHandle const& get(std::string const& key) const;
+
         void no_ci_warn_if(bool condition, std::string const& warning) const;
         void no_ci_stop_if(bool condition, std::string const& warning) const;
         void no_ci_stop_damaged_if(bool condition, std::string const& warning) const;
@@ -168,7 +168,7 @@ NNTreeIterator::resetLimits(Dictionary a_node, std::list&lt;PathElement&gt;::iterator 
                 }
             }
             if (a_node != path.begin()->node) {
-                a_node.replaceKey("/Limits", Array({first, last}));
+                a_node.replace("/Limits", Array({first, last}));
             }
         }
@@ -266,7 +266,7 @@ NNTreeIterator::split(Dictionary to_split, std::list&lt;PathElement&gt;::iterator pare
         new_kids.push_back(first_node);
         to_split.erase("/Limits"); // already shouldn't be there for root
         to_split.erase(impl.itemsKey());
-        to_split.replaceKey("/Kids", new_kids);
+        to_split.replace("/Kids", new_kids);
         if (is_leaf) {
             node = first_node;
         } else {
@@ -446,7 +446,7 @@ NNTreeIterator::remove()
         if (parent == path.end()) {
             // We erased the very last item. Convert the root to an empty items array.
             element->node.erase("/Kids");
-            element->node.replaceKey(impl.itemsKey(), Array::empty());
+            element->node.replace(impl.itemsKey(), Array::empty());
             path.clear();
             setItemNumber(impl.tree_root, -1);
             return;
@@ -673,8 +673,8 @@ NNTreeImpl::repair()
     for (auto const& [key, value]: items) {
         repl.insert(key, value);
     }
-    tree_root.replaceKey("/Kids", new_node["/Kids"]);
-    tree_root.replaceKey(itemsKey(), new_node[itemsKey()]);
+    tree_root.replace("/Kids", new_node["/Kids"]);
+    tree_root.replace(itemsKey(), new_node[itemsKey()]);
 }
 NNTreeImpl::iterator
@@ -572,7 +572,7 @@ Objects::Foreign::Copier::replace_indirect_object(QPDFObjectHandle const&amp; foreig
         auto result = Dictionary::empty();
         for (auto const& [key, value]: Dictionary(foreign)) {
             if (!value.null()) {
-                result.replaceKey(key, replace_indirect_object(value));
+                result.replace(key, replace_indirect_object(value));
             }
         }
         return result;
@@ -584,7 +584,7 @@ Objects::Foreign::Copier::replace_indirect_object(QPDFObjectHandle const&amp; foreig
         auto dict = result.getDict();
         for (auto const& [key, value]: stream.getDict()) {
             if (!value.null()) {
-                dict.replaceKey(key, replace_indirect_object(value));
+                dict.replace(key, replace_indirect_object(value));
             }
         }
         stream.copy_data_to(result);
@@ -658,7 +658,7 @@ QPDF::getRoot()
     // approach to more extensive checks and warning levels.
     if (m->cf.check_mode() && Name(Root["/Type"]) != "/Catalog") {
         warn(m->c.damagedPDF("", -1, "catalog /Type entry missing or invalid"));
-        Root.replaceKey("/Type", Name("/Catalog"));
+        Root.replace("/Type", Name("/Catalog"));
     }
     return Root.oh();
 }
@@ -799,7 +799,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
             }
             QPDFObjectHandle(dr).makeResourcesIndirect(qpdf);
             if (!dr.indirect()) {
-                acroform.replaceKey("/DR", qpdf.makeIndirectObject(dr));
+                acroform.replace("/DR", qpdf.makeIndirectObject(dr));
                 dr = acroform["/DR"];
             }
             // Merge the other document's /DR, creating a conflict map. mergeResources checks to
@@ -839,7 +839,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
                 if (parent.indirect()) {
                     auto parent_og = parent.id_gen();
                     if (orig_to_copy.contains(parent_og)) {
-                        obj.replaceKey("/Parent", orig_to_copy[parent_og]);
+                        obj.replace("/Parent", orig_to_copy[parent_og]);
                     } else {
                         parent.warn(
                             "while traversing field " + obj.id_gen().unparse(',') +
@@ -869,7 +869,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
                     // chrome, firefox, the mac Preview application, and several of the free
                     // readers on Linux all ignore /DR at the field level.
                     if (obj.contains("/DR")) {
-                        obj.replaceKey("/DR", dr);
+                        obj.replace("/DR", dr);
                     }
                     if (obj["/DA"].isString() && !dr_map.empty()) {
                         adjustDefaultAppearances(obj, dr_map);
@@ -988,7 +988,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         Dictionary apdict = ah.getAppearanceDictionary();
         std::vector<QPDFObjectHandle> streams;
         auto replace_stream = [](auto& dict, auto& key, auto& old) {
-            dict.replaceKey(key, old.copyStream());
+            dict.replace(key, old.copyStream());
             return dict[key];
         };
@@ -1015,7 +1015,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
             }
             apcm.concat(cm);
             if (omatrix || apcm != QPDFMatrix()) {
-                dict.replaceKey("/Matrix", QPDFObjectHandle::newFromMatrix(apcm));
+                dict.replace("/Matrix", QPDFObjectHandle::newFromMatrix(apcm));
             }
             Dictionary resources = dict["/Resources"];
             if (!dr_map.empty() && resources) {
@@ -33,14 +33,14 @@ QPDFAnnotationObjectHelper::getAppearanceDictionary()
 std::string
 QPDFAnnotationObjectHelper::getAppearanceState()
 {
-    Name AS = (*this)["/AS"];
+    Name AS = get("/AS");
     return AS ? AS.value() : "";
 }
 int
 QPDFAnnotationObjectHelper::getFlags()
 {
-    Integer flags_obj = (*this)["/F"];
+    Integer flags_obj = get("/F");
     return flags_obj ? flags_obj : 0;
 }
@@ -31,7 +31,7 @@ void
 QPDFEFStreamObjectHelper::setParam(std::string const& pkey, QPDFObjectHandle const& pval)
 {
     if (Dictionary Params = oh().getDict()["/Params"]) {
-        Params.replaceKey(pkey, pval);
+        Params.replace(pkey, pval);
         return;
     }
     oh().getDict().replaceKey("/Params", Dictionary({{pkey, pval}}));
@@ -42,7 +42,7 @@ std::string
 QPDFFileSpecObjectHelper::getFilename()
 {
     for (auto const& i: name_keys) {
-        if (String k = oh()[i]) {
+        if (String k = get(i)) {
             return k.utf8_value();
         }
     }
@@ -54,7 +54,7 @@ QPDFFileSpecObjectHelper::getFilenames()
 {
     std::map<std::string, std::string> result;
     for (auto const& i: name_keys) {
-        if (String k = oh()[i]) {
+        if (String k = get(i)) {
             result[i] = k.utf8_value();
         }
     }
@@ -64,7 +64,7 @@ QPDFFileSpecObjectHelper::getFilenames()
 QPDFObjectHandle
 QPDFFileSpecObjectHelper::getEmbeddedFileStream(std::string const& key)
 {
-    if (Dictionary EF = oh()["/EF"]) {
+    if (Dictionary EF = get("/EF")) {
         if (!key.empty() && EF.contains(key)) {
             if (auto result = EF[key]) {
                 return result;
@@ -956,7 +956,7 @@ FormField::generateTextAppearance(QPDFAnnotationObjectHelper&amp; aoh)
             aoh.getObjectHandle().replaceKey("/AP", Dictionary::empty());
             AP = aoh.getAppearanceDictionary();
         }
-        AP.replaceKey("/N", AS);
+        AP.replace("/N", AS);
     }
     if (!AS.isStream()) {
         aoh.warn("unable to get normal appearance stream for update");
@@ -53,9 +53,9 @@ QPDFOutlineObjectHelper::getKids()
 QPDFObjectHandle
 QPDFOutlineObjectHelper::getDest()
 {
-    auto dest = (*this)["/Dest"];
+    auto dest = get("/Dest");
     if (dest.null()) {
-        auto const& A = (*this)["/A"];
+        auto const& A = get("/A");
         if (Name(A["/S"]) == "/GoTo") {
             dest = A["/D"];
         }
@@ -2,6 +2,7 @@
 #include <qpdf/QPDFObject_private.hh>
 #include <qpdf/QTC.hh>
+#include <qpdf/Util.hh>
 using namespace std::literals;
 using namespace qpdf;
@@ -29,12 +30,70 @@ BaseHandle::operator[](std::string const&amp; key) const
     return null_obj;
 }
+/// Retrieves a reference to the QPDFObjectHandle associated with the given key in the
+/// dictionary object contained within this instance.
+///
+/// If the current object is not of dictionary type, a `std::runtime_error` is thrown.
+/// According to the PDF specification, missing keys in the dictionary are treated as
+/// keys with a `null` value. This behavior is reflected in this function's implementation,
+/// where a missing key will still return a reference to a newly inserted null value entry.
+///
+/// @param key The key for which the corresponding value in the dictionary is retrieved.
+/// @return A reference to the QPDFObjectHandle associated with the specified key.
+/// @throws std::runtime_error if the current object is not a dictionary.
+QPDFObjectHandle&
+BaseHandle::at(std::string const& key) const
+{
+    auto d = as<QPDF_Dictionary>();
+    if (!d) {
+        throw std::runtime_error("Expected a dictionary but found a non-dictionary object");
+    }
+    return d->items[key];
+}
+
+/// @brief Checks if the specified key exists in the object.
+///
+/// This method determines whether the given key is present in the object by verifying if the
+/// associated value is non-null.
+///
+/// @param key The key to look for in the object.
+/// @return True if the key exists and its associated value is non-null. Otherwise, returns false.
 bool
 BaseHandle::contains(std::string const& key) const
 {
     return !(*this)[key].null();
 }
+/// @brief Retrieves the value associated with the given key from  dictionary.
+///
+/// This method attempts to find the value corresponding to the specified key for objects that can
+/// be interpreted as dictionaries.
+///
+/// - If the object is a dictionary and the specified key exists, it returns a reference
+///   to the associated value.
+/// - If the object is not a dictionary or the specified key does not exist, it returns
+///   a reference to a static uninitialized object handle.
+///
+/// @note Modifying the uninitialized object returned when the key is not found is strictly
+/// prohibited.
+///
+/// @param key The key whose associated value should be retrieved.
+/// @return A reference to the associated value if the key is found or a reference to a static
+/// uninitialized object if the key is not found.
+QPDFObjectHandle&
+BaseHandle::find(std::string const& key) const
+{
+    static const QPDFObjectHandle null_obj;
+    qpdf_invariant(!null_obj);
+    if (auto d = as<QPDF_Dictionary>()) {
+        auto it = d->items.find(key);
+        if (it != d->items.end()) {
+            return it->second;
+        }
+    }
+    return const_cast<QPDFObjectHandle&>(null_obj);
+}
+
 std::set<std::string>
 BaseDictionary::getKeys()
 {
@@ -63,18 +122,29 @@ BaseHandle::erase(const std::string&amp; key)
     return 0;
 }
+bool
+BaseHandle::replace(std::string const& key, QPDFObjectHandle value)
+{
+    if (auto d = as<QPDF_Dictionary>()) {
+        if (value.null() && !value.indirect()) {
+            // The PDF spec doesn't distinguish between keys with null values and missing keys.
+            // Allow indirect nulls which are equivalent to a dangling reference, which is permitted
+            // by the spec.
+            d->items.erase(key);
+        } else {
+            // add or replace value
+            d->items[key] = value;
+        }
+        return true;
+    }
+    return false;
+}
+
 void
-BaseDictionary::replaceKey(std::string const& key, QPDFObjectHandle value)
-{
-    auto d = dict();
-    if (value.null() && !value.indirect()) {
-        // The PDF spec doesn't distinguish between keys with null values and missing keys.
-        // Allow indirect nulls which are equivalent to a dangling reference, which is
-        // permitted by the spec.
-        d->items.erase(key);
-    } else {
-        // add or replace value
-        d->items[key] = value;
+BaseDictionary::replace(std::string const& key, QPDFObjectHandle value)
+{
+    if (!BaseHandle::replace(key, value)) {
+        (void)dict();
     }
 }
@@ -121,7 +191,7 @@ QPDFObjectHandle::hasKey(std::string const&amp; key) const
 QPDFObjectHandle
 QPDFObjectHandle::getKey(std::string const& key) const
 {
-    if (auto result = (*this)[key]) {
+    if (auto result = get(key)) {
         return result;
     }
     if (isDictionary()) {
@@ -166,11 +236,10 @@ QPDFObjectHandle::replaceKey(std::string const&amp; key, QPDFObjectHandle const&amp; val
 {
     if (auto dict = as_dictionary(strict)) {
         checkOwnership(value);
-        dict.replaceKey(key, value);
+        dict.replace(key, value);
         return;
     }
     typeWarning("dictionary", "ignoring key replacement request");
-    QTC::TC("qpdf", "QPDFObjectHandle dictionary ignoring replaceKey");
 }
 QPDFObjectHandle
@@ -200,7 +269,7 @@ QPDFObjectHandle::removeKey(std::string const&amp; key)
 QPDFObjectHandle
 QPDFObjectHandle::removeKeyAndGetOld(std::string const& key)
 {
-    auto result = (*this)[key];
+    auto result = get(key);
     erase(key);
     return result ? result : newNull();
 }
@@ -127,7 +127,7 @@ namespace qpdf
         // The following methods are not part of the public API.
         std::set<std::string> getKeys();
         std::map<std::string, QPDFObjectHandle> const& getAsMap() const;
-        void replaceKey(std::string const& key, QPDFObjectHandle value);
+        void replace(std::string const& key, QPDFObjectHandle value);
         using iterator = std::map<std::string, QPDFObjectHandle>::iterator;
         using const_iterator = std::map<std::string, QPDFObjectHandle>::const_iterator;
@@ -806,6 +806,20 @@ namespace qpdf
         }
     }
+    /// @brief Retrieves the QPDFObjectHandle const& associated with the given key.
+    ///
+    /// This method provides a convenience alternative to the direct use of the subscript operator
+    /// "(*this)[key]" or "oh()[key]" in derived classes, enabling a simplified and readable way to
+    /// access object handles by key.
+    ///
+    /// @param key The string key used to look up the corresponding QPDFObjectHandle.
+    /// @return A constant reference to the QPDFObjectHandle associated with the specified key.
+    inline QPDFObjectHandle const&
+    BaseHandle::get(std::string const& key) const
+    {
+        return (*this)[key];
+    }
+
     inline bool
     BaseHandle::null() const
     {
@@ -54,35 +54,30 @@ test_0(QPDF&amp; pdf, char const* arg2)
     assert_compare_numbers(INT_MAX, t.getKey("/Q1").getIntValueAsInt());
     try {
         assert_compare_numbers(0u, QPDFObjectHandle::newNull().getUIntValueAsUInt());
-        std::cerr << "convert null to uint did not throw\n";
     } catch (QPDFExc const&) {
         std::cerr << "caught expected type error\n";
     }
     assert_compare_numbers(std::numeric_limits<int8_t>::max(), Integer(q1).value<int8_t>());
     assert_compare_numbers(std::numeric_limits<int8_t>::min(), Integer(-q1).value<int8_t>());
     try {
-        int8_t q1_8 = Integer(q1);
-        std::cerr << "q1_8: " << std::to_string(q1_8) << '\n';
+        [[maybe_unused]] int8_t q1_8 = Integer(q1);
     } catch (std::overflow_error const&) {
         std::cerr << "caught expected int8_t overflow error\n";
     }
     try {
-        int8_t q1_8 = Integer(-q1);
-        std::cerr << "q1_8: " << std::to_string(q1_8) << '\n';
+        [[maybe_unused]] int8_t q1_8 = Integer(-q1);
     } catch (std::underflow_error const&) {
         std::cerr << "caught expected int8_t underflow error\n";
     }
     assert_compare_numbers(std::numeric_limits<uint8_t>::max(), Integer(q1).value<uint8_t>());
     assert_compare_numbers(0, Integer(-q1).value<uint8_t>());
     try {
-        uint8_t q1_u8 = Integer(q1);
-        std::cerr << "q1_u8: " << std::to_string(q1_u8) << '\n';
+        [[maybe_unused]] uint8_t q1_u8 = Integer(q1);
     } catch (std::overflow_error const&) {
         std::cerr << "caught expected uint8_t overflow error\n";
     }
     try {
-        uint8_t q1_u8 = Integer(-q1);
-        std::cerr << "q1_u8: " << std::to_string(q1_u8) << '\n';
+        [[maybe_unused]] uint8_t q1_u8 = Integer(-q1);
     } catch (std::underflow_error const&) {
         std::cerr << "caught expected uint8_t underflow error\n";
     }
@@ -95,6 +90,70 @@ test_0(QPDF&amp; pdf, char const* arg2)
     assert_compare_numbers(UINT_MAX, t.getKey("/Q3").getUIntValueAsUInt());
 }
+
+static void
+test_1(QPDF& pdf, char const* arg2)
+{
+    // Test new dictionary methods.
+    using namespace qpdf;
+    auto d = Dictionary({{"/A", {}}, {"/B", Null()}, {"/C", Dictionary::empty()}});
+
+    // contains
+    assert(!d.contains("/A"));
+    assert(!d.contains("/B"));
+    assert(d.contains("/C"));
+
+    auto i = Integer(42);
+    assert(!i.contains("/A"));
+
+    // at
+    assert(!d.at("/A"));
+    assert(d.at("/B"));
+    assert(d.at("/B").null());
+    assert(d.at("/C"));
+    assert(!d.at("/C").null());
+    d.at("/C") = Integer(42);
+    assert(d.at("/C") == 42);
+    assert(!d.at("/D"));
+    assert(d.at("/D").null());
+    assert(QPDFObjectHandle(d).getDictAsMap().contains("/D"));
+    assert(QPDFObjectHandle(d).getDictAsMap().size() == 4);
+
+    bool thrown = false;
+    try {
+       i.at("/A");
+    } catch (std::runtime_error const&) {
+        thrown = true;
+    }
+    assert(thrown);
+
+    // find
+    assert(!d.find("/A"));
+    assert(d.find("/B"));
+    assert(d.find("/B").null());
+    assert(d.find("/C"));
+    assert(Integer(d.find("/C")) == 42);
+    d.find("/C") = Name("/DontPanic");
+    assert(Name(d.find("/C")) == "/DontPanic");
+    assert(!d.find("/E"));
+    assert(!QPDFObjectHandle(d).getDictAsMap().contains("/E"));
+    assert(QPDFObjectHandle(d).getDictAsMap().size() == 4);
+
+    // replace
+    assert(!i.replace("/A", Name("/DontPanic")));
+    Dictionary di = i.oh();
+    thrown = false;
+    try {
+        di.replace("/A", Name("/DontPanic"));
+    } catch (std::runtime_error const&) {
+        thrown = true;
+    }
+    assert(thrown);
+    d.replace("/C", Integer(42));
+    assert(Integer(d["/C"]) == 42);
+    assert(QPDFObjectHandle(d).getDictAsMap().size() == 4);
+}
+
 void
 runtest(int n, char const* filename1, char const* arg2)
 {
@@ -102,7 +161,7 @@ runtest(int n, char const* filename1, char const* arg2)
     // the test suite to see how the test is invoked to find the file
     // that the test is supposed to operate on.
-    std::set<int> ignore_filename = {};
+    std::set<int> ignore_filename = {1,};
     QPDF pdf;
     std::shared_ptr<char> file_buf;
@@ -116,7 +175,7 @@ runtest(int n, char const* filename1, char const* arg2)
     }
     std::map<int, void (*)(QPDF&, char const*)> test_functions = {
-        {0, test_0},
+        {0, test_0}, {1, test_1},
     };
     auto fn = test_functions.find(n);
@@ -11,12 +11,16 @@ require TestDriver;
 my $td = new TestDriver('objects');
-my $n_tests = 1;
-
+my $n_tests = 2;
 $td->runtest("integer type checks",
              {$td->COMMAND => "objects 0 minimal.pdf"},
              {$td->FILE => "test0.out", $td->EXIT_STATUS => 0},
              $td->NORMALIZE_NEWLINES);
+$td->runtest("dictionary checks",
+             {$td->COMMAND => "objects 1 -"},
+             {$td->STRING =>  => "test 1 done\n", $td->EXIT_STATUS => 0},
+             $td->NORMALIZE_NEWLINES);
+
 $td->report($n_tests);
@@ -171,7 +171,6 @@ QPDFObjectHandle array ignoring erase item 0
 QPDFObjectHandle dictionary false for hasKey 0
 QPDFObjectHandle dictionary empty set for getKeys 0
 QPDFObjectHandle dictionary empty map for asMap 0
-QPDFObjectHandle dictionary ignoring replaceKey 0
 QPDFObjectHandle numeric non-numeric 0
 QPDFObjectHandle erase array bounds 0
 qpdf-c called qpdf_check_pdf 0