diff --git a/frontend/src/app/plugins/utils.cljs b/frontend/src/app/plugins/utils.cljs index 0bbf358e20..49622d9710 100644 --- a/frontend/src/app/plugins/utils.cljs +++ b/frontend/src/app/plugins/utils.cljs @@ -328,10 +328,217 @@ [[(str/join "." path) (:message v)]]))) m))) +(def ^:private max-repr-length 100) +(def ^:private max-repr-depth 2) +(def ^:private max-repr-items 5) + +(defn- abbreviate + "Shorten `s` to `max-repr-length` code points. Cutting on a UTF-16 code + unit would split surrogate pairs and render astral characters as mojibake." + [s] + (if (> (count s) max-repr-length) + (let [points (js/Array.from s)] + (if (> (alength points) max-repr-length) + (dm/str (.join (.slice points 0 max-repr-length) "") "…") + s)) + s)) + +(defn- value->type + [value] + (cond + (string? value) "string" + (boolean? value) "boolean" + (number? value) "number" + (keyword? value) "keyword" + (map? value) "object" + (coll? value) "array" + (array? value) "array" + (fn? value) "function" + (instance? js/Object value) "object" + :else "unknown")) + +(defn- data-property + "Own property `k` of the JS object `o`, or `::skip` when `k` is an accessor. + Plugin proxies expose their contents through getters that read the + application state and may throw, so the error path must not run them." + [o k] + (let [descriptor (js/Object.getOwnPropertyDescriptor o k)] + (if (and (some? descriptor) + (undefined? (unchecked-get descriptor "get"))) + (unchecked-get descriptor "value") + ::skip))) + +(defn- value->repr + "Bounded representation of a value received from a plugin. Such values are + arbitrary JS data: `pr-str` never returns on a self referencing object + (`a.parent.child === a`), so the traversal is capped in depth and in width + and never descends into an ancestor." + ([value] + (value->repr value 0 [])) + ([value depth seen] + (cond + (string? value) + (pr-str (abbreviate value)) + + (or (nil? value) (number? value) (boolean? value) (keyword? value)) + (pr-str value) + + (fn? value) + "#function" + + (some #(identical? % value) seen) + "#recursive" + + (>= depth max-repr-depth) + "…" + + (map? value) + (let [seen (conj seen value)] + (dm/str "{" (->> (take max-repr-items value) + (map (fn [[k v]] + (dm/str (value->repr k (inc depth) seen) " " + (value->repr v (inc depth) seen)))) + (str/join ", ")) + (when (> (count value) max-repr-items) ", …") "}")) + + (or (array? value) (coll? value)) + (let [seen (conj seen value) + items (if (array? value) (array-seq value) (seq value))] + (dm/str "[" (->> (take max-repr-items items) + (map #(value->repr % (inc depth) seen)) + (str/join " ")) + (when (seq (drop max-repr-items items)) " …") "]")) + + (instance? js/Object value) + (let [seen (conj seen value) + ks (js/Object.keys value)] + (dm/str "{" (->> (take max-repr-items ks) + (keep (fn [k] + (let [v (data-property value k)] + (when-not (= ::skip v) + (dm/str k " " (value->repr v (inc depth) seen)))))) + (str/join ", ")) + (when (> (alength ks) max-repr-items) ", …") "}")) + + :else + (value->type value)))) + +(defn- printable? + "True when a schema form contains only data, so it can be shown to a plugin + author. `[:fn pred]` forms embed the predicate itself, which prints as an + unreadable `#object[…]` with the internal munged name." + [form] + (cond + (map? form) (every? printable? (vals form)) + (coll? form) (every? printable? form) + (keyword? form) true + (string? form) true + (number? form) true + (boolean? form) true + (symbol? form) true + (regexp? form) true + (nil? form) true + :else false)) + +(defn- simplify-form + "Drop from a schema form the properties that are not data, so a schema is + described by its shape alone: `[::sm/text {:error/fn f}]` becomes + `::sm/text`. Yields `::unprintable` when what is left cannot describe the + schema, as in `[:fn pred]`, where dropping the predicate would advertise a + schema (`fn`) that means nothing to a plugin author." + [form] + (cond + (map? form) + (not-empty (into {} (filter (comp printable? val)) form)) + + (vector? form) + (let [items (into [] (keep simplify-form) form)] + (cond + (some #(= ::unprintable %) items) ::unprintable + (= 1 (count items)) (first items) + :else items)) + + (printable? form) + form + + :else + ::unprintable)) + +(defn- expected-form + "Readable representation of the schema a problem failed against, or nil when + the schema cannot be shown." + [schema] + (let [title (or (:title (sm/properties schema)) + (:title (sm/type-properties schema)))] + (if (some? title) + title + (let [form (simplify-form (sm/form schema))] + (cond + (= ::unprintable form) nil + (keyword? form) (name form) + :else (pr-str form)))))) + +(defn- schema-message + "Message rendered by `csm/interpret-schema-problem` for a problem, or nil when + it degrades to the generic \"invalid data\": the token value schemas declare an + `:error/fn` that only speaks about empty values, so it renders nothing at all + for a wrong typed one, and saying nothing must not win over reporting what was + expected and what was received (#10072)." + [problem field] + (let [message (-> (csm/interpret-schema-problem {} problem) + (get-in field) + (get :message))] + (when (and (some? message) + (not= message (tr "errors.invalid-data"))) + message))) + +(defn- interpret-problem + "Like `csm/interpret-schema-problem`, but when the schema renders no message of + its own it reports the expected schemas together with the received value and + its type instead of a generic \"Invalid data\" (#10072). + + Malli reports one problem per `:or` branch, all on the same field, so the + branches are accumulated: reporting only one of them would tell the plugin + author that an alternative that is in fact valid is not accepted." + [acc {:keys [schema in value] :as problem}] + (let [field (or (:error/field (sm/properties schema)) in) + field (if (vector? field) field [field]) + current (when (seq field) (get-in acc field))] + (cond + (empty? field) + acc + + ;; A message the schema renders itself always wins over the generic + ;; expected/received one, and is never overwritten by it. + (and (some? current) (not (contains? current :expected))) + acc + + :else + (if-let [message (schema-message problem field)] + (assoc-in acc field {:message message}) + (let [form (expected-form schema) + complete? (and (get current :complete? true) (some? form)) + expected (if complete? + (-> (get current :expected []) + (conj form) + (distinct) + (vec)) + []) + received (value->type value) + repr (abbreviate (value->repr value)) + message (if (seq expected) + (tr "plugins.validation.invalid-value" + (abbreviate (str/join " or " expected)) + received repr) + (tr "plugins.validation.received-value" received repr))] + (assoc-in acc field {:message message + :expected expected + :complete? complete?})))))) + (defn error-messages [explain] (let [msg (->> (:errors explain) - (reduce csm/interpret-schema-problem {}) + (reduce interpret-problem {}) (flatten-error-map) (map (fn [[field message]] (tr "plugins.validation.message" field message))) diff --git a/frontend/test/frontend_tests/plugins/utils_test.cljs b/frontend/test/frontend_tests/plugins/utils_test.cljs index a2a1e98124..3c731055ea 100644 --- a/frontend/test/frontend_tests/plugins/utils_test.cljs +++ b/frontend/test/frontend_tests/plugins/utils_test.cljs @@ -6,8 +6,23 @@ (ns frontend-tests.plugins.utils-test (:require + [app.common.files.tokens :as cfo] + [app.common.schema :as sm] [app.plugins.utils :as plugins.utils] - [cljs.test :as t :include-macros true])) + [app.util.i18n :as i18n] + [cljs.test :as t :include-macros true] + [cuerdas.core :as str])) + +;; The plugin error renderer goes through `tr`, which returns the bare +;; translation code when no locale data is loaded. Load the strings this +;; namespace asserts on so the assertions exercise the real rendering. +(i18n/set-default-translations + #js {"plugins.validation.message" "Field %s is invalid: %s" + "plugins.validation.invalid-value" "expected %s, got %s (%s)" + "plugins.validation.received-value" "got %s (%s)" + "errors.invalid-data" "Invalid data" + "errors.field-missing" "Missing field" + "errors.tokens.empty-input" "Empty input"}) ;; Access the private flattener for direct testing. (def ^:private flatten-error-map @#'plugins.utils/flatten-error-map) @@ -87,3 +102,190 @@ ;; `error-messages` returns nil (not "") on an explain with no mappable ;; errors, so `handle-error` can distinguish "no message" from a real one. (t/is (nil? (plugins.utils/error-messages {:errors []})))) + +;; --------------------------------------------------------------------- +;; Issue #10072 — schema validation errors must say what was expected and +;; what was received, instead of collapsing into a generic "Invalid data". + +(t/deftest test-error-messages-reports-expected-and-received + (let [explain (sm/explain [:map [:value :string]] {:value 16}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field value is invalid: expected string, got number (16)" message)))) + +(t/deftest test-error-messages-reports-nested-field-path + (let [explain (sm/explain [:map [:sets [:vector [:map [:name :string]]]]] + {:sets [{:name true}]}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field sets.0.name is invalid: expected string, got boolean (true)" message)))) + +(t/deftest test-error-messages-honors-custom-schema-message + ;; A schema that declares its own `:error/message` must keep it; the + ;; expected/received rendering is only a fallback. + (let [explain (sm/explain [:map [:value [:string {:error/message "must not be empty"}]]] + {:value 16}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field value is invalid: must not be empty" message)))) + +(t/deftest test-error-messages-missing-key + ;; Missing keys keep the "field missing" message: there is no received + ;; value to report. + (let [explain (sm/explain [:map [:value :string]] {}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field value is invalid: Missing field" message)))) + +(t/deftest test-error-messages-never-generic + (let [explain (sm/explain [:map [:value :string]] {:value 16}) + message (plugins.utils/error-messages explain)] + (t/is (not (str/includes? message "Invalid data"))))) + +(t/deftest test-error-messages-truncates-long-values + (let [value (apply str (repeat 500 "a")) + explain (sm/explain [:map [:value :int]] {:value value}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "got string")) + (t/is (< (count message) 200)))) + +(t/deftest test-error-messages-unicode-value + (let [explain (sm/explain [:map [:value :int]] {:value "ñ😀"}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "ñ😀")))) + +(t/deftest test-error-messages-collection-value + (let [explain (sm/explain [:map [:value :string]] {:value [1 2]}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "got array")))) + +(defn- proxy? [value] (and (map? value) (contains? value :proxy))) + +(defn- well-formed? + "False when the string contains a lone UTF-16 surrogate." + [s] + (try + (js/encodeURIComponent s) + true + (catch :default _ false))) + +(t/deftest test-error-messages-union-reports-every-branch + ;; Malli emits one problem per `:or` branch, all sharing the same path. + ;; Reporting a single branch as if it were the only requirement is worse + ;; than saying nothing: `plugins.tokens` declares the token value setter + ;; as `[:or :string base]`, so a plugin author would be told a vector is + ;; required when a plain string is equally valid. + (let [explain (sm/explain [:map [:value [:or [:vector :string] :string]]] {:value 16}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field value is invalid: expected [:vector :string] or string, got number (16)" + message)))) + +(t/deftest test-error-messages-union-reports-every-branch-nested + (let [explain (sm/explain [:map [:sets [:vector [:or :string :int]]]] {:sets [true]}) + message (plugins.utils/error-messages explain)] + (t/is (= "Field sets.0 is invalid: expected string or int, got boolean (true)" + message)))) + +(t/deftest test-error-messages-never-leaks-function-objects + ;; `[:fn pred]` schemas (used by the token-set arguments) render their form + ;; as `#object[app$plugins$tokens$token_set_proxy_QMARK_]`. That must never + ;; reach a plugin author. + (let [explain (sm/explain [:map [:value [:or [:fn proxy?] :string]]] {:value 16}) + message (plugins.utils/error-messages explain)] + (t/is (not (str/includes? message "#object"))) + (t/is (= "Field value is invalid: got number (16)" message)))) + +(t/deftest test-error-messages-function-schema-falls-back-to-custom-message + ;; When one branch of the union carries its own message, that message wins + ;; over the un-renderable `[:fn …]` branch. + (let [explain (sm/explain [:map [:value [:or [:fn proxy?] ::sm/uuid]]] {:value "nope"}) + message (plugins.utils/error-messages explain)] + (t/is (not (str/includes? message "#object"))) + (t/is (= "Field value is invalid: should be an uuid" message)))) + +(t/deftest test-error-messages-truncation-keeps-surrogate-pairs + ;; Truncating at a UTF-16 code-unit boundary splits astral characters and + ;; produces mojibake in a message whose whole purpose is legibility. + (let [value (apply str (repeat 200 "😀")) + explain (sm/explain [:map [:value :int]] {:value value}) + message (plugins.utils/error-messages explain)] + (t/is (well-formed? message)) + (t/is (str/includes? message "…")))) + +(t/deftest test-error-messages-truncation-counts-code-points + ;; 60 emoji are 120 UTF-16 code units but only 60 code points: below the + ;; limit, so the value must be reported whole. + (let [value (apply str (repeat 60 "😀")) + explain (sm/explain [:map [:value :int]] {:value value}) + message (plugins.utils/error-messages explain)] + (t/is (well-formed? message)) + (t/is (str/includes? message value)))) + +;; --------------------------------------------------------------------- +;; The value reported back to the plugin author is arbitrary JS data, so +;; rendering it must terminate and must not run plugin code. + +(t/deftest test-error-messages-self-referencing-object + ;; A plugin argument is a native JS value, and an object graph with a back + ;; reference (`a.parent.child === a`) is ordinary. `pr-str` walks it forever, + ;; so the error renderer would throw a RangeError from inside the error + ;; handler itself: the author gets a stack overflow instead of a validation + ;; error. + (let [a #js {} + b #js {} + _ (unchecked-set a "parent" b) + _ (unchecked-set b "child" a) + explain (sm/explain [:map [:value :string]] {:value a}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "got object")))) + +(t/deftest test-error-messages-object-with-getters + ;; Penpot's own proxies expose their contents through getters that read the + ;; application state and may throw for a stale id. Rendering a value must + ;; never invoke them. + (let [called (atom 0) + value (js/Object.defineProperty + #js {:id "x"} "children" + #js {:enumerable true + :get (fn [] (swap! called inc) (throw (js/Error. "boom")))}) + explain (sm/explain [:map [:value :string]] {:value value}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "got object")) + (t/is (zero? @called)))) + +(t/deftest test-error-messages-deeply-nested-object + ;; A deep graph must not produce an unbounded message. + (let [value (reduce (fn [acc _] #js {:child acc}) #js {:leaf 1} (range 200)) + explain (sm/explain [:map [:value :string]] {:value value}) + message (plugins.utils/error-messages explain)] + (t/is (str/includes? message "got object")) + (t/is (< (count message) 200)))) + +;; --------------------------------------------------------------------- +;; Issue #10072 — the reported example: `addToken` with a wrong token value. +;; +;; The token value schemas declare an `:error/fn` that only speaks about empty +;; values, so it returns nil for a wrong-typed one and `interpret-schema-problem` +;; degrades to the generic "Invalid data". A message that says nothing must not +;; win over one that says what was expected and what was received. + +(t/deftest test-error-messages-token-value-numeric + (let [schema [:map [:value (cfo/make-token-value-schema :font-size)]] + message (plugins.utils/error-messages (sm/explain schema {:value 16}))] + (t/is (= "Field value is invalid: expected string, got number (16)" message)))) + +(t/deftest test-error-messages-token-value-opacity + (let [schema [:map [:value (cfo/make-token-value-schema :opacity)]] + message (plugins.utils/error-messages (sm/explain schema {:value true}))] + (t/is (= "Field value is invalid: expected string, got boolean (true)" message)))) + +(t/deftest test-error-messages-token-value-font-family + ;; Both branches of the union are printable, so both are reported. + (let [schema [:map [:value (cfo/make-token-value-schema :font-family)]] + message (plugins.utils/error-messages (sm/explain schema {:value 16}))] + (t/is (= (str "Field value is invalid: " + "expected [:vector :app.common.schema/text] or TokenRef, got number (16)") + message)))) + +(t/deftest test-error-messages-token-value-empty-keeps-custom-message + ;; When the schema's own `:error/fn` does render a message, it still wins. + (let [schema [:map [:value (cfo/make-token-value-schema :font-size)]] + message (plugins.utils/error-messages (sm/explain schema {:value ""}))] + (t/is (= "Field value is invalid: Empty input" message)))) + diff --git a/frontend/translations/en.po b/frontend/translations/en.po index d54c01b33e..051d183632 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -5042,10 +5042,18 @@ msgstr "This token does not exist or has been deleted." msgid "options.deleted-token-with-name" msgstr "{%s} token does not exist or has been deleted." -#: src/app/plugins/utils.cljs:318 +#: src/app/plugins/utils.cljs:533 +msgid "plugins.validation.invalid-value" +msgstr "expected %s, got %s (%s)" + +#: src/app/plugins/utils.cljs:547 msgid "plugins.validation.message" msgstr "Field %s is invalid: %s" +#: src/app/plugins/utils.cljs:536 +msgid "plugins.validation.received-value" +msgstr "got %s (%s)" + #: src/app/main/ui/auth/recovery.cljs:88 msgid "profile.recovery.go-to-login" msgstr "Go to login"