enhance: ignore blank property values from templates

to improves property type detection. Also fixed a bug in
upstream-properties detection and added more docs + better names related
to it. Part of LOG-2985

deps/graph-parser/src/logseq/graph_parser/exporter.cljs

@@ -184,11 +184,15 @@
        (into {})))
 
 (defn- handle-changed-property
-  "Handles converting a property value whose :type has changed. Returns the changed
-   value or nil if the property is to be ignored"
+  "Handles a property's schema changing across blocks. Handling usually means
+  converting a property value to a new changed value or nil if the property is
+  to be ignored. Sometimes handling a property change results in changing a
+  property's previous usages instead of its current value e.g. when changing to
+  a :default type. This is done by adding an entry to upstream-properties and
+  building the additional tx to ensure this happens"
   [val prop prop-name->uuid properties-text-values
    {:keys [ignored-properties property-schemas]}
-   {:keys [property-changes log-fn properties-to-change]}]
+   {:keys [property-changes log-fn upstream-properties]}]
   (let [type-change (get-in property-changes [prop :type])]
     (cond
       ;; ignore :to as any property value gets stringified
@@ -198,17 +202,17 @@
       ;; treat it the same as a :page
       (set (map (comp prop-name->uuid common-util/page-name-sanity-lc) val))
       ;; Unlike the other property changes, this one changes all the previous values of a property
-      ;; in order to accomodate the change
+      ;; in order to accommodate the change
       (= :default (:to type-change))
-      (if (get @properties-to-change prop)
+      (if (get @upstream-properties prop)
         ;; Ignore more than one property schema change per file to keep it simple
         (do
           (log-fn :prop-to-change-ignored {:property prop :val val :change type-change})
           (swap! ignored-properties conj {:property prop :value val :schema (get property-changes prop)})
           nil)
         (do
-          (swap! properties-to-change assoc prop {:schema {:type :default}})
-          (swap! property-schemas assoc prop {:schema {:type :default}})
+          (swap! upstream-properties assoc prop {:schema {:type :default}})
+          (swap! property-schemas assoc prop {:type :default})
           (get properties-text-values prop)))
       :else
       (do
@@ -216,6 +220,11 @@
         (swap! ignored-properties conj {:property prop :value val :schema (get property-changes prop)})
         nil))))
 
+(defn- user-property-value-to-ignore?
+  [val]
+  ;; Ignore blank values as they were usually generated by templates
+  (and (string? val) (string/blank? val)))
+
 (defn- update-user-property-values
   [props prop-name->uuid properties-text-values
    {:keys [property-schemas] :as import-state}
@@ -225,13 +234,14 @@
                (if (get-in property-changes [prop :type])
                  (when-let [val' (handle-changed-property val prop prop-name->uuid properties-text-values import-state options)]
                    [prop val'])
-                 [prop
-                  (if (set? val)
-                    (if (= :default (get-in @property-schemas [prop :type]))
-                      (get properties-text-values prop)
+                 (when-not (user-property-value-to-ignore? val)
+                   [prop
+                    (if (set? val)
+                      (if (= :default (get-in @property-schemas [prop :type]))
+                        (get properties-text-values prop)
                       ;; assume for now a ref's :block/name can always be translated by lc helper
-                      (set (map (comp prop-name->uuid common-util/page-name-sanity-lc) val)))
-                    val)])))
+                        (set (map (comp prop-name->uuid common-util/page-name-sanity-lc) val)))
+                      val)]))))
        (into {})))
 
 (defn- cached-prop-name->uuid [db page-names-to-uuids k]
@@ -295,7 +305,8 @@
               (->> properties-to-infer
                    (keep (fn [[prop val]]
                            (when-let [property-change
-                                      (infer-property-schema-and-get-property-change val prop (get (:block/properties-text-values block) prop) refs (:property-schemas import-state) macros)]
+                                      (and (not (user-property-value-to-ignore? val))
+                                           (infer-property-schema-and-get-property-change val prop (get (:block/properties-text-values block) prop) refs (:property-schemas import-state) macros))]
                              [prop property-change])))
                    (into {}))
               _ (when (seq property-changes) (log-fn :prop-changes property-changes))
@@ -452,14 +463,16 @@
     {:pages-tx pages-tx
      :page-names-to-uuids page-names-to-uuids}))
 
-(defn- build-properties-to-change-tx
-  [db page-names-to-uuids properties-to-change text-values-by-uuid log-fn]
-  (if (seq properties-to-change)
+(defn- build-upstream-properties-tx
+  "Builds tx for upstream properties that have changed. Upstream properties can be properties that
+   already exist in the DB from another file or from earlier uses of a property in the same file"
+  [db page-names-to-uuids upstream-properties text-values-by-uuid log-fn]
+  (if (seq upstream-properties)
     (do
-      (log-fn :props-upstream-to-change properties-to-change)
+      (log-fn :props-upstream-to-change upstream-properties)
       (mapcat
        (fn [[prop {:keys [schema]}]]
-       ;; property schema change
+         ;; property schema change
          (let [prop-uuid (cached-prop-name->uuid db page-names-to-uuids prop)
                block-vals-to-update (map first
                                          (d/q '[:find (pull ?b [:block/uuid :block/properties])
@@ -470,7 +483,7 @@
                                               prop
                                               (rules/extract-rules rules/db-query-dsl-rules)))]
            (into [{:block/name (name prop) :block/schema schema}]
-               ;; property value changes
+                 ;; property value changes
                  (when (= :default (:type schema))
                    (mapv #(hash-map :block/uuid (:block/uuid %)
                                     :block/properties
@@ -480,17 +493,36 @@
                                                                           {:property prop
                                                                            :block/uuid (:block/uuid %)})))}))
                          block-vals-to-update)))))
-       properties-to-change))
+       upstream-properties))
     []))
 
 (defn new-import-state
   "New import state that is used in add-file-to-db-graph. State is atom per
    key to make code more readable and encourage local mutations"
   []
-  {:ignored-properties (atom [])
+  {;; Vec of maps with keys :property, :value, :schema and :location.
+   ;; Properties are ignored to keep graph valid and notify users of ignored properties.
+   ;; Properties with :schema are ignored due to property schema changes
+   :ignored-properties (atom [])
+   ;; Map of property names (keyword) and their current schemas (map).
+   ;; Used for adding schemas to properties and detecting changes across a property's usage
    :property-schemas (atom {})
+   ;; Map of block uuids to their :block/properties-text-values value.
+   ;; Used if a property value changes to :default
    :block-properties-text-values (atom {})})
 
+(defn- build-tx-options [{:keys [user-options] :as options}]
+  (merge
+   (dissoc options :extract-options :user-options)
+   {:import-state (or (:import-state options) (new-import-state))
+    ;; Track per file changes to make to existing properties
+    ;; Map of property names (keyword) and their changes (map)
+    :upstream-properties (atom {})
+    :tag-classes (set (map string/lower-case (:tag-classes user-options)))
+    :property-classes (set/difference
+                       (set (map (comp keyword string/lower-case) (:property-classes user-options)))
+                       db-property/built-in-properties-keys)}))
+
 (defn add-file-to-db-graph
   "Parse file and save parsed data to the given db graph. Options available:
   
@@ -502,11 +534,12 @@
 * :macros - map of macros for use with macro expansion
 * :notify-user - Displays warnings to user without failing the import. Fn receives a map with :msg
 * :log-fn - Logs messages for development. Defaults to prn"
-  [conn file content {:keys [extract-options user-options notify-user log-fn]
+  [conn file content {:keys [extract-options notify-user log-fn]
                       :or {notify-user #(println "[WARNING]" (:msg %))
                            log-fn prn}
-                      :as options}]
-  (let [format (common-util/get-format file)
+                      :as *options}]
+  (let [options (assoc *options :notify-user notify-user :log-fn log-fn)
+        format (common-util/get-format file)
         extract-options' (merge {:block-pattern (common-config/get-block-pattern format)
                                  :date-formatter "MMM do, yyyy"
                                  :uri-encoded? false
@@ -523,17 +556,7 @@
 
               :else
               (notify-user {:msg (str "Skipped file since its format is not supported: " file)}))
-        tx-options (merge
-                    (dissoc options :extract-options :user-options)
-                    {:import-state (or (:import-state options) (new-import-state))
-                     ;; Track per file changes to make to existing properties
-                     :properties-to-change (atom {})
-                     :notify-user notify-user
-                     :log-fn log-fn
-                     :tag-classes (set (map string/lower-case (:tag-classes user-options)))
-                     :property-classes (set/difference
-                                        (set (map (comp keyword string/lower-case) (:property-classes user-options)))
-                                        db-property/built-in-properties-keys)})
+        tx-options (build-tx-options options)
         ;; Build page and block txs
         {:keys [pages-tx page-names-to-uuids]} (build-pages-tx conn pages blocks tx-options)
         whiteboard-pages (->> pages-tx
@@ -550,10 +573,10 @@
                        (remove :block/pre-block?)
                        (mapv #(build-block-tx @conn % pre-blocks page-names-to-uuids
                                               (assoc tx-options :whiteboard? (some? (seq whiteboard-pages))))))
-        properties-to-change-tx (build-properties-to-change-tx
+        upstream-properties-tx (build-upstream-properties-tx
                                  @conn
                                  page-names-to-uuids
-                                 @(:properties-to-change tx-options)
+                                 @(:upstream-properties tx-options)
                                  @(get-in tx-options [:import-state :block-properties-text-values])
                                  log-fn)
         ;; Build indices
@@ -566,7 +589,7 @@
                             (seq))
         ;; To prevent "unique constraint" on datascript
         block-ids (set/union (set block-ids) (set block-refs-ids))
-        tx (concat whiteboard-pages pages-index pages-tx block-ids blocks-tx properties-to-change-tx)
+        tx (concat whiteboard-pages pages-index pages-tx block-ids blocks-tx upstream-properties-tx)
         tx' (common-util/fast-remove-nils tx)
         result (d/transact! conn tx')]
     result))