github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-08-10 23:08:41 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Declare new shape attributes in schemas to match stored files (#11125)

Browse Source 
* 🐛 Declare the shape attributes stored files carry

`schema:shape-attrs` is the shape model as *declared*, and it has fallen
behind the `Shape` record. Three record fields are absent from it:
`rotation`, `flip-x` and `flip-y` are therefore present on every shape
that exists and declared nowhere. `rotation` is already named twice in
this namespace, in `allowed-shape-attrs`, and once in
`app.common.types.shape.attrs/editable-attrs`, so the schema is
demonstrably the odd one out rather than the data being unusual.

Nothing complains, because the maps are open: an undeclared key
validates fine. What breaks is everything that reads the model *from the
schema* rather than from a live value, such as the generative tests'
shape generator, the generated OpenAPI surface, and any consumer
reflecting over `schema:shape-attrs`.

Whether an entry is optional, nilable, or both is decided by the record
rather than by taste. `app.common.record/defrecord` cannot remove a base
field: its `without` assocs nil and its `containsKey` answers true
whatever the field holds, on both platforms. So a `Shape` base field is
always present, and nil is how that field says "unset". Every other key
lives in the `$extmap`, disappears on dissoc, and is dropped by
`setup-shape` when a caller passes nil. Base fields are therefore
nilable, and the rest are optional.

Declared here, measured over a 305-shape corpus:

- `rotation`, `flip-x` and `flip-y`, record fields present on every
  shape, nilable for the reason above: `make-minimal-shape` gives the
  two flip fields no default, so they are nil on all 305. Optional as
  well, unlike the geometry below, because `schema:shape-generic-attrs`
  has a second job: `check-shape-generic-attrs` validates partial update
  payloads with it, such as the `{:blocked true}` that
  `app.main.data.workspace/update-shape` passes, and a required key here
  would reject every such payload.
- `hide-in-viewer`, moved out of `schema:frame-attrs`, because circles,
  rects and texts carry it too, 197 shapes.
- `svg-attrs`, `svg-defs`, `svg-transform` and `svg-viewbox`, the SVG
  provenance an import leaves behind, 101 shapes and 63 for the
  transform. Typed `:map` rather than more precisely on purpose: legacy
  files hold `svg-transform` as a plain `{:a … :f}` map rather than a
  `::gmt/matrix` record, and `svg-viewbox` as either a `::grc/rect`
  record or a plain map, so a tighter schema would reject files that are
  otherwise valid.
- `use-for-thumbnail` on frames. The model has long had it:
  `app.common.files.migrations` renames `:use-for-thumbnail?` to it and
  `app.common.logic.libraries` reads it. This schema had not declared
  it.
- `rx` and `ry` on rects and circles, the legacy radii SVG import parses
  off the element and migration 0003 assocs as `0`. Superseded by `r1`
  to `r4`, but stored files carry them.
- `content` on svg-raw. `shapes-builder/create-raw-svg` sets it and
  `allowed-svg-attrs` names it. Typed `[:or :map :string]`, because a
  bare text node arrives as the string itself: `<text>hi</text>` becomes
  one svg-raw for the element and another for `"hi"`, and
  `shapes-builder/parse-svg-element` carries a FIXME about exactly that.

`schema:nilable-geom-attrs` is new, for bool and path. Those two are the
only shape types whose geometry can be nil: `make-minimal-shape` gives
`x`, `y`, `width` and `height` a default for every other type and skips
those two, whose extent their content and `selrect` imply instead. The
four keys stay required, as they already are in the other seven
branches, and only the nil is new.

**Do not make the analogous change to `ctf/schema:file`.** That map
carries `:backend`, `:comment-thread-seqn` and `:ignore-sync-until`,
none of which the schema declares, and declaring them breaks saving:
`app.binfile.common/update-file!` derives its UPDATE column list from a
file map's keys, and the `file` table has no `backend` column, it being
synthesized on read. Measured at 185 failures, mostly `rpc-file-test`.
Whether a schema serving as both read description and write contract is
itself a defect is a real design question, and a separate one. The
`check-shape-generic-attrs` case above is a second instance of it.

Adding entries changes what `shape-generator` produces, so generative
tests begin exercising code paths with these attributes present. That is
where a problem would surface. With this applied the common suite is
1142 tests and 24702 assertions on the Clojure side, 992 tests and 24017
assertions on the ClojureScript side, no failures on either.

AI-assisted-by: mixed models

* ✨ Align shape generator with declared schema and add key-presence test

shape-generator now selects geometry attrs per-type: nilable-geom-attrs
for bool/path, shape-geom-attrs for everything else, and always merges
them. This removes the dead attrs2 generation for bool/path and the
implicit dependency on create-shape adding nil defaults for missing
base record fields.

The new shape-generator-key-presence test asserts that generated shapes
carry the required keys: rotation, flip-x, flip-y on all shapes and x,
y, width, height on bool/path, even when nilable.

AI-assisted-by: longcat-2.0-free

* 🐛 Sample 200 shapes in the key-presence test, not 10

`sg/sample` hands its options to `malli.generator/sample`, which reads
`:size`. `:num` is test.check's option. It is correct for the
`smt/check!` call directly above, where it came from, but `sg/sample`
ignores it and falls back to its default of 10.

Ten samples leave the bool and path assertions vacuous about one run in
fourteen. Simulated over 200 draws of 10, 14 contained no bool and no
path at all, and the median draw held 2. Those four assertions defend
exactly the keys this branch made required, so a run that skips them
silently is the one case worth not missing.

The assertion count shows the arithmetic. The test contributed 42 with
`:num`, which is 10 shapes times 3 keys plus 3 bool-or-path shapes times
4 keys, and contributes 756 with `:size`. The common suite goes from
1143 tests and 24744 assertions to 1143 tests and 25458 assertions, no
failures either way.

AI-assisted-by: mixed models

---------

Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Álvaro Tejero-Cantero 2026-08-07 14:20:24 +02:00 committed by GitHub
parent 30bc2a4bc3
commit b5bec4f983
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 116 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

105
common/src/app/common/types/shape.cljc
View File

@ -233,7 +233,50 @@
[:grow-type {:optional true}
[::sm/one-of grow-types]]
[:applied-tokens {:optional true} cto/schema:applied-tokens]
[:plugin-data {:optional true} ctpg/schema:plugin-data]])
[:plugin-data {:optional true} ctpg/schema:plugin-data]
;; `rotation`, `flip-x` and `flip-y` are fields of the `Shape` record (see
;; `cr/defrecord Shape` above) and this schema did not declare them.
;; `rotation` was already named in `allowed-shape-attrs` here and in
;; `app.common.types.shape.attrs/editable-attrs`, so the omission was in this
;; schema and not in the model. Anything reading the model from the schema
;; rather than from a live shape missed all three: the graph projection
;; derives one column per entry (`app.graph.schema.projection`), so shape
;; nodes carried no rotation at all, and a consumer cannot place a shape
;; without it.
;;
;; Nilable, because `app.common.record/defrecord` cannot remove a base
;; field: its `without` assocs nil and its `containsKey` answers true
;; whatever the field holds, so nil is how a record field says "unset".
;; `flip-x` and `flip-y` are nil on every shape `setup-shape` builds, since
;; `make-minimal-shape` gives them no default.
;;
;; Optional as well, unlike the geometry group below, because this schema
;; has a second job: `check-shape-generic-attrs` validates partial update
;; payloads with it, such as the `{:blocked true}` that
;; `app.main.data.workspace/update-shape` passes. A required key here would
;; reject every such payload.
[:rotation {:optional true} [:maybe ::sm/safe-number]]
[:flip-x {:optional true} [:maybe :boolean]]
[:flip-y {:optional true} [:maybe :boolean]]
;; Carried on circles, rects and texts too, not only on frames, so it
;; belongs here rather than in `schema:frame-attrs`. Not nilable: the key
;; lives outside the record, `app.common.logic.shapes` dissocs it to unset
;; it, and `setup-shape` drops it when a caller passes nil.
[:hide-in-viewer {:optional true} :boolean]
;; The SVG provenance an import leaves on a shape. Typed `:map` rather than
;; more precisely on purpose: legacy files hold `svg-transform` as a plain
;; `{:a … :f}` map rather than a `::gmt/matrix` record, and `svg-viewbox` as
;; either a `::grc/rect` record or a plain map, so a tighter schema here
;; would reject files that are otherwise valid. The graph *column* types are
;; tightened separately, where a wrong guess costs a column rather than a
;; rejected file (`app.graph.schema.contract/type-overrides`).
[:svg-attrs {:optional true} :map]
[:svg-defs {:optional true} :map]
[:svg-transform {:optional true} :map]
[:svg-viewbox {:optional true} :map]])
(def schema:group-attrs
[:map {:title "GroupAttrs"}
@ -244,7 +287,30 @@
[:shapes [:vector {:gen/max 10 :gen/min 1} ::sm/uuid]]
[:hide-fill-on-export {:optional true} :boolean]
[:show-content {:optional true} :boolean]
[:hide-in-viewer {:optional true} :boolean]])
;; `hide-in-viewer` moved to `schema:shape-generic-attrs`: stored files carry
;; it on circles, rects and texts too, not only on frames.
;; `use-for-thumbnail` is a frame attribute the model has long had, since
;; `app.common.files.migrations` renames `:use-for-thumbnail?` to it and
;; `app.common.logic.libraries` reads it, and this schema had not declared.
[:use-for-thumbnail {:optional true} :boolean]])
(def ^:private schema:nilable-geom-attrs
"`schema:shape-geom-attrs`, but nilable.
Bools and paths are the only two shape types whose geometry can be nil:
`make-minimal-shape` gives `x`, `y`, `width` and `height` a default for every
other type and skips those two, whose extent their content and `selrect`
imply instead. The four keys stay required, because they are `Shape` record
fields and `app.common.record/defrecord` keeps a base field present whatever
it holds. So these two branches cannot merge `schema:shape-geom-attrs`, which
rejects the nil, and declare the same four keys nilable instead. A
schema-derived reader previously saw a bool or a path as having no position or
size at all."
[:map {:title "NilableGeometryAttrs"}
[:x [:maybe ::sm/safe-number]]
[:y [:maybe ::sm/safe-number]]
[:width [:maybe ::sm/safe-number]]
[:height [:maybe ::sm/safe-number]]])
(def ^:private schema:bool-attrs
[:map {:title "BoolAttrs"}
@ -253,10 +319,19 @@
[:content path/schema:content]])
(def ^:private schema:rect-attrs
[:map {:title "RectAttrs"}])
[:map {:title "RectAttrs"}
;; Legacy radii, set by SVG import (`app.common.files.shapes-builder` parses
;; `rx`/`ry` off the element) and by migration 0003, which assocs `0`.
;; Superseded by `r1` to `r4`, but stored files still carry them. Not
;; nilable: both keys live outside the `Shape` record, so a dissoc removes
;; them, and `setup-shape` drops a nil before the merge.
[:rx {:optional true} ::sm/safe-number]
[:ry {:optional true} ::sm/safe-number]])
(def ^:private schema:circle-attrs
[:map {:title "CircleAttrs"}])
[:map {:title "CircleAttrs"}
[:rx {:optional true} ::sm/safe-number]
[:ry {:optional true} ::sm/safe-number]])
(def ^:private schema:svg-raw-attrs
[:map {:title "SvgRawAttrs"}
@ -266,7 +341,15 @@
;; keeps the child ids typed as uuid, so a JSON round trip (binfile
;; export/import) decodes them back to uuids instead of leaving
;; strings that no longer resolve against the objects map.
[:shapes {:optional true} [:vector {:gen/max 10} ::sm/uuid]]])
[:shapes {:optional true} [:vector {:gen/max 10} ::sm/uuid]]
;; The raw SVG node an import kept.
;; `app.common.files.shapes-builder/create-raw-svg` sets it and
;; `allowed-svg-attrs` names it. Usually the parsed element,
;; `{:tag … :attrs … :content …}`, but a bare text node arrives as the
;; string itself: `<text>hi</text>` becomes one svg-raw for the element
;; and another for `"hi"`. `app.common.files.shapes-builder/parse-svg-element`
;; carries a FIXME about exactly that. Both forms are legal and stored.
[:content {:optional true} [:or :map :string]]])
(def schema:image-attrs
[:map {:title "ImageAttrs"}
@ -301,7 +384,10 @@
(->> (sg/generator schema:shape-base-attrs)
(sg/mcat (fn [{:keys [type] :as shape}]
(sg/let [attrs1 (sg/generator schema:shape-generic-attrs)
attrs2 (sg/generator schema:shape-geom-attrs)
attrs2 (if (or (= type :path)
(= type :bool))
(sg/generator schema:nilable-geom-attrs)
(sg/generator schema:shape-geom-attrs))
attrs3 (case type
:text (sg/generator schema:text-attrs)
:path (sg/generator schema:path-attrs)
@ -312,10 +398,7 @@
:bool (sg/generator schema:bool-attrs)
:group (sg/generator schema:group-attrs)
:frame (sg/generator schema:frame-attrs))]
(if (or (= type :path)
(= type :bool))
(merge attrs1 shape attrs3)
(merge attrs1 shape attrs2 attrs3)))))
(merge attrs1 shape attrs2 attrs3))))
(sg/fmap create-shape)))
(def schema:shape-attrs
@ -347,6 +430,7 @@
ctsl/schema:layout-child-attrs
schema:bool-attrs
schema:shape-generic-attrs
schema:nilable-geom-attrs
schema:shape-base-attrs]]
[:rect
@ -386,6 +470,7 @@
ctsl/schema:layout-child-attrs
schema:path-attrs
schema:shape-generic-attrs
schema:nilable-geom-attrs
schema:shape-base-attrs]]
[:text

21
common/test/common_tests/types/shape_decode_encode_test.cljc
View File

@ -146,3 +146,24 @@
;; (app.common.pprint/pprint shape-3)
(= shape shape-3)))
{:num 200})))
(t/deftest shape-generator-key-presence
"The generator must produce the keys the schema declares required, even when
nilable. This is a targeted check for the attributes added to
`schema:shape-generic-attrs` and `schema:nilable-geom-attrs`."
(let [shapes (sg/sample (sg/generator schema:shape) {:size 200})
by-type (group-by :type shapes)]
;; All shapes: rotation, flip-x, flip-y are base record fields, always
;; present (possibly nil).
(doseq [shape shapes]
(t/is (contains? shape :rotation) "missing :rotation")
(t/is (contains? shape :flip-x) "missing :flip-x")
(t/is (contains? shape :flip-y) "missing :flip-y"))
;; Bool and path: x/y/width/height are required-but-nilable in the
;; schema. The generator must produce them (nil is a valid value).
(doseq [shape (concat (get by-type :bool [])
(get by-type :path []))]
(t/is (contains? shape :x) "bool/path missing :x")
(t/is (contains? shape :y) "bool/path missing :y")
(t/is (contains? shape :width) "bool/path missing :width")
(t/is (contains? shape :height) "bool/path missing :height"))))