Browse Source

fix: invalid keywords can be created by user

In https://github.com/logseq/db-test/issues/278, user has graph that
contains invalid edn keyword caused by name part of keyword starting
with a number e.g. :user.property/2ndsomething. Added thorough tests
based on reference doc for both nbb and cljs versions of buggy fn. For
cljs version, allowed a few more special characters in as they've
all been confirmed to be valid edn
Gabriel Horner 8 months ago
parent
commit
6c07d8838e

+ 9 - 2
deps/db/src/logseq/db/frontend/db_ident.cljc

@@ -71,10 +71,17 @@
          :cljs (and (exists? js/process) (or js/process.env.REPEATABLE_IDENTS js/process.env.DB_GRAPH))
          :default false)
     ;; Used for contexts where we want repeatable idents e.g. tests and CLIs
-    (keyword user-namespace (-> name-string (string/replace #"[/()]|\s+" "-") (string/replace-first #"^(\d)" "NUM-$1")))
+    (keyword user-namespace
+             (->> (string/replace-first name-string #"^(\d)" "NUM-$1")
+                  ;; '-' must go last in char class
+                  (filter #(re-find #"[0-9a-zA-Z*+!_'?<>=-]{1}" %))
+                  (apply str)))
     (keyword user-namespace
              (str
-              (->> (filter #(re-find #"[0-9a-zA-Z-]{1}" %) (seq name-string)) (apply str))
+              (->> (string/replace-first name-string #"^(\d)" "NUM-$1")
+                   ;; '-' must go last in char class
+                   (filter #(re-find #"[0-9a-zA-Z*+!_'?<>=-]{1}" %))
+                   (apply str))
               "-"
               (rand-nth non-int-char-range)
               (nano-id 7)))))

+ 44 - 0
deps/db/test/logseq/db/frontend/db_ident_test.cljc

@@ -0,0 +1,44 @@
+(ns logseq.db.frontend.db-ident-test
+  (:require [cljs.test :refer [deftest is testing]]
+            [clojure.edn :as edn]
+            [clojure.string :as string]
+            [logseq.db.frontend.db-ident :as db-ident]))
+
+(defn- valid-edn-keyword?
+  "Like common-util/valid-edn-keyword? but with kw as argument"
+  [kw]
+  (try
+    (boolean (edn/read-string (str "{" kw " nil}")))
+    (catch :default _
+      false)))
+
+;; These tests are copied to and should be kept in sync with logseq.db.misc-test
+;; Tests edge cases as described in https://clojure.org/reference/reader#_symbols
+(deftest create-db-ident-from-name
+  (testing "Symbols begin with a non-numeric character and can contain alphanumeric characters and *, +, !, -, _, ', ?, <, > and ="
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "f@!{h[#"))
+        "Kw created from name containing invalid special characters is valid edn")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo*+!_'?<>=-"))
+        "Kw created from name with all valid special characters is valid edn")
+    (is (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo*+!_'?<>=-"))
+                          "*+!_'?<>=-")
+        "Kw created from name with all valid special characters contains all special characters")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "2ndCity"))
+        "Kw created from name starting with number is valid edn")
+    (is (string/starts-with? (name (db-ident/create-db-ident-from-name "user.property" "2ndCity"))
+                             "NUM-2nd")
+        "Kw created from name starting with number does not start with that number"))
+
+  (testing "All other special characters"
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" ":name"))
+        "Kw created from name with ':' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" ":name")) ":"))
+        "Kw created from name with ':' doesn't contain ':'")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo/bar"))
+        "Kw created from name with '/' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo/bar")) "/"))
+        "Kw created from name with '/' doesn't contain '/'")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo.bar"))
+        "Kw created from name with '.' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo.bar")) "."))
+        "Kw created from name with '.' doesn't contain '.'")))

+ 1 - 1
deps/graph-parser/test/logseq/graph_parser/exporter_test.cljs

@@ -260,7 +260,7 @@
       (is (= {:user.property/prop-bool true
               :user.property/prop-num 5
               :user.property/prop-string "yeehaw"
-              :block/tags [:logseq.class/Page :user.class/Some---Namespace]}
+              :block/tags [:logseq.class/Page :user.class/SomeNamespace]}
              (db-test/readable-properties (db-test/find-page-by-title @conn "some page")))
           "Existing page has correct properties")
 

+ 45 - 0
src/test/logseq/db/misc_test.cljs

@@ -0,0 +1,45 @@
+(ns logseq.db.misc-test
+  "These are misc tests for logseq.db dep that must run in cljs"
+  (:require [cljs.test :refer [deftest is testing]]
+            [clojure.edn :as edn]
+            [clojure.string :as string]
+            [logseq.db.frontend.db-ident :as db-ident]))
+
+(defn- valid-edn-keyword?
+  "Like common-util/valid-edn-keyword? but with kw as argument"
+  [kw]
+  (try
+    (boolean (edn/read-string (str "{" kw " nil}")))
+    (catch :default _
+      false)))
+
+;; These tests are copied from and kept in sync with logseq.db.frontend.db-ident-test
+;; Tests edge cases as described in https://clojure.org/reference/reader#_symbols
+(deftest create-db-ident-from-name
+  (testing "Symbols begin with a non-numeric character and can contain alphanumeric characters and *, +, !, -, _, ', ?, <, > and ="
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "f@!{h[#"))
+        "Kw created from name containing invalid special characters is valid edn")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo*+!_'?<>=-"))
+        "Kw created from name with all valid special characters is valid edn")
+    (is (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo*+!_'?<>=-"))
+                          "*+!_'?<>=-")
+        "Kw created from name with all valid special characters contains all special characters")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "2ndCity"))
+        "Kw created from name starting with number is valid edn")
+    (is (string/starts-with? (name (db-ident/create-db-ident-from-name "user.property" "2ndCity"))
+                             "NUM-2nd")
+        "Kw created from name starting with number does not start with that number"))
+
+  (testing "All other special characters"
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" ":name"))
+        "Kw created from name with ':' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" ":name")) ":"))
+        "Kw created from name with ':' doesn't contain ':'")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo/bar"))
+        "Kw created from name with '/' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo/bar")) "/"))
+        "Kw created from name with '/' doesn't contain '/'")
+    (is (valid-edn-keyword? (db-ident/create-db-ident-from-name "user.property" "foo.bar"))
+        "Kw created from name with '.' is valid edn")
+    (is (not (string/includes? (name (db-ident/create-db-ident-from-name "user.property" "foo.bar")) "."))
+        "Kw created from name with '.' doesn't contain '.'")))