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 dd6b8975c7, 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

@@ -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

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}

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}))}))))

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

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}]