From 09b598094ee2d27c6bb9d1b296f15feafcf83078 Mon Sep 17 00:00:00 2001 From: Gabriel Horner Date: Tue, 14 Apr 2026 12:15:21 -0400 Subject: [PATCH] fix(cli): multiple upsert commands randomly choose first page found when an ambiguous page is given instead of erroring to ask for the specific page. Like dd6b8975c7c329ddcaf0aa588bdc0700c6db7e4b, there are still several paths where we incorrectly assume :block/name is unique (and don't take into account recyling). This commit fixed the following options: * upsert block --target-page * upsert task --target-page * upsert asset --target-page * upsert page --page There are still two bugs where ambiguous page names leads to random page selection but which can't be addressed easily because there is no way for a user to select by page id in those contexts. Put a TODO next to the ensure-first-page! workaround --- src/main/logseq/cli/command/add.cljs | 53 +++++++++++++++++++- src/main/logseq/cli/command/remove.cljs | 15 +----- src/main/logseq/cli/command/upsert.cljs | 45 +++++++++++------ src/test/logseq/cli/command/upsert_test.cljs | 21 ++++++-- src/test/logseq/cli/commands_test.cljs | 41 +++++++++++++++ 5 files changed, 142 insertions(+), 33 deletions(-) diff --git a/src/main/logseq/cli/command/add.cljs b/src/main/logseq/cli/command/add.cljs index b3851fa2fc..4b682bb05a 100644 --- a/src/main/logseq/cli/command/add.cljs +++ b/src/main/logseq/cli/command/add.cljs @@ -11,6 +11,7 @@ [logseq.common.util.date-time :as date-time-util] [logseq.common.util.page-ref :as page-ref] [logseq.common.uuid :as common-uuid] + [logseq.db :as ldb] [logseq.db.frontend.content :as db-content] [logseq.db.frontend.property :as db-property] [logseq.db.frontend.property.type :as db-property-type] @@ -24,7 +25,55 @@ now (tc/from-date (js/Date.))] (date-time-util/format now formatter))) +(defn find-pages-by-name + "Query all live (non-recycled) pages matching a given name. Returns a vector + of entity maps. Because :block/name is not unique (a tag, property and page + can share the same name), callers should check for ambiguity." + [config repo page-name selector] + (p/let [results (transport/invoke config :thread-api/q false + [repo + [{:find [[(list 'pull '?e selector) '...]] + :in '[$ ?name] + :where '[[?e :block/name ?name]]} + (common-util/page-name-sanity-lc page-name)]])] + (vec (remove ldb/recycled? (or results []))))) + +(defn throw-ambiguous-page-error! + "Throws when multiple pages match the same name. Callers should present the + candidates so the user can rerun with --id." + [page-name matches] + (let [candidates (->> matches + (map (fn [item] + {:id (:db/id item) + :name (or (:block/title item) (:block/name item))})) + (filter :id) + vec)] + (throw (ex-info (str "multiple pages match name: " page-name "; rerun with --id") + {:code :ambiguous-page-name + :candidates candidates})))) + (defn- ensure-page! + "Ensure a page only if it is unique. Otherwise returns an ambiugous error to let user choose the specific + intended page" + [config repo page-name] + (p/let [live (find-pages-by-name config repo page-name + [:db/id :block/uuid :block/name :block/title]) + _ (when (> (count live) 1) + (throw-ambiguous-page-error! page-name live)) + page (first live)] + (if (:db/id page) + page + (let [page-name-lc (common-util/page-name-sanity-lc page-name)] + (p/let [_ (transport/invoke config :thread-api/apply-outliner-ops false + [repo [[:create-page [page-name {}]]] {}])] + (transport/invoke config :thread-api/pull false + [repo [:db/id :block/uuid :block/name :block/title] [:block/name page-name-lc]])))))) + +;; TODO: Replace uses of this fn with ensure-page! when users are able to specify ids for contexts this +;; is used in +(defn- ensure-first-page! + "Unlike ensure-page!, chooses the first random page and doesn't ensure the page is unique. Only use this + when the user unable to specificy the specific page e.g. page refs in a block/title" [config repo page-name] (let [page-name-lc (common-util/page-name-sanity-lc page-name)] (p/let [page (transport/invoke config :thread-api/pull false @@ -246,7 +295,7 @@ page-refs)] (p/let [resolved (p/all (map (fn [[_ page-name]] - (p/let [page (ensure-page! config repo page-name) + (p/let [page (ensure-first-page! config repo page-name) page-uuid (:block/uuid page)] (when-not page-uuid (throw (ex-info "page not found" @@ -705,7 +754,7 @@ (and (string? value) (common-util/uuid-string? (string/trim value))) (resolve-entity-id config repo [:block/uuid (uuid (string/trim value))]) (string? value) - (p/let [page (ensure-page! config repo value)] + (p/let [page (ensure-first-page! config repo value)] (or (:db/id page) (throw (ex-info "page not found" {:code :page-not-found :value value})))) :else diff --git a/src/main/logseq/cli/command/remove.cljs b/src/main/logseq/cli/command/remove.cljs index bb31eb791e..96123be0ad 100644 --- a/src/main/logseq/cli/command/remove.cljs +++ b/src/main/logseq/cli/command/remove.cljs @@ -1,12 +1,12 @@ (ns logseq.cli.command.remove "Remove-related CLI commands." (:require [clojure.string :as string] + [logseq.cli.command.add :as add-command] [logseq.cli.command.core :as core] [logseq.cli.command.id :as id-command] [logseq.cli.server :as cli-server] [logseq.cli.transport :as transport] [logseq.common.util :as common-util] - [logseq.db :as ldb] [promesa.core :as p])) (def ^:private remove-block-spec @@ -186,18 +186,7 @@ (defn- resolve-page-by-name [config repo name] - ;; `:block/name` is not unique — multiple pages can share the same - ;; sanity-lc'd name. Query for ALL matches via :thread-api/q rather than - ;; using :thread-api/pull (which only returns the first hit via - ;; `get-first-page-by-name`), filter out recycled entries, and let the - ;; caller error out on ambiguity instead of randomly removing one of them. - (p/let [results (transport/invoke config :thread-api/q false - [repo - [{:find [[(list 'pull '?e page-id-selector) '...]] - :in '[$ ?name] - :where '[[?e :block/name ?name]]} - (common-util/page-name-sanity-lc name)]]) - live (vec (remove ldb/recycled? (or results [])))] + (p/let [live (add-command/find-pages-by-name config repo name page-id-selector)] (cond (empty? live) nil (> (count live) 1) {:ambiguous? true :matches live} diff --git a/src/main/logseq/cli/command/upsert.cljs b/src/main/logseq/cli/command/upsert.cljs index 61b98160e6..b5eebfa2f0 100644 --- a/src/main/logseq/cli/command/upsert.cljs +++ b/src/main/logseq/cli/command/upsert.cljs @@ -709,9 +709,12 @@ (defn- ensure-page-entity! [config repo page-name] - (p/let [existing (pull-page-by-name config repo page-name - [:db/id :block/uuid :logseq.property/deleted-at])] - (if (and (:db/id existing) (not (ldb/recycled? existing))) + (p/let [live (add-command/find-pages-by-name config repo page-name + [:db/id :block/uuid]) + _ (when (> (count live) 1) + (add-command/throw-ambiguous-page-error! page-name live)) + existing (first live)] + (if (:db/id existing) existing ;; Either no page exists, or only a recycled one does. Calling ;; :create-page in both cases is correct: outliner-page/create has a @@ -1024,8 +1027,10 @@ :data {:result created-ids}})) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) (defn execute-upsert-page [action config] @@ -1064,8 +1069,10 @@ :data {:result [page-id]}}) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) (defn- normalize-status-input [value] @@ -1128,8 +1135,10 @@ :message "invalid upsert task mode"}})))) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) (defn- asset-file-exists? [path] @@ -1256,8 +1265,10 @@ :message "invalid upsert asset mode"}})) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) (defn execute-upsert-tag [action config] @@ -1313,8 +1324,10 @@ :data {:result [page-id]}}))))) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) (defn execute-upsert-property [action config] @@ -1354,5 +1367,7 @@ :message "property not found after upsert"}}))))) (p/catch (fn [e] {:status :error - :error {:code (or (get-in (ex-data e) [:code]) :exception) - :message (or (ex-message e) (str e))}})))) + :error (merge {:code (or (:code (ex-data e)) :exception) + :message (or (ex-message e) (str e))} + (when-let [candidates (:candidates (ex-data e))] + {:candidates candidates}))})))) diff --git a/src/test/logseq/cli/command/upsert_test.cljs b/src/test/logseq/cli/command/upsert_test.cljs index 74802ce0b0..6a56d47e8d 100644 --- a/src/test/logseq/cli/command/upsert_test.cljs +++ b/src/test/logseq/cli/command/upsert_test.cljs @@ -363,9 +363,12 @@ transport/invoke (fn [_ method _ args] (case method :thread-api/q - (p/resolved [:logseq.property/status.todo - :logseq.property/status.doing - :logseq.property/status.done]) + (let [[_ [_query input]] args] + (if (= input "taskhome") + (p/resolved [{:db/id 42 :block/uuid (uuid "00000000-0000-0000-0000-000000000042")}]) + (p/resolved [:logseq.property/status.todo + :logseq.property/status.doing + :logseq.property/status.done]))) :thread-api/pull (let [[_ selector lookup] args] @@ -421,6 +424,12 @@ (p/resolved (assoc config :base-url "http://example"))) transport/invoke (fn [_ method _ args] (case method + :thread-api/q + (let [[_ [_query input]] args] + (if (= input "taskhome") + (p/resolved [{:db/id 42 :block/uuid (uuid "00000000-0000-0000-0000-000000000042")}]) + (p/resolved []))) + :thread-api/pull (let [[_ selector lookup] args] (cond @@ -474,6 +483,12 @@ (p/resolved (assoc config :base-url "http://example"))) transport/invoke (fn [_ method _ args] (case method + :thread-api/q + (let [[_ [_query input]] args] + (if (= input "taskhome") + (p/resolved [{:db/id 42 :block/uuid (uuid "00000000-0000-0000-0000-000000000042")}]) + (p/resolved []))) + :thread-api/pull (let [[_ selector lookup] args] (cond diff --git a/src/test/logseq/cli/commands_test.cljs b/src/test/logseq/cli/commands_test.cljs index 7527e72964..152f3f8038 100644 --- a/src/test/logseq/cli/commands_test.cljs +++ b/src/test/logseq/cli/commands_test.cljs @@ -3351,6 +3351,10 @@ add-command/resolve-property-identifiers (fn [_ _ properties & _] (p/resolved properties)) transport/invoke (fn [_ method _ args] (case method + :thread-api/q (let [[_ [_query input]] args] + (if (= input "home") + [{:db/id 50 :block/uuid (uuid "00000000-0000-0000-0000-000000000050")}] + [])) :thread-api/pull (let [[_ _ lookup] args] (cond (= lookup [:block/name "home"]) @@ -3391,6 +3395,12 @@ add-command/resolve-property-identifiers (fn [_ _ properties & _] (p/resolved properties)) transport/invoke (fn [_ method _ args] (case method + :thread-api/q (let [[_ [_query input]] args] + (if (= input "home") + [{:db/id 50 + :block/uuid recycled-uuid + :logseq.property/deleted-at 1712000000000}] + [])) :thread-api/pull (let [[_ _ lookup] args] (cond (= lookup [:block/name "home"]) @@ -3415,6 +3425,37 @@ (p/catch (fn [e] (is false (str "unexpected error: " e)))) (p/finally done))))) +(deftest test-execute-upsert-page-errors-on-ambiguous-name + (async done + (let [action {:type :upsert-page :repo "demo" :page "c1" + :update-tags [:tag/next]}] + (-> (p/with-redefs [cli-server/list-graphs (fn [_] ["demo"]) + cli-server/ensure-server! (fn [_ _] {:base-url "http://example"}) + add-command/resolve-tags (fn [_ _ _] (p/resolved nil)) + add-command/resolve-properties (fn [_ _ _ & _] (p/resolved nil)) + add-command/resolve-property-identifiers (fn [_ _ _ & _] (p/resolved nil)) + transport/invoke (fn [_ method _ _] + (case method + :thread-api/q [{:db/id 21 + :block/name "c1" + :block/title "c1" + :block/uuid (uuid "00000000-0000-0000-0000-0000000000c1")} + {:db/id 22 + :block/name "c1" + :block/title "c1" + :block/uuid (uuid "00000000-0000-0000-0000-0000000000c2")}] + :thread-api/apply-outliner-ops + (throw (ex-info "should not modify on ambiguous match" {:method method})) + (throw (ex-info "unexpected invoke" {:method method}))))] + (p/let [result (commands/execute action {})] + (is (= :error (:status result))) + (is (= :ambiguous-page-name (get-in result [:error :code]))) + (is (= #{21 22} + (set (map :id (get-in result [:error :candidates]))))) + (is (string/includes? (get-in result [:error :message]) "rerun with --id")))) + (p/catch (fn [e] (is false (str "unexpected error: " e)))) + (p/finally done))))) + (deftest test-execute-upsert-page-by-id-rejects-recycled-page (async done (let [action {:type :upsert-page :mode :update :repo "demo" :id 50}]