From f5e339293faee08bd1c4a3cc8b4a257891cea996 Mon Sep 17 00:00:00 2001 From: Gabriel Horner Date: Thu, 30 Nov 2023 14:47:44 -0500 Subject: [PATCH] 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 --- .../src/logseq/db/frontend/malli_schema.cljs | 46 +++++++++++----- .../src/logseq/db/frontend/property/type.cljs | 36 +++++++++++-- .../create_graph_with_properties.cljs | 3 +- src/main/frontend/components/property.cljs | 53 ++++++++++--------- .../handler/db_based/property_test.cljs | 18 +++---- 5 files changed, 103 insertions(+), 53 deletions(-) diff --git a/deps/db/src/logseq/db/frontend/malli_schema.cljs b/deps/db/src/logseq/db/frontend/malli_schema.cljs index 1920de57a6..a91d2e6b93 100644 --- a/deps/db/src/logseq/db/frontend/malli_schema.cljs +++ b/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))) diff --git a/deps/db/src/logseq/db/frontend/property/type.cljs b/deps/db/src/logseq/db/frontend/property/type.cljs index 01f0b0045d..f733cda013 100644 --- a/deps/db/src/logseq/db/frontend/property/type.cljs +++ b/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)) diff --git a/scripts/src/logseq/tasks/db_graph/create_graph_with_properties.cljs b/scripts/src/logseq/tasks/db_graph/create_graph_with_properties.cljs index 4970e0f993..9d5575500d 100644 --- a/scripts/src/logseq/tasks/db_graph/create_graph_with_properties.cljs +++ b/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 {}))})) diff --git a/src/main/frontend/components/property.cljs b/src/main/frontend/components/property.cljs index b956f19a8c..1cae60d03c 100644 --- a/src/main/frontend/components/property.cljs +++ b/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))]) + (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))] + :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) + nil)) (when (and enable-closed-values? (empty? (:classes @*property-schema))) [:div.grid.grid-cols-4.gap-1.items-start.leading-8 diff --git a/src/test/frontend/handler/db_based/property_test.cljs b/src/test/frontend/handler/db_based/property_test.cljs index 62e576f681..68b96a073b 100644 --- a/src/test/frontend/handler/db_based/property_test.cljs +++ b/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)]