fix: add validation for property types and :schema attributes

Part of LOG-2953. We shouldn't persist unusuable property type
configurations as shown by this bug. By enumerating what schema
attributes are allowed for each type, we can prevent future bugs
like this. When changing between property types, this also cleans up
:classes, :position and :values that were accidentally hanging around
for certain types. Also modify test since we don't allow users
to use a :default property with :cardinality

deps/db/src/logseq/db/frontend/malli_schema.cljs

@@ -118,10 +118,9 @@
     page-attrs
     page-or-block-attrs)))
 
-(def property-schema-attrs
-  [[:hide? {:optional true} :boolean]
-   [:description {:optional true} :string]
-   ;; For any types except for :checkbox :default :template
+(def property-type-schema-attrs
+  "Property :schema attributes that vary by :type"
+  [;; For any types except for :checkbox :default :template
    [:cardinality {:optional true} [:enum :one :many]]
    ;; For closed values
    [:values {:optional true}  [:vector :uuid]]
@@ -130,28 +129,47 @@
    ;; For :page and :template
    [:classes {:optional true} [:set [:or :uuid :keyword]]]])
 
+(def property-common-schema-attrs
+  "Property :schema attributes common to all properties"
+  [[:hide? {:optional true} :boolean]
+   [:description {:optional true} :string]])
+
 (def internal-property
   (vec
    (concat
     [:map
      [:block/schema
-      (into [:map
-             [:type (apply vector :enum (into db-property-type/internal-builtin-schema-types
-                                              db-property-type/user-builtin-schema-types))]]
-            property-schema-attrs)]]
+      (vec
+       (concat
+        [:map
+         [:type (apply vector :enum (into db-property-type/internal-builtin-schema-types
+                                          db-property-type/user-builtin-schema-types))]]
+        property-common-schema-attrs
+        property-type-schema-attrs))]]
     page-attrs
     page-or-block-attrs)))
 
+(def user-property-schema
+  (into
+   [:multi {:dispatch :type}]
+   (map
+    (fn [prop-type]
+      [prop-type
+       (vec
+        (concat
+         [:map
+          ;; Once a schema is defined it must have :type as this is an irreversible decision
+          [:type :keyword]]
+         property-common-schema-attrs
+         (remove #(not (db-property-type/property-type-allows-schema-attribute? prop-type (first %)))
+                 property-type-schema-attrs)))])
+    db-property-type/user-builtin-schema-types)))
+
 (def user-property
   (vec
    (concat
     [:map
-     [:block/schema
-      {:optional true}
-      (into [:map
-             ;; Once a schema is defined it must have :type as this is an irreversible decision
-             [:type (apply vector :enum db-property-type/user-builtin-schema-types)]]
-            property-schema-attrs)]]
+     [:block/schema {:optional true} user-property-schema]]
     page-attrs
     page-or-block-attrs)))
 

deps/db/src/logseq/db/frontend/property/type.cljs

@@ -1,8 +1,14 @@
 (ns logseq.db.frontend.property.type
-  "Provides property types including fns to validate them"
+  "Provides property types and related helper fns e.g. property value validation
+  fns and their allowed schema attributes"
   (:require [datascript.core :as d]
             [clojure.set :as set]))
 
+;; Config vars
+;; ===========
+;; These vars enumerate all known property types and their associated behaviors
+;; except for validation which is in its own section
+
 (def internal-builtin-schema-types
   "Valid schema :type only to be used by built-in-properties"
   #{:keyword :map :coll :any})
@@ -18,6 +24,22 @@
 (assert (set/subset? closed-values-schema-types (set user-builtin-schema-types))
         "All closed value types are valid property types")
 
+(def ^:private user-built-in-allowed-schema-attributes
+  "Map of types to their set of allowed :schema attributes"
+  (merge-with into
+              (zipmap closed-values-schema-types (repeat #{:values :position}))
+              {:number #{:cardinality}
+               :date #{:cardinality}
+               :url #{:cardinality}
+               :page #{:cardinality :classes}
+               :template #{:classes}
+               :checkbox #{}}))
+
+(assert (= (set user-builtin-schema-types) (set (keys user-built-in-allowed-schema-attributes)))
+        "Each user built in type should have an allowed schema attribute")
+
+;; Property value validation
+;; =========================
 ;; TODO:
 ;; Validate && list fixes for non-validated values when updating property schema
 
@@ -69,6 +91,7 @@
       (type-validate-fn value))))
 
 (def builtin-schema-types
+  "Map of types to malli fns that validate a property value for that type"
   {:default  [:fn
               {:error/message "should be a text"}
               ;; uuid check needed for property block values
@@ -97,9 +120,6 @@
    :coll     coll?
    :any      some?})
 
-(def property-types-with-cardinality
-  #{:number :date :url :page})
-
 (def property-types-with-db
   "Property types whose validation fn requires a datascript db"
   #{:date :page :template})
@@ -108,3 +128,11 @@
            (into internal-builtin-schema-types
                  user-builtin-schema-types))
         "Built-in schema types must be equal")
+
+;; Helper fns
+;; ==========
+(defn property-type-allows-schema-attribute?
+  "Returns boolean to indicate if property type allows the given :schema attribute"
+  [property-type schema-attribute]
+  (contains? (get user-built-in-allowed-schema-attributes property-type)
+             schema-attribute))

scripts/src/logseq/tasks/db_graph/create_graph_with_properties.cljs

@@ -4,6 +4,7 @@
    NOTE: This script is also used in CI to confirm graph creation works"
   (:require [logseq.tasks.db-graph.create-graph :as create-graph]
             [logseq.db.sqlite.util :as sqlite-util]
+            [logseq.db.frontend.property.type :as db-property-type]
             [clojure.string :as string]
             [datascript.core :as d]
             ["path" :as node-path]
@@ -70,7 +71,7 @@
      :properties
      (->> [:default :url :checkbox :number :page :date]
           (mapcat #(cond-> [[% {:block/schema {:type %}}]]
-                     (not (#{:checkbox :default} %))
+                     (db-property-type/property-type-allows-schema-attribute? % :cardinality)
                      (conj [(keyword (str (name %) "-many")) {:block/schema {:type % :cardinality :many}}])))
           (into {}))}))
 

src/main/frontend/components/property.cljs

@@ -189,15 +189,17 @@
             (ui/select schema-types
                        (fn [_e v]
                          (let [type (keyword (string/lower-case v))
-                               update-schema-fn (comp
-                                                 (if (contains? db-property-type/property-types-with-cardinality type)
-                                                   identity
-                                                   #(dissoc % :cardinality))
-                                                 #(assoc % :type type))]
+                               update-schema-fn (apply comp
+                                                       #(assoc % :type type)
+                                                       (keep
+                                                        (fn [attr]
+                                                          (when-not (db-property-type/property-type-allows-schema-attribute? type attr)
+                                                           #(dissoc % attr)))
+                                                        [:values :position :cardinality :classes]))]
                            (swap! *property-schema update-schema-fn)
                            (components-pu/update-property! property @*property-name @*property-schema))))]))]
 
-      (when (contains? db-property-type/property-types-with-cardinality (:type @*property-schema))
+      (when (db-property-type/property-type-allows-schema-attribute? (:type @*property-schema) :cardinality)
         [:div.grid.grid-cols-4.gap-1.items-center.leading-8
          [:label "Multiple values:"]
          (let [many? (boolean (= :many (:cardinality @*property-schema)))]
@@ -208,27 +210,28 @@
                                       (save-property-fn))}))])
 
 
-      (case (:type @*property-schema)
-        :page
-        (when (empty? (:values @*property-schema))
-          [:div.grid.grid-cols-4.gap-1.items-center.leading-8
-           [:label "Specify classes:"]
-           (class-select *property-schema
-                         (:classes @*property-schema)
-                         (assoc opts
-                                :disabled? disabled?
-                                :save-property-fn save-property-fn))])
-
-        :template
-        [:div.grid.grid-cols-4.gap-1.items-center.leading-8
-         [:label "Specify template:"]
-         (class-select *property-schema (:classes @*property-schema)
-                       (assoc opts
-                              :multiple-choices? false
-                              :disabled? disabled?
-                              :save-property-fn save-property-fn))]
-
-        nil)
+      (when (db-property-type/property-type-allows-schema-attribute? (:type @*property-schema) :classes)
+       (case (:type @*property-schema)
+         :page
+         (when (empty? (:values @*property-schema))
+           [:div.grid.grid-cols-4.gap-1.items-center.leading-8
+            [:label "Specify classes:"]
+            (class-select *property-schema
+                          (:classes @*property-schema)
+                          (assoc opts
+                                 :disabled? disabled?
+                                 :save-property-fn save-property-fn))])
+
+         :template
+         [:div.grid.grid-cols-4.gap-1.items-center.leading-8
+          [:label "Specify template:"]
+          (class-select *property-schema (:classes @*property-schema)
+                        (assoc opts
+                               :multiple-choices? false
+                               :disabled? disabled?
+                               :save-property-fn save-property-fn))]
+
+         nil))
 
       (when (and enable-closed-values? (empty? (:classes @*property-schema)))
         [:div.grid.grid-cols-4.gap-1.items-start.leading-8

src/test/frontend/handler/db_based/property_test.cljs

@@ -98,17 +98,17 @@
         2)))
 
   (testing "Add a multi-values property"
-    (db-property-handler/upsert-property! repo "property-3" {:type :default :cardinality :many} {})
-    (db-property-handler/set-block-property! repo fbid "property-3" "value 1" {})
-    (db-property-handler/set-block-property! repo fbid "property-3" "value 2" {})
-    (db-property-handler/set-block-property! repo fbid "property-3" "value 3" {})
+    (db-property-handler/upsert-property! repo "property-3" {:type :number :cardinality :many} {})
+    (db-property-handler/set-block-property! repo fbid "property-3" 1 {})
+    (db-property-handler/set-block-property! repo fbid "property-3" 2 {})
+    (db-property-handler/set-block-property! repo fbid "property-3" 3 {})
     (let [block (db/entity [:block/uuid fbid])
           properties (:block/properties block)
           property (db/entity [:block/name "property-3"])]
       ;; ensure property exists
       (are [x y] (= x y)
         (:block/schema property)
-        {:type :default :cardinality :many}
+        {:type :number :cardinality :many}
         (:block/type property)
         #{"property"})
       ;; check block's properties
@@ -116,10 +116,10 @@
         (count properties)
         3
         (get properties (:block/uuid property))
-        #{"value 1" "value 2" "value 3"}))
+        #{1 2 3}))
 
-    ;; update property value from "value 1" to "value 4"
-    (db-property-handler/set-block-property! repo fbid "property-3" "value 4" {:old-value "value 1"})
+    ;; update property value from 1 to 4
+    (db-property-handler/set-block-property! repo fbid "property-3" 4 {:old-value 1})
     (let [block (db/entity [:block/uuid fbid])
           properties (:block/properties block)
           property (db/entity [:block/name "property-3"])]
@@ -128,7 +128,7 @@
         (count properties)
         3
         (get properties (:block/uuid property))
-        #{"value 4" "value 2" "value 3"})
+        #{4 2 3})
 
       (db-property-handler/delete-property-value! repo block (:block/uuid property) "value 4")
       (let [properties (:block/properties block)]