mirror of
https://github.com/penpot/penpot.git
synced 2026-08-08 22:08:39 +00:00
b5bec4f983
* 🐛 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>