Add validation for repo config.edn

- Add test, task and schema for repo config.edn
- Also removed no longer needed temp fix

bb.edn

@@ -63,8 +63,11 @@
   dev:validate-plugins-edn
   logseq.tasks.malli/validate-plugins-edn
 
-  dev:validate-config-edn
-  logseq.tasks.malli/validate-config-edn
+  dev:validate-repo-config-edn
+  logseq.tasks.malli/validate-repo-config-edn
+
+  dev:validate-global-config-edn
+  logseq.tasks.malli/validate-global-config-edn
 
   dev:lint
   logseq.tasks.dev/lint

scripts/src/logseq/tasks/malli.clj

@@ -4,6 +4,7 @@
             [malli.error :as me]
             [frontend.schema.handler.plugin-config :as plugin-config-schema]
             [frontend.schema.handler.global-config :as global-config-schema]
+            [frontend.schema.handler.repo-config :as repo-config-schema]
             [clojure.pprint :as pprint]
             [clojure.edn :as edn]))
 
@@ -20,16 +21,25 @@
       (pprint/pprint errors))
     (println "Valid!")))
 
-;; This fn should be split if the global and repo definitions diverge
-(defn validate-config-edn
-  "Validate a global or repo config.edn file"
-  [file]
+(defn- validate-file-with-schema
+  "Validate a file given its schema"
+  [file schema]
   (if-let [errors (->> file
                        slurp
                        edn/read-string
-                       (m/explain global-config-schema/Config-edn)
+                       (m/explain schema)
                        me/humanize)]
     (do
       (println "Found errors:")
       (pprint/pprint errors))
     (println "Valid!")))
+
+(defn validate-repo-config-edn
+  "Validate a repo config.edn"
+  [file]
+  (validate-file-with-schema file global-config-schema/Config-edn))
+
+(defn validate-global-config-edn
+  "Validate a global config.edn"
+  [file]
+  (validate-file-with-schema file repo-config-schema/Config-edn))

src/main/frontend/handler/common/config_edn.cljs

@@ -0,0 +1,65 @@
+(ns frontend.handler.common.config-edn
+  "Common fns related to config.edn - global and repo"
+  (:require [malli.error :as me]
+            [malli.core :as m]
+            [goog.string :as gstring]
+            [clojure.string :as string]
+            [clojure.edn :as edn]
+            [frontend.handler.notification :as notification]))
+
+(defn- humanize-more
+  "Make error maps from me/humanize more readable for users. Doesn't try to handle
+nested keys or positional errors e.g. tuples"
+  [errors]
+  (map
+   (fn [[k v]]
+     (if (map? v)
+       [k (str "Has errors in the following keys - " (string/join ", " (keys v)))]
+       ;; Only show first error since we don't have a use case yet for multiple yet
+       [k (->> v flatten (remove nil?) first)]))
+   errors))
+
+(defn- validate-config-map
+  [m schema path]
+  (if-let [errors (->> m (m/explain schema) me/humanize)]
+    (do
+      (notification/show! (gstring/format "The file '%s' has the following errors:\n%s"
+                                          path
+                                          (->> errors
+                                               humanize-more
+                                               (map (fn [[k v]]
+                                                      (str k " - " v)))
+                                               (string/join "\n")))
+                          :error)
+      false)
+    true))
+
+(defn validate-config-edn
+  "Validates a global config.edn file for correctness and pops up an error
+  notification if invalid. Returns a boolean indicating if file is invalid.
+  Error messages are written with consideration that this validation is called
+  regardless of whether a file is written outside or inside Logseq."
+  [path file-body schema]
+  (let [parsed-body (try
+                      (edn/read-string file-body)
+                      (catch :default _ ::failed-to-read))]
+    (cond
+      (nil? parsed-body)
+      true
+
+      (= ::failed-to-read parsed-body)
+      (do
+        (notification/show! (gstring/format "Failed to read file '%s'. Make sure your config is wrapped
+in {}. Also make sure that the characters '( { [' have their corresponding closing character ') } ]'."
+                                            path)
+                            :error)
+        false)
+      ;; Custom error message is better than malli's "invalid type" error
+      (not (map? parsed-body))
+      (do
+        (notification/show! (gstring/format "The file '%s' is not valid. Make sure the config is wrapped in {}."
+                                            path)
+                            :error)
+        false)
+      :else
+      (validate-config-map parsed-body schema path))))

src/main/frontend/handler/file.cljs

@@ -7,9 +7,12 @@
             [frontend.fs.nfs :as nfs]
             [frontend.fs.capacitor-fs :as capacitor-fs]
             [frontend.handler.common.file :as file-common-handler]
+            [frontend.handler.common.config-edn :as config-edn-common-handler]
             [frontend.handler.repo-config :as repo-config-handler]
             [frontend.handler.global-config :as global-config-handler]
             [frontend.handler.ui :as ui-handler]
+            [frontend.schema.handler.global-config :as global-config-schema]
+            [frontend.schema.handler.repo-config :as repo-config-schema]
             [frontend.state :as state]
             [frontend.util :as util]
             [logseq.graph-parser.util :as gp-util]
@@ -88,11 +91,17 @@
 (defn- validate-file
   "Returns true if valid and if false validator displays error message. Files
   that are not validated just return true"
-  [path content]
-  (if (and
-       (config/global-config-enabled?)
-       (= (path/dirname path) (global-config-handler/global-config-dir)))
-    (global-config-handler/validate-config-edn path content)
+  [repo path content]
+  (cond
+    (= path (config/get-repo-config-path repo))
+    (config-edn-common-handler/validate-config-edn path content repo-config-schema/Config-edn)
+
+    (and
+     (config/global-config-enabled?)
+     (= (path/dirname path) (global-config-handler/global-config-dir)))
+    (config-edn-common-handler/validate-config-edn path content global-config-schema/Config-edn)
+
+    :else
     true))
 
 (defn- validate-and-write-file
@@ -109,7 +118,7 @@
         write-file-options' (merge write-file-options
                                    (when original-content {:old-content original-content}))]
     (p/do!
-     (if (validate-file path content)
+     (if (validate-file repo path content)
        (do
          (fs/write-file! repo path-dir path content write-file-options')
          true)
@@ -125,7 +134,7 @@
                            skip-compare? false}}]
   (let [path (gp-util/path-normalize path)
         write-file! (if from-disk?
-                      #(p/promise (validate-file path content))
+                      #(p/promise (validate-file repo path content))
                       #(validate-and-write-file repo path content {:skip-compare? skip-compare?}))
         opts {:new-graph? new-graph?
               :from-disk? from-disk?
@@ -149,7 +158,8 @@
                        (= path (config/get-custom-css-path repo))
                        (ui-handler/add-style-if-exists!)
 
-                       (= path (config/get-repo-config-path repo))
+                       (and (= path (config/get-repo-config-path repo))
+                            valid-result?)
                        (p/let [_ (repo-config-handler/restore-repo-config! repo content)]
                               (state/pub-event! [:shortcut/refresh]))
 

src/main/frontend/handler/global_config.cljs

@@ -4,16 +4,10 @@
   component depends on a repo."
   (:require [frontend.fs :as fs]
             [frontend.handler.common.file :as file-common-handler]
-            [frontend.handler.notification :as notification]
-            [frontend.schema.handler.global-config :as global-config-schema]
             [frontend.state :as state]
             [promesa.core :as p]
             [shadow.resource :as rc]
-            [malli.error :as me]
-            [malli.core :as m]
-            [goog.string :as gstring]
             [clojure.edn :as edn]
-            [clojure.string :as string]
             [electron.ipc :as ipc]
             ["path" :as path]))
 
@@ -62,63 +56,6 @@
     (p/let [config-content (fs/read-file config-dir config-path)]
            (set-global-config-state! config-content))))
 
-(defn- humanize-more
-  "Make error maps from me/humanize more readable for users. Doesn't try to handle
-nested keys or positional errors e.g. tuples"
-  [errors]
-  (map
-   (fn [[k v]]
-     (if (map? v)
-       [k (str "Has errors in the following keys - " (string/join ", " (keys v)))]
-       ;; Only show first error since we don't have a use case yet for multiple yet
-       [k (->> v flatten (remove nil?) first)]))
-   errors))
-
-(defn- validate-config-map
-  [m path]
-  (if-let [errors (->> m (m/explain global-config-schema/Config-edn) me/humanize)]
-    (do
-      (notification/show! (gstring/format "The file '%s' has the following errors:\n%s"
-                                          path
-                                          (->> errors
-                                               humanize-more
-                                               (map (fn [[k v]]
-                                                      (str k " - " v)))
-                                               (string/join "\n")))
-                          :error)
-      false)
-    true))
-
-(defn validate-config-edn
-  "Validates a global config.edn file for correctness and pops up an error
-  notification if invalid. Returns a boolean indicating if file is invalid.
-  Error messages are written with consideration that this validation is called
-  regardless of whether a file is written outside or inside Logseq."
-  [path file-body]
-  (let [parsed-body (try
-                      (edn/read-string file-body)
-                      (catch :default _ ::failed-to-read))]
-    (cond
-      (nil? parsed-body)
-      true
-
-      (= ::failed-to-read parsed-body)
-      (do
-        (notification/show! (gstring/format "Failed to read file '%s'. Make sure your config is wrapped
-in {}. Also make sure that the characters '( { [' have their corresponding closing character ') } ]'."
-                                            path)
-                            :error)
-        false)
-      ;; Custom error message is better than malli's "invalid type" error
-      (not (map? parsed-body))
-      (do
-        (notification/show! (gstring/format "The file '%s' is not valid. Make sure the config is wrapped in {}."
-                                            path)
-                            :error)
-        false)
-      :else
-      (validate-config-map parsed-body path))))
-
 (defn start
   "This component has four responsibilities on start:
 - Fetch root-dir for later use with config paths

src/main/frontend/schema/handler/repo_config.cljc

@@ -0,0 +1,9 @@
+(ns frontend.schema.handler.repo-config
+  "Malli schemas for repo-config"
+  (:require [frontend.schema.handler.common-config :as common-config]))
+
+;; For now this just references a common schema but repo-config and
+;; global-config could diverge
+(def Config-edn
+  "Schema for repo config.edn"
+  common-config/Config-edn)

src/main/frontend/state.cljs

@@ -331,11 +331,6 @@
              new))
          configs))
 
-(defn validate-current-config
-  "TODO: Temporal fix"
-  [config]
-  (when (map? config) config))
-
 (defn get-config
   "User config for the given repo or current repo if none given. All config fetching
 should be done through this fn in order to get global config and config defaults"
@@ -345,7 +340,7 @@ should be done through this fn in order to get global config and config defaults
    (merge-configs
     default-config
     (get-in @state [:config ::global-config])
-    (validate-current-config (get-in @state [:config repo-url])))))
+    (get-in @state [:config repo-url]))))
 
 (defonce publishing? (atom nil))
 
@@ -557,10 +552,10 @@ Similar to re-frame subscriptions"
   "Sub equivalent to get-config which should handle all sub user-config access"
   ([] (sub-config (get-current-repo)))
   ([repo]
-   (let [config (validate-current-config (sub :config))]
+   (let [config (sub :config)]
      (merge-configs default-config
                     (get config ::global-config)
-                    (validate-current-config (get config repo))))))
+                    (get config repo)))))
 
 (defn enable-grammarly?
   []

src/test/frontend/handler/common/config_edn_test.cljs

@@ -0,0 +1,45 @@
+(ns frontend.handler.common.config-edn-test
+  (:require [clojure.test :refer [is testing deftest]]
+            [clojure.string :as string]
+            [frontend.handler.common.config-edn :as config-edn-common-handler]
+            [frontend.schema.handler.global-config :as global-config-schema]
+            [frontend.schema.handler.repo-config :as repo-config-schema]
+            [frontend.handler.notification :as notification]))
+
+(defn- validation-config-error-for
+  [config-body schema]
+  (let [error-message (atom nil)]
+    (with-redefs [notification/show! (fn [msg _] (reset! error-message msg))]
+      (is (= false
+             (config-edn-common-handler/validate-config-edn "config.edn" config-body schema)))
+      (str @error-message))))
+
+(deftest validate-config-edn
+  (testing "Valid cases"
+    (is (= true
+           (config-edn-common-handler/validate-config-edn
+            "config.edn" "{:preferred-workflow :todo}" global-config-schema/Config-edn))
+        "global config.edn")
+
+    (is (= true
+           (config-edn-common-handler/validate-config-edn
+            "config.edn" "{:preferred-workflow :todo}" repo-config-schema/Config-edn))
+        "repo config.edn"))
+
+  (doseq [[file-type schema] {"global config.edn" global-config-schema/Config-edn
+                              "repo config.edn" repo-config-schema/Config-edn}]
+    (testing (str "Invalid cases for " file-type)
+      (is (string/includes?
+           (validation-config-error-for ":export/bullet-indentation :two-spaces" schema)
+           "wrapped in {}")
+          (str "Not a map for " file-type))
+
+      (is (string/includes?
+           (validation-config-error-for "{:preferred-workflow :todo" schema)
+           "Failed to read")
+          (str "Invalid edn for " file-type))
+
+      (is (string/includes?
+           (validation-config-error-for "{:start-of-week 7}" schema)
+           "has the following errors")
+          (str "Invalid map for " file-type)))))

src/test/frontend/handler/global_config_test.cljs

@@ -1,32 +0,0 @@
-(ns frontend.handler.global-config-test
-  (:require [clojure.test :refer [is testing deftest]]
-            [frontend.handler.global-config :as global-config-handler]
-            [clojure.string :as string]
-            [frontend.handler.notification :as notification]))
-
-(defn- validation-config-error-for
-  [config-body]
-  (let [error-message (atom nil)]
-      (with-redefs [notification/show! (fn [msg _] (reset! error-message msg))]
-        (is (= false
-               (global-config-handler/validate-config-edn "config.edn" config-body)))
-        (str @error-message))))
-
-(deftest validate-config-edn
-  (testing "Valid cases"
-    (is (= true
-           (global-config-handler/validate-config-edn
-            "config.edn" "{:preferred-workflow :todo}"))))
-
-  (testing "Invalid cases"
-    (is (string/includes?
-         (validation-config-error-for ":export/bullet-indentation :two-spaces")
-         "wrapped in {}"))
-
-    (is (string/includes?
-         (validation-config-error-for "{:preferred-workflow :todo")
-         "Failed to read"))
-
-    (is (string/includes?
-         (validation-config-error-for "{:start-of-week 7}")
-         "has the following errors"))))