Merge pull request #1535 from m-holger/afdh

Refactor `QPDFAcroFormDocumentHelper`: simplify `traverseField`, remo…

Merge pull request #1535 from m-holger/afdh
Refactor `QPDFAcroFormDocumentHelper`: simplify `traverseField`, remo…
m-holger · GitHub
2 parents bdceb083 9a3f2969
Showing 3 changed files with 25 additions and 50 deletions
include/qpdf/QPDFAcroFormDocumentHelper.hh
libqpdf/QPDFAcroFormDocumentHelper.cc
qpdf/qpdf.testcov
@@ -226,8 +226,7 @@ class QPDFAcroFormDocumentHelper: public QPDFDocumentHelper
   private:
     void analyze();
-    void traverseField(
-        QPDFObjectHandle field, QPDFObjectHandle parent, int depth, QPDFObjGen::set& visited);
+    void traverseField(QPDFObjectHandle const& field, QPDFObjectHandle const& parent, int depth);
     QPDFObjectHandle getOrCreateAcroForm();
     void adjustInheritedFields(
         QPDFObjectHandle obj,
@@ -89,8 +89,7 @@ QPDFAcroFormDocumentHelper::addFormField(QPDFFormFieldObjectHelper ff)
         fields = acroform.replaceKeyAndGetNew("/Fields", QPDFObjectHandle::newArray());
     }
     fields.appendItem(ff.getObjectHandle());
-    QPDFObjGen::set visited;
-    traverseField(ff.getObjectHandle(), QPDFObjectHandle::newNull(), 0, visited);
+    traverseField(ff.getObjectHandle(), {}, 0);
 }
 void
@@ -188,9 +187,7 @@ void
 QPDFAcroFormDocumentHelper::setFormFieldName(QPDFFormFieldObjectHelper ff, std::string const& name)
 {
     ff.setFieldAttribute("/T", name);
-    QPDFObjGen::set visited;
-    auto ff_oh = ff.getObjectHandle();
-    traverseField(ff_oh, ff_oh.getKey("/Parent"), 0, visited);
+    traverseField(ff, ff["/Parent"], 0);
 }
 std::vector<QPDFFormFieldObjectHelper>
@@ -199,7 +196,7 @@ QPDFAcroFormDocumentHelper::getFormFields()
     analyze();
     std::vector<QPDFFormFieldObjectHelper> result;
     for (auto const& [og, data]: m->field_to) {
-        if (!(data.annotations.empty())) {
+        if (!data.annotations.empty()) {
             result.emplace_back(qpdf.getObject(og));
         }
     }
@@ -279,18 +276,14 @@ QPDFAcroFormDocumentHelper::analyze()
         return;
     }
     QPDFObjectHandle fields = acroform.getKey("/Fields");
-    if (auto fa = fields.as_array(strict)) {
+    if (Array fa = fields) {
         // Traverse /AcroForm to find annotations and map them bidirectionally to fields.
-
-        QPDFObjGen::set visited;
-        QPDFObjectHandle null(QPDFObjectHandle::newNull());
         for (auto const& field: fa) {
-            traverseField(field, null, 0, visited);
+            traverseField(field, {}, 0);
         }
     } else {
-        QTC::TC("qpdf", "QPDFAcroFormDocumentHelper fields not array");
         acroform.warn("/Fields key of /AcroForm dictionary is not an array; ignoring");
-        fields = QPDFObjectHandle::newArray();
+        fields = Array::empty();
     }
     // All Widget annotations should have been encountered by traversing /AcroForm, but in case any
@@ -305,7 +298,6 @@ QPDFAcroFormDocumentHelper::analyze()
             QPDFObjectHandle annot(iter.getObjectHandle());
             QPDFObjGen og(annot.getObjGen());
             if (!m->annotation_to_field.contains(og)) {
-                QTC::TC("qpdf", "QPDFAcroFormDocumentHelper orphaned widget");
                 // This is not supposed to happen, but it's easy enough for us to handle this case.
                 // Treat the annotation as its own field. This could allow qpdf to sensibly handle a
                 // case such as a PDF creator adding a self-contained annotation (merged with the
@@ -323,7 +315,7 @@ QPDFAcroFormDocumentHelper::analyze()
 void
 QPDFAcroFormDocumentHelper::traverseField(
-    QPDFObjectHandle field, QPDFObjectHandle parent, int depth, QPDFObjGen::set& visited)
+    QPDFObjectHandle const& field, QPDFObjectHandle const& parent, int depth)
 {
     if (depth > 100) {
         // Arbitrarily cut off recursion at a fixed depth to avoid specially crafted files that
@@ -331,22 +323,19 @@ QPDFAcroFormDocumentHelper::traverseField(
         return;
     }
     if (!field.indirect()) {
-        QTC::TC("qpdf", "QPDFAcroFormDocumentHelper direct field");
         field.warn(
-            "encountered a direct object as a field or annotation while "
-            "traversing /AcroForm; ignoring field or annotation");
+            "encountered a direct object as a field or annotation while traversing /AcroForm; "
+            "ignoring field or annotation");
         return;
     }
     if (!field.isDictionary()) {
-        QTC::TC("qpdf", "QPDFAcroFormDocumentHelper non-dictionary field");
         field.warn(
-            "encountered a non-dictionary as a field or annotation while"
-            " traversing /AcroForm; ignoring field or annotation");
+            "encountered a non-dictionary as a field or annotation while traversing /AcroForm; "
+            "ignoring field or annotation");
         return;
     }
     QPDFObjGen og(field.getObjGen());
-    if (!visited.add(og)) {
-        QTC::TC("qpdf", "QPDFAcroFormDocumentHelper loop");
+    if (m->field_to.contains(og) || m->annotation_to_field.contains(og)) {
         field.warn("loop detected while traversing /AcroForm");
         return;
     }
@@ -357,21 +346,10 @@ QPDFAcroFormDocumentHelper::traverseField(
     // fields can be merged with terminal field dictionaries. Otherwise, the annotation fields might
     // be there to be inherited by annotations below it.
-    bool is_annotation = false;
-    bool is_field = (0 == depth);
-    if (auto a = field.getKey("/Kids").as_array(strict)) {
-        is_field = true;
-        for (auto const& item: a) {
-            traverseField(item, field, 1 + depth, visited);
-        }
-    } else {
-        if (field.hasKey("/Parent")) {
-            is_field = true;
-        }
-        if (field.hasKey("/Subtype") || field.hasKey("/Rect") || field.hasKey("/AP")) {
-            is_annotation = true;
-        }
-    }
+    Array Kids = field["/Kids"];
+    const bool is_field = depth == 0 || Kids || field.hasKey("/Parent");
+    const bool is_annotation =
+        !Kids && (field.hasKey("/Subtype") || field.hasKey("/Rect") || field.hasKey("/AP"));
     QTC::TC("qpdf", "QPDFAcroFormDocumentHelper field found", (depth == 0) ? 0 : 1);
     QTC::TC("qpdf", "QPDFAcroFormDocumentHelper annotation found", (is_field ? 0 : 1));
@@ -384,15 +362,18 @@ QPDFAcroFormDocumentHelper::traverseField(
     if (is_field && (field.hasKey("/T"))) {
         QPDFFormFieldObjectHelper foh(field);
-        auto f_og = field.getObjGen();
         std::string name = foh.getFullyQualifiedName();
-        auto old = m->field_to.find(f_og);
+        auto old = m->field_to.find(og);
         if (old != m->field_to.end() && !old->second.name.empty()) {
             // We might be updating after a name change, so remove any old information
-            m->name_to_fields[old->second.name].erase(f_og);
+            m->name_to_fields[old->second.name].erase(og);
         }
-        m->field_to[f_og].name = name;
-        m->name_to_fields[name].insert(f_og);
+        m->field_to[og].name = name;
+        m->name_to_fields[name].insert(og);
+    }
+
+    for (auto const& kid: Kids) {
+        traverseField(kid, field, 1 + depth);
     }
 }
@@ -204,11 +204,6 @@ QPDFAnnotationObjectHelper AP stream 0
 QPDFAnnotationObjectHelper AP dictionary 0
 QPDFAnnotationObjectHelper AP sub stream 0
 QPDFAnnotationObjectHelper AP null 0
-QPDFAcroFormDocumentHelper fields not array 0
-QPDFAcroFormDocumentHelper orphaned widget 0
-QPDFAcroFormDocumentHelper direct field 0
-QPDFAcroFormDocumentHelper non-dictionary field 0
-QPDFAcroFormDocumentHelper loop 0
 QPDFAcroFormDocumentHelper field found 1
 QPDFAcroFormDocumentHelper annotation found 1
 QPDFJob automatically set keep files open 1