From be7e37e9241b7c24e46c9f851131552be2a680cb Mon Sep 17 00:00:00 2001 From: Matt Tracy Date: Wed, 16 Nov 2022 13:24:14 -0800 Subject: [PATCH 1/3] Retain block references on file reload Commit fixes a bug where block references (i.e. `:block/refs` association in the db) are lost when the page containing the referenced block is re-loaded due to an update of its underlying file. Description of Bug: The bug occurs because when a file is re-loaded to the DB from disk, all existing blocks belonging to the file are deleted via `retractEntity`, and then blocks from the parsed file are added. If the file had only had small changes, the new block set will be very similar to the previous one, although with different db/ids. However, while new blocks with "id:: " properties will assume the UUID value of the previous version of the block, any references to that block via UUID will *not* be restored in the DB; they are deleted with the retractEntity command. This results in an inconsistent DB state, where references that should exist do not. Description of Fix: The 'delete-blocks-fn' passed to the graph_parser has been modified as such: - It now accepts a list of block uuids to *retain*; graph parser will pass the blocks parsed from the new file content. - For any blocks which match a UUID in the retain list, instead of deleting via retractEntity, the individual attributes are deleted via retractAttribute (the `retract-attributes` from schema.cljs is used for this purpose). --- .../graph-parser/src/logseq/graph_parser.cljs | 2 +- src/main/frontend/handler/common/file.cljs | 38 +++++++++++++++---- src/test/frontend/db/model_test.cljs | 21 +++++++++- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/deps/graph-parser/src/logseq/graph_parser.cljs b/deps/graph-parser/src/logseq/graph_parser.cljs index fc50bb1c61..67e78e62ef 100644 --- a/deps/graph-parser/src/logseq/graph_parser.cljs +++ b/deps/graph-parser/src/logseq/graph_parser.cljs @@ -37,8 +37,8 @@ (extract/extract-whiteboard-edn file content extract-options') :else nil) - delete-blocks (delete-blocks-fn (first pages) file) block-ids (map (fn [block] {:block/uuid (:block/uuid block)}) blocks) + delete-blocks (delete-blocks-fn (first pages) file block-ids) block-refs-ids (->> (mapcat :block/refs blocks) (filter (fn [ref] (and (vector? ref) (= :block/uuid (first ref))))) diff --git a/src/main/frontend/handler/common/file.cljs b/src/main/frontend/handler/common/file.cljs index c35f9b0d5a..1be2c64eac 100644 --- a/src/main/frontend/handler/common/file.cljs +++ b/src/main/frontend/handler/common/file.cljs @@ -6,6 +6,7 @@ [frontend.db :as db] ["/frontend/utils" :as utils] [frontend.mobile.util :as mobile-util] + [logseq.db.schema :as db-schema] [logseq.graph-parser :as graph-parser] [logseq.graph-parser.util :as gp-util] [logseq.graph-parser.config :as gp-config] @@ -19,12 +20,32 @@ (when (not= file current-file) current-file)))) -(defn- get-delete-blocks [repo-url first-page file] - (let [delete-blocks (-> - (concat - (db/delete-file-blocks! repo-url file) - (when first-page (db/delete-page-blocks repo-url (:block/name first-page)))) - (distinct))] +(defn- get-clear-blocks-tx + [blocks retain-uuids] + (let [tx-for-block (fn [block] (let [{uuid :block/uuid eid :db/id} block + should-retain? (and uuid (contains? retain-uuids uuid))] + (cond + should-retain? + (map (fn [attr] [:db.fn/retractAttribute eid attr]) db-schema/retract-attributes) + :else + [[:db.fn/retractEntity eid]])))] + (mapcat tx-for-block (distinct blocks))) + ) + +(defn- get-clear-block-tx + "Returns the transactional operations to clear blocks belonging to the given + page and file. + + Blocks are by default fully deleted via retractEntity. However, a collection + of block UUIDs to retain can be passed, and any blocks with matching uuids + will instead have their attributes cleared individually via + 'retractAttribute'. This will preserve block references to the retained + UUIDs." + [repo-url first-page file retain-uuid-blocks] + (let [pages-to-clear (filter some? [(db/get-file-page file) file]) + blocks (mapcat (fn [page] (db/get-page-blocks-no-cache repo-url page {:pull-keys [:db/id :block/uuid]})) pages-to-clear) + retain-uuids (if (seq retain-uuid-blocks) (set (filter some? (map :block/uuid retain-uuid-blocks))) []) + tx (get-clear-blocks-tx blocks retain-uuids)] (when-let [current-file (page-exists-in-another-file repo-url first-page file)] (when (not= file current-file) (let [error (str "Page already exists with another file: " current-file ", current file: " file)] @@ -32,7 +53,8 @@ {:content error :status :error :clear? false}])))) - delete-blocks)) + tx + )) (defn reset-file! "Main fn for updating a db with the results of a parsed file" @@ -62,7 +84,7 @@ new? (nil? (db/entity [:file/path file])) options (merge (dissoc options :verbose) {:new? new? - :delete-blocks-fn (partial get-delete-blocks repo-url) + :delete-blocks-fn (partial get-clear-block-tx repo-url) :extract-options (merge {:user-config (state/get-config) :date-formatter (state/get-date-formatter) diff --git a/src/test/frontend/db/model_test.cljs b/src/test/frontend/db/model_test.cljs index 5511ccdc59..cc91811b97 100644 --- a/src/test/frontend/db/model_test.cljs +++ b/src/test/frontend/db/model_test.cljs @@ -1,7 +1,9 @@ (ns frontend.db.model-test - (:require [cljs.test :refer [use-fixtures deftest is]] + (:require [cljs.test :refer [use-fixtures deftest testing is]] [frontend.db.model :as model] - [frontend.test.helper :as test-helper :refer [load-test-files]])) + [frontend.test.helper :as test-helper :refer [load-test-files]] + [logseq.graph-parser.util.block-ref :as block-ref] + )) (use-fixtures :each {:before test-helper/start-test-db! :after test-helper/destroy-test-db!}) @@ -136,6 +138,21 @@ (#'model/get-unnecessary-namespaces-name '("one/two/tree" "one" "one/two" "non nested tag" "non nested link"))) "Must be one/two one")) +(deftest refs-to-page-maintained-on-reload + (testing + "Refs to blocks on a page are retained if that page is reload." + (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9acd" + target-page-content (str "first line\n- target block\n id:: " test-uuid) + referring-page-content (str "first line\n- " (block-ref/->block-ref test-uuid))] + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content} + {:file/path "pages/referrer.md" + :file/content referring-page-content}]) + (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content}]) + (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) + ))) From 2848d101b0f88fdaaecfcb44564bd85927e62d92 Mon Sep 17 00:00:00 2001 From: Matt Tracy Date: Fri, 18 Nov 2022 15:35:29 -0800 Subject: [PATCH 2/3] Address Review Feedback - Fixed new page name not being cleared - Clarified some fn and parameter names - Added test for reload page w/ title:: property change --- src/main/frontend/handler/common/file.cljs | 32 ++++++++------ src/test/frontend/db/model_test.cljs | 49 ++++++++++++++++------ 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/main/frontend/handler/common/file.cljs b/src/main/frontend/handler/common/file.cljs index 1be2c64eac..f31c401415 100644 --- a/src/main/frontend/handler/common/file.cljs +++ b/src/main/frontend/handler/common/file.cljs @@ -20,7 +20,7 @@ (when (not= file current-file) current-file)))) -(defn- get-clear-blocks-tx +(defn- retract-blocks-tx [blocks retain-uuids] (let [tx-for-block (fn [block] (let [{uuid :block/uuid eid :db/id} block should-retain? (and uuid (contains? retain-uuids uuid))] @@ -32,23 +32,31 @@ (mapcat tx-for-block (distinct blocks))) ) -(defn- get-clear-block-tx - "Returns the transactional operations to clear blocks belonging to the given - page and file. +(defn- retract-file-blocks-tx + "Returns the transactional operations to retract blocks belonging to the + given page name and file path. This function is required when a file is being + parsed from disk; before saving the parsed, blocks from the previous version + of that file need to be retracted. + + The 'Page' parsed from the new file version is passed separately from the + file-path, as the page name can be set via properties in the file, and thus + can change between versions. If it has changed, existing blocks for both the + old and new page name will be retracted. - Blocks are by default fully deleted via retractEntity. However, a collection + Blocks are by default fully cleared via retractEntity. However, a collection of block UUIDs to retain can be passed, and any blocks with matching uuids will instead have their attributes cleared individually via 'retractAttribute'. This will preserve block references to the retained UUIDs." - [repo-url first-page file retain-uuid-blocks] - (let [pages-to-clear (filter some? [(db/get-file-page file) file]) + [repo-url file-page file-path retain-uuid-blocks] + (let [existing-file-page (db/get-file-page file-path) + pages-to-clear (distinct (filter some? [existing-file-page (:block/name file-page)])) blocks (mapcat (fn [page] (db/get-page-blocks-no-cache repo-url page {:pull-keys [:db/id :block/uuid]})) pages-to-clear) retain-uuids (if (seq retain-uuid-blocks) (set (filter some? (map :block/uuid retain-uuid-blocks))) []) - tx (get-clear-blocks-tx blocks retain-uuids)] - (when-let [current-file (page-exists-in-another-file repo-url first-page file)] - (when (not= file current-file) - (let [error (str "Page already exists with another file: " current-file ", current file: " file)] + tx (retract-blocks-tx blocks retain-uuids)] + (when-let [current-file (page-exists-in-another-file repo-url file-page file-path)] + (when (not= file-path current-file) + (let [error (str "Page already exists with another file: " current-file ", current file: " file-path)] (state/pub-event! [:notification/show {:content error :status :error @@ -84,7 +92,7 @@ new? (nil? (db/entity [:file/path file])) options (merge (dissoc options :verbose) {:new? new? - :delete-blocks-fn (partial get-clear-block-tx repo-url) + :delete-blocks-fn (partial retract-file-blocks-tx repo-url) :extract-options (merge {:user-config (state/get-config) :date-formatter (state/get-date-formatter) diff --git a/src/test/frontend/db/model_test.cljs b/src/test/frontend/db/model_test.cljs index cc91811b97..deeb36b344 100644 --- a/src/test/frontend/db/model_test.cljs +++ b/src/test/frontend/db/model_test.cljs @@ -141,19 +141,42 @@ (deftest refs-to-page-maintained-on-reload (testing "Refs to blocks on a page are retained if that page is reload." - (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9acd" - target-page-content (str "first line\n- target block\n id:: " test-uuid) - referring-page-content (str "first line\n- " (block-ref/->block-ref test-uuid))] - (load-test-files [{:file/path "pages/target.md" - :file/content target-page-content} - {:file/path "pages/referrer.md" - :file/content referring-page-content}]) - (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) - (load-test-files [{:file/path "pages/target.md" - :file/content target-page-content}]) - (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) - ))) - + (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9acd" + target-page-content (str "- target block\n id:: " (block-ref/->block-ref test-uuid)) + referring-page-content (str "- " (block-ref/->block-ref test-uuid))] + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content} + {:file/path "pages/referrer.md" + :file/content referring-page-content}]) + (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content}]) + (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) + ))) +(deftest reload-file-with-page-rename + (testing + "Reload a file when the disk contents result in the file having a new page name." + (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9efc" + target-page-content (str "- target block\n id:: " (block-ref/->block-ref test-uuid)) + referring-page-content (str "- " (block-ref/->block-ref test-uuid)) + update-referring-page-content (str "title:: updatedPage\n- " (block-ref/->block-ref test-uuid)) + get-page-block-count (fn [page-name] (let [page-id (:db/id (model/get-page page-name))] + (if (some? page-id) + (model/get-page-blocks-count test-helper/test-db page-id) + 0)))] + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content} + {:file/path "pages/referrer.md" + :file/content referring-page-content}]) + (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid))) + (is (= 1 (get-page-block-count "referrer"))) + (is (= 0 (get-page-block-count "updatedPage"))) + (load-test-files [{:file/path "pages/referrer.md" + :file/content update-referring-page-content}]) + (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) + (is (= 0 (get-page-block-count "referrer"))) + (is (= 2 (get-page-block-count "updatedPage"))) + ))) #_(cljs.test/test-ns 'frontend.db.model-test) From e2fe300da77f73ea490d77e0bdc5e352736702dc Mon Sep 17 00:00:00 2001 From: Gabriel Horner Date: Sat, 26 Nov 2022 01:01:36 -0500 Subject: [PATCH 3/3] Move delete-blocks-fn to graph-parser for reuse for nbb Also: - Move test to a more appropriate ns - model-test isn't for testing higher level parse file behavior - Delete model fns that are no longer used - Fix tests which had incorrect target-page-content and were no longer testing retractAttribute - Add options to cli ns for related nbb reuse - Light cleanup of block deletion --- deps/graph-parser/.carve/ignore | 2 + .../graph-parser/src/logseq/graph_parser.cljs | 80 +++++++++++++++++-- .../src/logseq/graph_parser/cli.cljs | 9 ++- .../test/logseq/graph_parser_test.cljs | 2 +- src/main/frontend/db.cljs | 4 +- src/main/frontend/db/model.cljs | 26 ------ src/main/frontend/handler/common/file.cljs | 58 ++++---------- src/test/frontend/db/model_test.cljs | 47 +---------- .../extensions/zotero/extractor_test.cljs | 1 - src/test/frontend/handler/repo_test.cljs | 47 ++++++++++- 10 files changed, 145 insertions(+), 131 deletions(-) diff --git a/deps/graph-parser/.carve/ignore b/deps/graph-parser/.carve/ignore index 21a5332289..5745e8f143 100644 --- a/deps/graph-parser/.carve/ignore +++ b/deps/graph-parser/.carve/ignore @@ -32,3 +32,5 @@ logseq.graph-parser.property/->block-content logseq.graph-parser.property/property-value-from-content ;; API logseq.graph-parser.whiteboard/page-block->tldr-page +;; API +logseq.graph-parser/get-blocks-to-delete diff --git a/deps/graph-parser/src/logseq/graph_parser.cljs b/deps/graph-parser/src/logseq/graph_parser.cljs index 67e78e62ef..baed594f81 100644 --- a/deps/graph-parser/src/logseq/graph_parser.cljs +++ b/deps/graph-parser/src/logseq/graph_parser.cljs @@ -6,11 +6,77 @@ [logseq.graph-parser.util :as gp-util] [logseq.graph-parser.date-time-util :as date-time-util] [logseq.graph-parser.config :as gp-config] + [logseq.db.schema :as db-schema] [clojure.string :as string] [clojure.set :as set])) +(defn- retract-blocks-tx + [blocks retain-uuids] + (mapcat (fn [{uuid :block/uuid eid :db/id}] + (if (and uuid (contains? retain-uuids uuid)) + (map (fn [attr] [:db.fn/retractAttribute eid attr]) db-schema/retract-attributes) + [[:db.fn/retractEntity eid]])) + blocks)) + +(defn- get-file-page + "Copy of db/get-file-page. Too basic to couple to main app" + [db file-path] + (ffirst + (d/q + '[:find ?page-name + :in $ ?path + :where + [?file :file/path ?path] + [?page :block/file ?file] + [?page :block/original-name ?page-name]] + db + file-path))) + +(defn- get-page-blocks-no-cache + "Copy of db/get-page-blocks-no-cache. Too basic to couple to main app" + [db page {:keys [pull-keys] + :or {pull-keys '[*]}}] + (let [sanitized-page (gp-util/page-name-sanity-lc page) + page-id (:db/id (d/entity db [:block/name sanitized-page]))] + (when page-id + (let [datoms (d/datoms db :avet :block/page page-id) + block-eids (mapv :e datoms)] + (d/pull-many db pull-keys block-eids))))) + +(defn get-blocks-to-delete + "Returns the transactional operations to retract blocks belonging to the + given page name and file path. This function is required when a file is being + parsed from disk; before saving the parsed, blocks from the previous version + of that file need to be retracted. + + The 'Page' parsed from the new file version is passed separately from the + file-path, as the page name can be set via properties in the file, and thus + can change between versions. If it has changed, existing blocks for both the + old and new page name will be retracted. + + Blocks are by default fully cleared via retractEntity. However, a collection + of block UUIDs to retain can be passed, and any blocks with matching uuids + will instead have their attributes cleared individually via + 'retractAttribute'. This will preserve block references to the retained + UUIDs." + [db file-page file-path retain-uuid-blocks] + (let [existing-file-page (get-file-page db file-path) + pages-to-clear (distinct (filter some? [existing-file-page (:block/name file-page)])) + blocks (mapcat (fn [page] + (get-page-blocks-no-cache db page {:pull-keys [:db/id :block/uuid]})) + pages-to-clear) + retain-uuids (set (keep :block/uuid retain-uuid-blocks))] + (retract-blocks-tx (distinct blocks) retain-uuids))) + (defn parse-file - "Parse file and save parsed data to the given db. Main parse fn used by logseq app" + "Parse file and save parsed data to the given db. Main parse fn used by logseq app. +Options available: + +* :new? - Boolean which indicates if this file already exists. Default is true. +* :delete-blocks-fn - Optional fn which is called with the new page, file and existing block uuids + which may be referenced elsewhere. +* :skip-db-transact? - Boolean which skips transacting in order to batch transactions. Default is false +* :extract-options - Options map to pass to extract/extract" [conn file content {:keys [new? delete-blocks-fn extract-options skip-db-transact?] :or {new? true delete-blocks-fn (constantly []) @@ -31,20 +97,20 @@ blocks [] ast []}} (cond (contains? gp-config/mldoc-support-formats format) - (extract/extract file content extract-options') + (extract/extract file content extract-options') - (gp-config/whiteboard? file) - (extract/extract-whiteboard-edn file content extract-options') + (gp-config/whiteboard? file) + (extract/extract-whiteboard-edn file content extract-options') - :else nil) + :else nil) block-ids (map (fn [block] {:block/uuid (:block/uuid block)}) blocks) - delete-blocks (delete-blocks-fn (first pages) file block-ids) + delete-blocks (delete-blocks-fn @conn (first pages) file block-ids) block-refs-ids (->> (mapcat :block/refs blocks) (filter (fn [ref] (and (vector? ref) (= :block/uuid (first ref))))) (map (fn [ref] {:block/uuid (second ref)})) (seq)) - ;; To prevent "unique constraint" on datascript + ;; To prevent "unique constraint" on datascript block-ids (set/union (set block-ids) (set block-refs-ids)) pages (extract/with-ref-pages pages blocks) pages-index (map #(select-keys % [:block/name]) pages)] diff --git a/deps/graph-parser/src/logseq/graph_parser/cli.cljs b/deps/graph-parser/src/logseq/graph_parser/cli.cljs index 884a58e3bd..879c9f1543 100644 --- a/deps/graph-parser/src/logseq/graph_parser/cli.cljs +++ b/deps/graph-parser/src/logseq/graph_parser/cli.cljs @@ -49,7 +49,8 @@ TODO: Fail fast when process exits 1" (mapv (fn [{:file/keys [path content]}] (let [{:keys [ast]} - (graph-parser/parse-file conn path content {:extract-options extract-options})] + (graph-parser/parse-file conn path content (merge {:extract-options extract-options} + (:parse-file-options options)))] {:file path :ast ast})) files))) @@ -59,12 +60,14 @@ TODO: Fail fast when process exits 1" as it can't assume that the metadata in logseq/ is up to date. Directory is assumed to be using git. This fn takes the following options: * :verbose - When enabled prints more information during parsing. Defaults to true -* :files - Specific files to parse instead of parsing the whole directory" +* :files - Specific files to parse instead of parsing the whole directory +* :conn - Database connection to use instead of creating new one +* :parse-file-options - Options map to pass to graph-parser/parse-file" ([dir] (parse-graph dir {})) ([dir options] (let [files (or (:files options) (build-graph-files dir)) - conn (ldb/start-conn) + conn (or (:conn options) (ldb/start-conn)) config (read-config dir) _ (when-not (:files options) (println "Parsing" (count files) "files...")) asts (parse-files conn files (merge options {:config config}))] diff --git a/deps/graph-parser/test/logseq/graph_parser_test.cljs b/deps/graph-parser/test/logseq/graph_parser_test.cljs index fe16d6c5ae..438945d5ee 100644 --- a/deps/graph-parser/test/logseq/graph_parser_test.cljs +++ b/deps/graph-parser/test/logseq/graph_parser_test.cljs @@ -74,7 +74,7 @@ (throw (js/Error "Testing unexpected failure")))] (try (graph-parser/parse-file conn "foo.md" "- id:: 628953c1-8d75-49fe-a648-f4c612109098" - {:delete-blocks-fn (fn [page _file] + {:delete-blocks-fn (fn [_db page _file _uuids] (reset! deleted-page page))}) (catch :default _))) (is (= nil @deleted-page) diff --git a/src/main/frontend/db.cljs b/src/main/frontend/db.cljs index c65d43ea74..7008b91ea3 100644 --- a/src/main/frontend/db.cljs +++ b/src/main/frontend/db.cljs @@ -38,13 +38,13 @@ [frontend.db.model blocks-count blocks-count-cache clean-export! delete-blocks get-pre-block - delete-file-blocks! delete-page-blocks delete-files delete-pages-by-files + delete-files delete-pages-by-files filter-only-public-pages-and-blocks get-all-block-contents get-all-tagged-pages get-all-templates get-block-and-children get-block-by-uuid get-block-children sort-by-left get-block-parent get-block-parents parents-collapsed? get-block-referenced-blocks get-all-referenced-blocks-uuid get-block-children-ids get-block-immediate-children get-block-page get-custom-css get-date-scheduled-or-deadlines - get-file-blocks get-file-last-modified-at get-file get-file-page get-file-page-id file-exists? + get-file-last-modified-at get-file get-file-page get-file-page-id file-exists? get-files get-files-blocks get-files-full get-journals-length get-pages-with-file get-latest-journals get-page get-page-alias get-page-alias-names get-paginated-blocks get-page-blocks-count get-page-blocks-no-cache get-page-file get-page-format get-page-properties diff --git a/src/main/frontend/db/model.cljs b/src/main/frontend/db/model.cljs index e44d103ce2..f0fc87ee5d 100644 --- a/src/main/frontend/db/model.cljs +++ b/src/main/frontend/db/model.cljs @@ -212,17 +212,6 @@ (conn/get-db repo-url) pred) db-utils/seq-flatten))) -(defn get-file-blocks - [repo-url path] - (-> (d/q '[:find ?block - :in $ ?path - :where - [?file :file/path ?path] - [?p :block/file ?file] - [?block :block/page ?p]] - (conn/get-db repo-url) path) - db-utils/seq-flatten)) - (defn set-file-last-modified-at! [repo path last-modified-at] (when (and repo path last-modified-at) @@ -1538,21 +1527,6 @@ [files] (mapv (fn [path] [:db.fn/retractEntity [:file/path path]]) files)) -(defn delete-file-blocks! - [repo-url path] - (let [blocks (get-file-blocks repo-url path)] - (mapv (fn [eid] [:db.fn/retractEntity eid]) blocks))) - -(defn delete-page-blocks - [repo-url page] - (when page - (when-let [db (conn/get-db repo-url)] - (let [page (db-utils/pull [:block/name (util/page-name-sanity-lc page)])] - (when page - (let [datoms (d/datoms db :avet :block/page (:db/id page)) - block-eids (mapv :e datoms)] - (mapv (fn [eid] [:db.fn/retractEntity eid]) block-eids))))))) - (defn delete-pages-by-files [files] (let [pages (->> (mapv get-file-page files) diff --git a/src/main/frontend/handler/common/file.cljs b/src/main/frontend/handler/common/file.cljs index 8a006f5b30..5e3a8ee426 100644 --- a/src/main/frontend/handler/common/file.cljs +++ b/src/main/frontend/handler/common/file.cljs @@ -6,7 +6,6 @@ [frontend.db :as db] ["/frontend/utils" :as utils] [frontend.mobile.util :as mobile-util] - [logseq.db.schema :as db-schema] [logseq.graph-parser :as graph-parser] [logseq.graph-parser.util :as gp-util] [logseq.graph-parser.config :as gp-config] @@ -20,49 +19,20 @@ (when (not= file current-file) current-file)))) -(defn- retract-blocks-tx - [blocks retain-uuids] - (let [tx-for-block (fn [block] (let [{uuid :block/uuid eid :db/id} block - should-retain? (and uuid (contains? retain-uuids uuid))] - (cond - should-retain? - (map (fn [attr] [:db.fn/retractAttribute eid attr]) db-schema/retract-attributes) - :else - [[:db.fn/retractEntity eid]])))] - (mapcat tx-for-block (distinct blocks))) - ) +(defn- validate-existing-file + [repo-url file-page file-path] + (when-let [current-file (page-exists-in-another-file repo-url file-page file-path)] + (when (not= file-path current-file) + (let [error (str "Page already exists with another file: " current-file ", current file: " file-path ". Please keep only one of them and re-index your graph.")] + (state/pub-event! [:notification/show + {:content error + :status :error + :clear? false}]))))) -(defn- retract-file-blocks-tx - "Returns the transactional operations to retract blocks belonging to the - given page name and file path. This function is required when a file is being - parsed from disk; before saving the parsed, blocks from the previous version - of that file need to be retracted. - - The 'Page' parsed from the new file version is passed separately from the - file-path, as the page name can be set via properties in the file, and thus - can change between versions. If it has changed, existing blocks for both the - old and new page name will be retracted. - - Blocks are by default fully cleared via retractEntity. However, a collection - of block UUIDs to retain can be passed, and any blocks with matching uuids - will instead have their attributes cleared individually via - 'retractAttribute'. This will preserve block references to the retained - UUIDs." - [repo-url file-page file-path retain-uuid-blocks] - (let [existing-file-page (db/get-file-page file-path) - pages-to-clear (distinct (filter some? [existing-file-page (:block/name file-page)])) - blocks (mapcat (fn [page] (db/get-page-blocks-no-cache repo-url page {:pull-keys [:db/id :block/uuid]})) pages-to-clear) - retain-uuids (if (seq retain-uuid-blocks) (set (filter some? (map :block/uuid retain-uuid-blocks))) []) - tx (retract-blocks-tx blocks retain-uuids)] - (when-let [current-file (page-exists-in-another-file repo-url file-page file-path)] - (when (not= file-path current-file) - (let [error (str "Page already exists with another file: " current-file ", current file: " file-path ". Please keep only one of them and re-index your graph.")] - (state/pub-event! [:notification/show - {:content error - :status :error - :clear? false}])))) - tx - )) +(defn- validate-and-get-blocks-to-delete + [repo-url db file-page file-path retain-uuid-blocks] + (validate-existing-file repo-url file-page file-path) + (graph-parser/get-blocks-to-delete db file-page file-path retain-uuid-blocks)) (defn reset-file! "Main fn for updating a db with the results of a parsed file" @@ -92,7 +62,7 @@ new? (nil? (db/entity [:file/path file])) options (merge (dissoc options :verbose) {:new? new? - :delete-blocks-fn (partial retract-file-blocks-tx repo-url) + :delete-blocks-fn (partial validate-and-get-blocks-to-delete repo-url) :extract-options (merge {:user-config (state/get-config) :date-formatter (state/get-date-formatter) diff --git a/src/test/frontend/db/model_test.cljs b/src/test/frontend/db/model_test.cljs index c1dbc1ca3d..425fafdae8 100644 --- a/src/test/frontend/db/model_test.cljs +++ b/src/test/frontend/db/model_test.cljs @@ -1,9 +1,7 @@ (ns frontend.db.model-test - (:require [cljs.test :refer [use-fixtures deftest testing is are]] + (:require [cljs.test :refer [use-fixtures deftest is are]] [frontend.db.model :as model] - [frontend.test.helper :as test-helper :refer [load-test-files]] - [logseq.graph-parser.util.block-ref :as block-ref] - )) + [frontend.test.helper :as test-helper :refer [load-test-files]])) (use-fixtures :each {:before test-helper/start-test-db! :after test-helper/destroy-test-db!}) @@ -123,45 +121,4 @@ (#'model/get-unnecessary-namespaces-name '("one/two/tree" "one" "one/two" "non nested tag" "non nested link"))) "Must be one/two one")) -(deftest refs-to-page-maintained-on-reload - (testing - "Refs to blocks on a page are retained if that page is reload." - (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9acd" - target-page-content (str "- target block\n id:: " (block-ref/->block-ref test-uuid)) - referring-page-content (str "- " (block-ref/->block-ref test-uuid))] - (load-test-files [{:file/path "pages/target.md" - :file/content target-page-content} - {:file/path "pages/referrer.md" - :file/content referring-page-content}]) - (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) - (load-test-files [{:file/path "pages/target.md" - :file/content target-page-content}]) - (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) - ))) - -(deftest reload-file-with-page-rename - (testing - "Reload a file when the disk contents result in the file having a new page name." - (let [ test-uuid "16c90195-6a03-4b3f-839d-095a496d9efc" - target-page-content (str "- target block\n id:: " (block-ref/->block-ref test-uuid)) - referring-page-content (str "- " (block-ref/->block-ref test-uuid)) - update-referring-page-content (str "title:: updatedPage\n- " (block-ref/->block-ref test-uuid)) - get-page-block-count (fn [page-name] (let [page-id (:db/id (model/get-page page-name))] - (if (some? page-id) - (model/get-page-blocks-count test-helper/test-db page-id) - 0)))] - (load-test-files [{:file/path "pages/target.md" - :file/content target-page-content} - {:file/path "pages/referrer.md" - :file/content referring-page-content}]) - (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid))) - (is (= 1 (get-page-block-count "referrer"))) - (is (= 0 (get-page-block-count "updatedPage"))) - (load-test-files [{:file/path "pages/referrer.md" - :file/content update-referring-page-content}]) - (is (= (model/get-all-referenced-blocks-uuid) [(parse-uuid test-uuid)])) - (is (= 0 (get-page-block-count "referrer"))) - (is (= 2 (get-page-block-count "updatedPage"))) - ))) - #_(cljs.test/test-ns 'frontend.db.model-test) diff --git a/src/test/frontend/extensions/zotero/extractor_test.cljs b/src/test/frontend/extensions/zotero/extractor_test.cljs index 64ea0fa03f..a57fd5cc16 100644 --- a/src/test/frontend/extensions/zotero/extractor_test.cljs +++ b/src/test/frontend/extensions/zotero/extractor_test.cljs @@ -43,7 +43,6 @@ (is (= 8 authors))) (testing "tags" - (prn (-> properties :tags)) ;; tags split by `,` are counted into different tags ;; https://github.com/logseq/logseq/commit/435c2110bcc2d30ed743ba31375450f1a705b00b (is (= 20 tags))))) diff --git a/src/test/frontend/handler/repo_test.cljs b/src/test/frontend/handler/repo_test.cljs index c6461b966d..7a1de484d2 100644 --- a/src/test/frontend/handler/repo_test.cljs +++ b/src/test/frontend/handler/repo_test.cljs @@ -1,9 +1,11 @@ (ns frontend.handler.repo-test - (:require [cljs.test :refer [deftest use-fixtures]] + (:require [cljs.test :refer [deftest use-fixtures testing is]] [frontend.handler.repo :as repo-handler] - [frontend.test.helper :as test-helper] + [frontend.test.helper :as test-helper :refer [load-test-files]] [logseq.graph-parser.cli :as gp-cli] [logseq.graph-parser.test.docs-graph-helper :as docs-graph-helper] + [logseq.graph-parser.util.block-ref :as block-ref] + [frontend.db.model :as model] [frontend.db.conn :as conn])) (use-fixtures :each {:before test-helper/start-test-db! @@ -19,3 +21,44 @@ db (conn/get-db test-helper/test-db)] (docs-graph-helper/docs-graph-assertions db (map :file/path files)))) + +(deftest parse-files-and-load-to-db-with-block-refs-on-reload + (testing "Refs to blocks on a page are retained if that page is reloaded" + (let [test-uuid "16c90195-6a03-4b3f-839d-095a496d9acd" + target-page-content (str "- target block\n id:: " test-uuid) + referring-page-content (str "- " (block-ref/->block-ref test-uuid))] + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content} + {:file/path "pages/referrer.md" + :file/content referring-page-content}]) + (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid))) + + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content}]) + (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid)))))) + +(deftest parse-files-and-load-to-db-with-page-rename + (testing + "Reload a file when the disk contents result in the file having a new page name" + (let [test-uuid "16c90195-6a03-4b3f-839d-095a496d9efc" + target-page-content (str "- target block\n id:: " test-uuid) + referring-page-content (str "- " (block-ref/->block-ref test-uuid)) + update-referring-page-content (str "title:: updatedPage\n- " (block-ref/->block-ref test-uuid)) + get-page-block-count (fn [page-name] + (let [page-id (:db/id (model/get-page page-name))] + (if (some? page-id) + (model/get-page-blocks-count test-helper/test-db page-id) + 0)))] + (load-test-files [{:file/path "pages/target.md" + :file/content target-page-content} + {:file/path "pages/referrer.md" + :file/content referring-page-content}]) + (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid))) + (is (= 1 (get-page-block-count "referrer"))) + (is (= 0 (get-page-block-count "updatedPage"))) + + (load-test-files [{:file/path "pages/referrer.md" + :file/content update-referring-page-content}]) + (is (= [(parse-uuid test-uuid)] (model/get-all-referenced-blocks-uuid))) + (is (= 0 (get-page-block-count "referrer"))) + (is (= 2 (get-page-block-count "updatedPage"))))))