mirror of
https://github.com/penpot/penpot.git
synced 2026-05-14 12:34:10 +00:00
🐛 Fix plugin API error message for nested malli validation paths (#9486)
When a plugin call fails malli validation, the frontend renders one
"plugins.validation.message" line per error via
`app.plugins.utils/error-messages`, which reduces the explain via
`csm/interpret-schema-problem` and then destructures each entry as
`[field {:keys [message]}]` for translation.
That works only when the underlying malli error path has a single
element. `interpret-schema-problem` calls `(assoc-in acc field ...)`
where `field` can be a multi-element vector (e.g. `[:sets 0 :name]`).
For single-element paths the resulting map is flat
(`{:group {:message "..."}}`); for multi-element paths it is nested
(`{:sets {0 {:name {:message "..."}}}}`). The destructure assumes the
flat shape, so for a nested error the consumer reads:
field -> :sets
message -> nil (the nested entry has no :message at the top level)
and the produced i18n line resolves to `Field sets is invalid: ` --
or, when several errors are merged together at the same outer key,
to the user-facing `Field message is invalid` that the bug report
calls out, because `:message` then becomes the field name of the
deepest nested entry.
The original consumer carried a `#_(mapcat (comp seq val))` FIXME
that hinted at the missing flattening but did not implement one,
because the data shape produced by `interpret-schema-problem` is
not uniform.
Fix
---
Add a private `flatten-error-map` helper inside `app.plugins.utils`
that walks the error map produced by `interpret-schema-problem` and
yields `[path message]` pairs where `path` is the dot-joined field
path. Keywords use `(name k)`, strings pass through, anything else
(such as numeric indices from vector positions in the malli path)
is coerced via `str`. The recursion descends until it hits a leaf
that carries `:message`, which matches what
`interpret-schema-problem` produces in every branch.
The producer side (`csm/interpret-schema-problem` in
`common/src/app/common/schema/messages.cljc`) is left alone: it
already has another consumer (`collect-schema-errors` + the
form-validators pipeline) that depends on the keyed-by-field-path
shape, so normalising it at the source would require auditing every
validator. Flattening at the plugin consumer is the narrowest fix.
The FIXME comment is removed because the new helper supersedes it.
Tests
-----
`frontend-tests.plugins.utils-test` (new file, registered in
`runner.cljs`) covers:
- flat single-segment paths (`{:group {:message "..."}}`)
- nested multi-segment paths
(`{:sets {0 {:name {:message "..."}}}}`) -- the case from #9417
- mixed single- and multi-segment paths at the same explain
- mixed key types (keyword / string / numeric index)
- empty explain (no validation errors)
Closes #9417
Signed-off-by: bitcompass <devwiz.sh@gmail.com>
Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
parent
74ca40abd4
commit
fbb1f9e634
@ -276,13 +276,39 @@
|
||||
(let [s (set values)]
|
||||
(if (= (count s) 1) (first s) "mixed")))
|
||||
|
||||
(defn- flatten-error-map
|
||||
"Walk an error map produced by `csm/interpret-schema-problem` and yield
|
||||
`[path message]` pairs, where `path` is the dot-joined field path
|
||||
(e.g. `:group` -> \"group\", `[:sets 0 :name]` -> \"sets.0.name\").
|
||||
|
||||
`interpret-schema-problem` calls `(assoc-in acc field {:message …})`, so
|
||||
when the malli error path has more than one element the resulting map is
|
||||
nested (e.g. `{:sets {0 {:name {:message \"…\"}}}}`); when the path has
|
||||
a single element it is flat (`{:group {:message \"…\"}}`). The plugin
|
||||
error-message renderer needs both cases reduced to per-leaf
|
||||
`[path message]` pairs so it can produce one `plugins.validation.message`
|
||||
string per actual validation problem."
|
||||
([m] (flatten-error-map [] m))
|
||||
([prefix m]
|
||||
(mapcat
|
||||
(fn [[k v]]
|
||||
(let [segment (cond
|
||||
(keyword? k) (name k)
|
||||
(string? k) k
|
||||
:else (str k))
|
||||
path (conj prefix segment)]
|
||||
(if (and (map? v) (not (contains? v :message)))
|
||||
(flatten-error-map path v)
|
||||
[[(str/join "." path) (:message v)]])))
|
||||
m)))
|
||||
|
||||
(defn error-messages
|
||||
[explain]
|
||||
(->> (:errors explain)
|
||||
(reduce csm/interpret-schema-problem {})
|
||||
#_(mapcat (comp seq val)) ;; FIXME: why is this for? it breaks the message
|
||||
(map (fn [[field {:keys [message]}]]
|
||||
(tr "plugins.validation.message" (name field) message)))
|
||||
(flatten-error-map)
|
||||
(map (fn [[field message]]
|
||||
(tr "plugins.validation.message" field message)))
|
||||
(str/join ". ")))
|
||||
|
||||
(defn handle-error
|
||||
|
||||
56
frontend/test/frontend_tests/plugins/utils_test.cljs
Normal file
56
frontend/test/frontend_tests/plugins/utils_test.cljs
Normal file
@ -0,0 +1,56 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC
|
||||
|
||||
(ns frontend-tests.plugins.utils-test
|
||||
(:require
|
||||
[app.plugins.utils :as plugins.utils]
|
||||
[cljs.test :as t :include-macros true]))
|
||||
|
||||
;; Access the private flattener for direct testing.
|
||||
(def ^:private flatten-error-map @#'plugins.utils/flatten-error-map)
|
||||
|
||||
(t/deftest test-flatten-error-map-flat-input
|
||||
;; Regression test for issue #9417.
|
||||
;;
|
||||
;; When a malli error path has a single element, `interpret-schema-problem`
|
||||
;; produces a flat map. The flattener must pass that through unchanged.
|
||||
(let [result (flatten-error-map {:group {:message "must be string"}})]
|
||||
(t/is (= [["group" "must be string"]] result))))
|
||||
|
||||
(t/deftest test-flatten-error-map-nested-input
|
||||
;; Regression test for issue #9417.
|
||||
;;
|
||||
;; When a malli error path has multiple elements, `interpret-schema-problem`
|
||||
;; produces a nested map via `(assoc-in acc path …)`. The old plugin
|
||||
;; consumer destructured assuming a flat shape, so the nested case rendered
|
||||
;; the message text as the field name (`Field message is invalid`) instead
|
||||
;; of the real validation reason. The flattener must descend until it finds
|
||||
;; a leaf carrying a `:message`.
|
||||
(let [explain {:sets {0 {:name {:message "must not be empty"}}}}
|
||||
result (set (flatten-error-map explain))]
|
||||
(t/is (= #{["sets.0.name" "must not be empty"]} result))))
|
||||
|
||||
(t/deftest test-flatten-error-map-multiple-fields
|
||||
;; Multiple validation problems on the same explain produce multiple
|
||||
;; entries, none of which clobber each other.
|
||||
(let [explain {:group {:message "must be string"}
|
||||
:sets {0 {:message "set not found"}}}
|
||||
result (set (flatten-error-map explain))]
|
||||
(t/is (= #{["group" "must be string"]
|
||||
["sets.0" "set not found"]} result))))
|
||||
|
||||
(t/deftest test-flatten-error-map-mixed-key-types
|
||||
;; Numeric indices (from vector positions in the malli path) must render
|
||||
;; cleanly; string keys must also be accepted alongside keywords.
|
||||
(let [explain {:items {2 {"label" {:message "invalid label"}}}}
|
||||
[[path message]] (flatten-error-map explain)]
|
||||
(t/is (= "items.2.label" path))
|
||||
(t/is (= "invalid label" message))))
|
||||
|
||||
(t/deftest test-flatten-error-map-empty
|
||||
;; No validation errors -> no output (callers join with ". " and would
|
||||
;; otherwise emit an empty string, which is fine).
|
||||
(t/is (empty? (flatten-error-map {}))))
|
||||
@ -24,6 +24,7 @@
|
||||
[frontend-tests.plugins.context-shapes-test]
|
||||
[frontend-tests.plugins.parser-test]
|
||||
[frontend-tests.plugins.tokens-test]
|
||||
[frontend-tests.plugins.utils-test]
|
||||
[frontend-tests.svg-fills-test]
|
||||
[frontend-tests.tokens.import-export-test]
|
||||
[frontend-tests.tokens.logic.token-actions-test]
|
||||
@ -72,6 +73,7 @@
|
||||
'frontend-tests.plugins.context-shapes-test
|
||||
'frontend-tests.plugins.parser-test
|
||||
'frontend-tests.plugins.tokens-test
|
||||
'frontend-tests.plugins.utils-test
|
||||
'frontend-tests.svg-fills-test
|
||||
'frontend-tests.tokens.import-export-test
|
||||
'frontend-tests.tokens.logic.token-actions-test
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user