Internal: Change where option values are constructed

- Don't store option_ins, option_delid. - Don't remove overridden options for prepare.
kohler · Nov 30, 2024 · ced8eaa · ced8eaa
1 parent b6bf487
commit ced8eaa
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 65 deletions.
diff --git a/src/options/o_abstract.php b/src/options/o_abstract.php
@@ -20,6 +20,9 @@ function value_export_json(PaperValue $ov, PaperExport $pex) {
         return (string) $ov->data();
     }
     function value_save(PaperValue $ov, PaperStatus $ps) {
+        if ($ov->equals($ov->prow->base_option($this->id))) {
+            return true;
+        }
         $ps->change_at($this);
         $ab = $ov->data();
         if ($ab === null || strlen($ab) < 16383) {

diff --git a/src/options/o_authors.php b/src/options/o_authors.php
@@ -122,21 +122,23 @@ function value_check(PaperValue $ov, Contact $user) {
     }
 
     function value_save(PaperValue $ov, PaperStatus $ps) {
-        // set property
+        // construct property
         $authlist = $this->author_list($ov);
-        $v = "";
+        $d = "";
         $emails = [];
         foreach ($authlist as $auth) {
             if (!$auth->is_empty()) {
-                $v .= ($v === "" ? "" : "\n") . $auth->unparse_tabbed();
+                $d .= ($d === "" ? "" : "\n") . $auth->unparse_tabbed();
             }
             $emails[] = $auth->email;
         }
-        if ($v === $ov->prow->authorInformation) {
+
+        // check for change
+        if ($d === $ov->prow->base_option($this->id)->data()) {
             return true;
         }
         $ps->change_at($this);
-        $ov->prow->set_prop("authorInformation", $v);
+        $ov->prow->set_prop("authorInformation", $d);
 
         // set conflicts
         $ps->clear_conflict_values(CONFLICT_AUTHOR);

diff --git a/src/options/o_collaborators.php b/src/options/o_collaborators.php
@@ -28,6 +28,9 @@ function value_check(PaperValue $ov, Contact $user) {
         }
     }
     function value_save(PaperValue $ov, PaperStatus $ps) {
+        if ($ov->equals($ov->prow->base_option($this->id))) {
+            return true;
+        }
         $ps->change_at($this);
         $collab = $ov->data();
         if ($collab === null || strlen($collab) < 8190) {

diff --git a/src/options/o_nonblind.php b/src/options/o_nonblind.php
@@ -18,6 +18,9 @@ function value_export_json(PaperValue $ov, PaperExport $ps) {
         return !!$ov->value;
     }
     function value_save(PaperValue $ov, PaperStatus $ps) {
+        if ($ov->equals($ov->prow->base_option($this->id))) {
+            return true;
+        }
         $ps->change_at($this);
         $ov->prow->set_prop("blind", $ov->value ? 0 : 1);
         return true;

diff --git a/src/options/o_title.php b/src/options/o_title.php
@@ -20,6 +20,9 @@ function value_export_json(PaperValue $ov, PaperExport $pex) {
         return (string) $ov->data();
     }
     function value_save(PaperValue $ov, PaperStatus $ps) {
+        if ($ov->equals($ov->prow->base_option($this->id))) {
+            return true;
+        }
         $ps->change_at($this);
         $ov->prow->set_prop("title", $ov->data());
         return true;

diff --git a/src/options/o_topics.php b/src/options/o_topics.php
@@ -61,12 +61,14 @@ function value_force(PaperValue $ov) {
     }
 
     function value_save(PaperValue $ov, PaperStatus $ps) {
-        if ($this->id === PaperOption::TOPICSID) {
-            $ps->change_at($this);
-            $ov->prow->set_prop("topicIds", join(",", $ov->value_list()));
+        if ($ov->equals($ov->prow->base_option($this->id))) {
             return true;
-        } else {
+        }
+        if ($this->id !== PaperOption::TOPICSID) {
             return false;
         }
+        $ps->change_at($this);
+        $ov->prow->set_prop("topicIds", join(",", $ov->value_list()));
+        return true;
     }
 }
diff --git a/src/paperinfo.php b/src/paperinfo.php
@@ -2466,6 +2466,11 @@ function override_option(PaperValue $ov) {
         $this->_option_array[$id] = $ov;
     }
 
+    /** @return list<int> */
+    function overridden_option_ids() {
+        return array_keys($this->_base_option_array ?? []);
+    }
+
     function remove_option_overrides() {
         if ($this->_base_option_array !== null) {
             foreach ($this->_base_option_array as $id => $ov) {

diff --git a/src/paperoption.php b/src/paperoption.php
@@ -1451,13 +1451,12 @@ function value_store(PaperValue $ov, PaperStatus $ps) {
         }
     }
     function value_save(PaperValue $ov, PaperStatus $ps) {
-        if ($this->id === DTYPE_SUBMISSION || $this->id === DTYPE_FINAL) {
-            $ps->change_at($this);
-            $ov->prow->set_prop($this->id ? "finalPaperStorageId" : "paperStorageId", $ov->value ?? 0);
-            return true;
-        } else {
+        if ($this->id !== DTYPE_SUBMISSION && $this->id !== DTYPE_FINAL) {
             return false;
         }
+        $ps->change_at($this);
+        $ov->prow->set_prop($this->id ? "finalPaperStorageId" : "paperStorageId", $ov->value ?? 0);
+        return true;
     }
 
     function parse_qreq(PaperInfo $prow, Qrequest $qreq) {

diff --git a/src/paperstatus.php b/src/paperstatus.php
@@ -34,12 +34,6 @@ class PaperStatus extends MessageSet {
     private $_fdiffs;
     /** @var list<string> */
     private $_xdiffs;
-    /** @var associative-array<int,PaperValue> */
-    private $_field_values;
-    /** @var ?list<int> */
-    private $_option_delid;
-    /** @var ?list<list> */
-    private $_option_ins;
     /** @var associative-array<int,array{int,int,int}> */
     private $_conflict_values;
     /** @var ?list<array{int,int,int}> */
@@ -710,7 +704,6 @@ private function _check_field($pj, $opt) {
             if (!$ov->has_error()) {
                 $opt->value_store($ov, $this);
                 $this->prow->override_option($ov);
-                $this->_field_values[$opt->id] = $ov;
             }
         } else {
             $ov = $this->prow->force_option($opt);
@@ -719,34 +712,6 @@ private function _check_field($pj, $opt) {
         $ov->append_messages_to($this);
     }
 
-    /** @param PaperValue $ov */
-    private function _prepare_save_field($ov) {
-        // erroneous values shouldn't be saved
-        if ($ov->has_error()) { // XXX should never have errors here
-            return;
-        }
-        // return if no change
-        if ($ov->equals($this->prow->base_option($ov->option))) {
-            return;
-        }
-        // option may know how to save itself
-        if ($ov->option->value_save($ov, $this)) {
-            return;
-        }
-        // otherwise, save option normal way
-        $this->change_at($ov->option);
-        $oid = $ov->option_id();
-        $this->_option_delid[] = $oid;
-        $d1 = $ov->data_list();
-        foreach ($ov->value_list() as $i => $v) {
-            $qv0 = [-1, $oid, $v, null, null];
-            if ($d1[$i] !== null) {
-                $qv0[strlen($d1[$i]) < 32768 ? 3 : 4] = $d1[$i];
-            }
-            $this->_option_ins[] = $qv0;
-        }
-    }
-
     private function _prepare_status($pj) {
         // check whether it’s ok to change withdrawn status
         $old_withdrawn = $this->prow->timeWithdrawn > 0;
@@ -1048,7 +1013,6 @@ private function _reset(PaperInfo $prow, $pid) {
         $this->paperId = $this->title = null;
         $this->_fdiffs = $this->_xdiffs = [];
         $this->_unknown_fields = null;
-        $this->_field_values = $this->_option_delid = $this->_option_ins = [];
         $this->_conflict_values = [];
         $this->_conflict_ins = $this->_created_contacts = null;
         $this->_author_change_cids = null;
@@ -1153,13 +1117,29 @@ function prepare_save_paper_json($pj) {
      * @return bool */
     private function _finish_prepare($pj) {
         $ok = $this->_normalize_and_check($pj);
-        $this->prow->remove_option_overrides();
         if ($ok) {
             return true;
         } else {
             $this->prow->abort_prop();
+            $this->prow->remove_option_overrides();
+            return false;
+        }
+    }
+
+    /** @return bool */
+    private function _new_paper_is_empty() {
+        foreach ($this->_fdiffs as $opt) {
+            if ($opt->id === PaperOption::ANONYMITYID) {
+                continue;
+            }
+            if ($opt->id === PaperOption::AUTHORSID
+                && (!$this->prow->authorInformation
+                    || $this->prow->authorInformation === (new Author($this->user))->unparse_tabbed())) {
+                continue;
+            }
             return false;
         }
+        return true;
     }
 
     /** @param object $pj
@@ -1192,9 +1172,14 @@ private function _normalize_and_check($pj) {
             return false;
         }
 
-        // prepare fields for saving, mark which fields have changed
-        foreach ($this->_field_values ?? [] as $ov) {
-            $this->_prepare_save_field($ov);
+        // prepare fields for saving, mark changed fields
+        foreach ($this->prow->overridden_option_ids() as $oid) {
+            $ov = $this->prow->option($oid);
+            if (!$ov->has_error()
+                && !$ov->option->value_save($ov, $this)
+                && !$ov->equals($this->prow->base_option($oid))) {
+                $this->change_at($ov->option);
+            }
         }
 
         // prepare non-fields for saving
@@ -1227,10 +1212,7 @@ private function _normalize_and_check($pj) {
 
         // don't save if creating a mostly-empty paper
         if ($this->prow->is_new()
-            && !array_diff(array_keys($this->prow->_old_prop), ["authorInformation", "blind"])
-            && (!$this->prow->authorInformation
-                || $this->prow->authorInformation === (new Author($this->user))->unparse_tabbed())
-            && empty($this->_option_ins)) {
+            && $this->_new_paper_is_empty()) {
             $this->error_at(null, $this->_("<0>Empty {submission}. Please fill out the fields and try again"));
             $this->conf->qe("delete from Paper where paperId=?", $this->prow->paperId);
             $this->conf->qe("update PaperStorage set paperId=-1 where paperId=?", $this->prow->paperId);
@@ -1342,14 +1324,32 @@ private function _execute_topics() {
     }
 
     private function _execute_options() {
-        if (!empty($this->_option_delid)) {
-            $this->conf->qe("delete from PaperOption where paperId=? and optionId?a", $this->paperId, $this->_option_delid);
+        $x = [];
+        foreach ($this->_fdiffs as $opt) {
+            $x[] = $opt->field_key();
         }
-        if (!empty($this->_option_ins)) {
-            foreach ($this->_option_ins as &$x) {
-                $x[0] = $this->paperId;
+
+        $del = $ins = [];
+        foreach ($this->_fdiffs as $opt) {
+            if ($opt->id <= 0) {
+                continue;
+            }
+            $ov = $this->prow->option($opt);
+            $del[] = $opt->id;
+            $dl = $ov->data_list();
+            foreach ($ov->value_list() as $i => $v) {
+                $qv0 = [$this->paperId, $opt->id, $v, null, null];
+                if ($dl[$i] !== null) {
+                    $qv0[strlen($dl[$i]) < 32768 ? 3 : 4] = $dl[$i];
+                }
+                $ins[] = $qv0;
             }
-            $this->conf->qe("insert into PaperOption (paperId, optionId, value, data, dataOverflow) values ?v", $this->_option_ins);
+        }
+        if (!empty($del) && !$this->prow->is_new()) {
+            $this->conf->qe("delete from PaperOption where paperId=? and optionId?a", $this->paperId, $del);
+        }
+        if (!empty($ins)) {
+            $this->conf->qe("insert into PaperOption (paperId, optionId, value, data, dataOverflow) values ?v", $ins);
         }
     }
 
@@ -1518,14 +1518,14 @@ function execute_save() {
         // do (e.g. in old tests), invalidate it now when it's convenient
         // to do so. XXX It would be useful to keep `$this->prow` around...
         $this->prow->commit_prop();
+        $this->prow->remove_option_overrides();
         $this->prow->invalidate_options();
         $this->prow->invalidate_conflicts();
         $this->prow->invalidate_documents();
 
         // save new title and clear out memory
         $this->title = $this->prow->title();
         $this->prow = null;
-        $this->_field_values = null;
         $this->_update_pid_dids = $this->_joindocs = null;
         return true;
     }

diff --git a/src/topicset.php b/src/topicset.php
@@ -166,6 +166,7 @@ function offsetSet($offset, $value) {
     function offsetUnset($offset) {
         throw new Exception("invalid TopicSet::offsetUnset");
     }
+
     /** @param int $tid
      * @return ?string */
     function name($tid) {
@@ -207,6 +208,7 @@ function sort(&$ids) {
             return $this->_order[$a] - $this->_order[$b];
         });
     }
+
     /** @param array<int,mixed> &$by_id */
     function ksort(&$by_id) {
         uksort($by_id, function ($a, $b) {