mirror of
https://github.com/penpot/penpot.git
synced 2026-08-08 22:08:39 +00:00
🐛 Write graph values Ladybug's CSV reader cannot carry through Cypher
Three parity failures against beadpot's suite, all one cause: the bulk loader put compound and multi-line values into CSV, where Ladybug parses a field's *contents* as a literal with no escape mechanism at all. Verified against 0.18: a comma inside a list element ends the element, quotes are kept as part of the value rather than delimiting it, and the parallel reader rejects quoted newlines outright. So a value now goes through CSV only if it cannot be misread there — UUIDs, numbers, booleans, single-line strings, and lists of those. Everything else (MAP, STRUCT, STRING[]/JSON[], any string containing a newline) is written after the COPY by one Cypher statement per row, where `app.graph.ladybug` escapes properly. Parquet removes the distinction entirely and is still the right destination (masterplan P0 T1); this is what CSV can honestly do. Consequences beyond the encoding: - `touched` entries reached the graph as `:swap-slot-…`, keywords stringified with their colon, so `LinkSwapSlots` matched nothing. Keywords now render through `name`. - Shape names lost their newlines to a flattening step that existed only to keep the CSV writer happy. They are preserved. - `applied_tokens` keys are rendered camelCase, the form Penpot's own JSON encoder produces and the one beadpot's `AppliedTokenKey` holds — a MAP column's keys are values, not schema, so they are not snake_cased. - `link-component-instances!` keys on `component-file`, not `component-id` alone. The projection denormalizes `component-id` down the shape tree, after which it no longer tells an instance head from a shape inside one, and the transform linked every descendant frame; `ctk/instance-of?` requires both keys anyway. IsInstanceOf on the variants fixture: 78 -> 60, matching beadpot exactly. `app.graph.schema.nodes/format-column-value` is now the single place that knows a column's type and its contract details, used by the bulk loader and the incremental sync alike so the two cannot disagree about a value's shape.
This commit is contained in:
parent
6b2a6de411
commit
c3db857936
@ -26,17 +26,20 @@
|
||||
string fields are treated as column separators."
|
||||
"HEADER=true, DELIM=',', QUOTE='\"'")
|
||||
|
||||
(defn- csv-normalize-string
|
||||
"Graph node names are single-line labels; flatten Penpot text newlines."
|
||||
[s]
|
||||
(-> (str s)
|
||||
(str/replace #"\r\n" " ")
|
||||
(str/replace #"\r" " ")
|
||||
(str/replace #"\n" " ")))
|
||||
|
||||
(defn- csv-escape-string
|
||||
[s]
|
||||
(str "\"" (str/replace (csv-normalize-string s) "\"" "\"\"") "\""))
|
||||
(str "\"" (str/replace (str s) "\"" "\"\"") "\""))
|
||||
|
||||
(defn- multiline?
|
||||
"Does this value contain a newline?
|
||||
|
||||
Ladybug's parallel CSV reader rejects quoted newlines outright, and a shape
|
||||
name or text body may well contain one. Rather than flatten them — beadpot
|
||||
keeps them, and a graph is not a place to lose characters — such values are
|
||||
written through Cypher afterwards (`fixup-statements`)."
|
||||
[v]
|
||||
(and (string? v)
|
||||
(or (str/includes? v "\n") (str/includes? v "\r"))))
|
||||
|
||||
(defn- csv-cell
|
||||
[v]
|
||||
@ -61,7 +64,13 @@
|
||||
"NULL"
|
||||
|
||||
(contains? #{"STRING" "JSON"} elem-type)
|
||||
(let [s (if (coll? v) (json/encode v) (csv-normalize-string (str v)))]
|
||||
;; Unreachable in practice: string-bearing lists go through Cypher
|
||||
;; (`csv-representable?`), because Ladybug's CSV list-literal parser has no
|
||||
;; escaping at all. Kept so the function stays total.
|
||||
(let [s (cond
|
||||
(coll? v) (json/encode v)
|
||||
(keyword? v) (name v)
|
||||
:else (str v))]
|
||||
(str "'" (-> s
|
||||
(str/replace "\\" "\\\\")
|
||||
(str/replace "'" "\\'"))
|
||||
@ -80,13 +89,74 @@
|
||||
(csv-escape-string
|
||||
(str "[" (str/join "," (map #(kuzu-list-element elem-type %) elems)) "]"))))
|
||||
|
||||
(defn- csv-representable?
|
||||
"Can a value of `ladybug-type` survive a CSV round-trip?
|
||||
|
||||
Ladybug parses the *contents* of a CSV field as a Cypher-ish literal for
|
||||
compound types, and that parser has no escape mechanism whatsoever: a comma
|
||||
inside a string element ends the element, and quotes are kept as part of the
|
||||
value rather than delimiting it (verified against 0.18). So only compound
|
||||
types whose elements cannot contain a delimiter — UUID, numbers, booleans —
|
||||
are safe; anything carrying a string or JSON is not, and neither is a MAP or
|
||||
a STRUCT. Those go through Cypher instead (`fixup-statements`), where
|
||||
`app.graph.ladybug` escapes properly.
|
||||
|
||||
Parquet would remove the distinction entirely (masterplan P0 T1 chose it,
|
||||
with CSV as the fallback of last resort); until the JVM side grows a Parquet
|
||||
writer, this is where the line falls."
|
||||
[ladybug-type]
|
||||
(cond
|
||||
(not (string? ladybug-type)) true
|
||||
(ladybug/map-type? ladybug-type) false
|
||||
(str/starts-with? ladybug-type "STRUCT") false
|
||||
(str/ends-with? ladybug-type "[]")
|
||||
(not (contains? #{"STRING" "JSON"}
|
||||
(subs ladybug-type 0 (- (count ladybug-type) 2))))
|
||||
:else true))
|
||||
|
||||
(defn- defer-to-cypher?
|
||||
"Must this value be written after the COPY rather than in the CSV?
|
||||
|
||||
Two reasons, both limitations of Ladybug's CSV reader rather than choices:
|
||||
a type its literal parser cannot escape, or a string containing a newline."
|
||||
[ladybug-type v]
|
||||
(or (not (csv-representable? ladybug-type))
|
||||
(multiline? v)))
|
||||
|
||||
(defn- csv-typed-cell
|
||||
[ladybug-type v]
|
||||
(if (and (some? v)
|
||||
(string? ladybug-type)
|
||||
(str/ends-with? ladybug-type "[]"))
|
||||
(cond
|
||||
;; Written after the COPY, through Cypher — see `fixup-statements`.
|
||||
(defer-to-cypher? ladybug-type v) ""
|
||||
|
||||
(and (some? v)
|
||||
(string? ladybug-type)
|
||||
(str/ends-with? ladybug-type "[]"))
|
||||
(kuzu-list-cell ladybug-type v)
|
||||
(csv-cell v)))
|
||||
|
||||
:else (csv-cell v)))
|
||||
|
||||
(defn- fixup-statements
|
||||
"Cypher to set the values the CSV had to leave empty.
|
||||
|
||||
One statement per row that has any — not per column — so the cost is one
|
||||
round-trip per shape rather than per attribute, and a row with none costs
|
||||
nothing at all."
|
||||
[table rows]
|
||||
(let [columns (nodes/column-keys table)]
|
||||
(for [row rows
|
||||
:let [sets (for [k columns
|
||||
:let [v (get row k)
|
||||
t (nodes/column-ladybug-type table k)]
|
||||
:when (some? v)
|
||||
:when (defer-to-cypher? t v)
|
||||
:when (or (not (coll? v)) (seq v))]
|
||||
(str "n." (nodes/cypher-property-key table k) " = "
|
||||
(nodes/format-column-value table k v)))]
|
||||
:when (seq sets)]
|
||||
(str "MATCH (n:" (nodes/match-label table) " {id: "
|
||||
(ladybug/format-uuid (:id row)) "}) "
|
||||
"SET " (str/join ", " sets) ";"))))
|
||||
|
||||
(defn- cypher-file-path
|
||||
[^File file]
|
||||
@ -169,7 +239,9 @@
|
||||
:when (seq rows)]
|
||||
(let [csv-file (io/file staging-path (str table ".csv"))]
|
||||
(write-node-csv! csv-file table rows)
|
||||
(copy-node-table! conn table csv-file)))
|
||||
(copy-node-table! conn table csv-file)
|
||||
(when-let [stmts (seq (fixup-statements table rows))]
|
||||
(ladybug/exec-on-connection! conn stmts))))
|
||||
(doseq [[[from-table to-table] group]
|
||||
(sort-by identity (group-by (juxt :from-table :to-table) edges))
|
||||
:when (seq group)]
|
||||
|
||||
@ -105,7 +105,9 @@
|
||||
[elem-type v]
|
||||
(case elem-type
|
||||
"UUID" (format-uuid v)
|
||||
"STRING" (format-string (str v))
|
||||
;; `name` for keywords, so a `:touched` entry reads `swap-slot-…` and not
|
||||
;; `:swap-slot-…` — see `app.graph.bulk/kuzu-list-element`.
|
||||
"STRING" (format-string (if (keyword? v) (name v) (str v)))
|
||||
"JSON" (format-json v)
|
||||
"INT64" (format-int v)
|
||||
"DOUBLE" (format-number v)
|
||||
@ -126,12 +128,38 @@
|
||||
(str/join ", " (map #(format-list-element elem-type %) elems))
|
||||
"]")))
|
||||
|
||||
(defn format-map
|
||||
"Cypher literal for a `MAP(STRING, STRING)` column.
|
||||
|
||||
Ladybug's CSV reader parses map literals (`{k=v, …}`) with no escape
|
||||
mechanism: a comma inside a value ends the entry, and quotes are kept as part
|
||||
of the string. Nothing user-authored — a design-token name, say — survives
|
||||
that round-trip, so map columns are written through Cypher instead, where
|
||||
`map/2` takes two properly escaped lists (`app.graph.bulk`).
|
||||
|
||||
`key-fn` renders each key; the caller supplies it because the right form is
|
||||
a property of the column, not of this function
|
||||
(`app.graph.schema.contract/map-key-fn`)."
|
||||
([m] (format-map m name))
|
||||
([m key-fn]
|
||||
(let [entries (seq m)]
|
||||
(str "map([" (str/join ", " (map #(format-string (key-fn (key %))) entries)) "], "
|
||||
"[" (str/join ", " (map #(format-string (str (val %))) entries)) "])"))))
|
||||
|
||||
(defn map-type?
|
||||
"Is `ladybug-type` a MAP column? Those cannot be bulk-loaded from CSV."
|
||||
[ladybug-type]
|
||||
(and (string? ladybug-type) (str/starts-with? ladybug-type "MAP(")))
|
||||
|
||||
(defn format-typed-value
|
||||
[ladybug-type v]
|
||||
(cond
|
||||
(nil? v)
|
||||
"NULL"
|
||||
|
||||
(map-type? ladybug-type)
|
||||
(format-map v)
|
||||
|
||||
(= ladybug-type "JSON")
|
||||
(format-json v)
|
||||
|
||||
|
||||
@ -32,14 +32,20 @@
|
||||
(defn- link-component-instances!
|
||||
"`IsInstanceOf` from Frame instance heads to their Component.
|
||||
|
||||
beadpot `graph/transform/assets.py::LinkComponentInstances`. An instance
|
||||
head carries `:component-id` pointing at its component record (see
|
||||
`app.common.types.component/instance-of?`); every such head is linked, the
|
||||
main instance and any copy root alike."
|
||||
beadpot `graph/transform/assets.py::LinkComponentInstances`. Every head is
|
||||
linked, the main instance and any copy root alike.
|
||||
|
||||
`component-file` is what makes a head a head here, not `component-id` alone.
|
||||
`app.common.types.component/instance-of?` requires both, and the projection
|
||||
denormalizes `component-id` down the shape tree
|
||||
(`app.graph.project.document`), so on its own it no longer distinguishes a
|
||||
head from a shape that merely lives inside one. `component-file` is not
|
||||
denormalized and remains the head marker Penpot itself uses."
|
||||
[^Connection conn]
|
||||
(run-scalar! conn
|
||||
(str "MATCH (f:Frame), (c:Component) "
|
||||
"WHERE f.component_id = c.id "
|
||||
"AND f.component_file IS NOT NULL "
|
||||
"AND NOT COALESCE(c.deleted, false) "
|
||||
"MERGE (f)-[:IsInstanceOf]->(c) "
|
||||
"RETURN count(*);")))
|
||||
|
||||
@ -30,6 +30,7 @@
|
||||
account for. Schema drift becomes a failing test with a precise message
|
||||
instead of a silently renamed column in a training set."
|
||||
(:require
|
||||
[app.common.json :as json]
|
||||
[clojure.string :as str]))
|
||||
|
||||
(def ^:private renames
|
||||
@ -100,6 +101,23 @@
|
||||
;; `map_extract`; as JSON the transform cannot run at all.
|
||||
"applied_tokens" "MAP(STRING, STRING)"})
|
||||
|
||||
(def ^:private map-key-fns
|
||||
"How to render the *keys* of a MAP column, per column.
|
||||
|
||||
Column names are snake_case because they are graph schema; the keys inside a
|
||||
MAP are not — they are values, and beadpot models them as whatever it parsed
|
||||
from the wire. `applied_tokens` is keyed by shape attribute in the camelCase
|
||||
form Penpot's own JSON encoder produces (`app.common.json/write-camel-key`),
|
||||
which is what beadpot's `AppliedTokenKey` holds and what
|
||||
`UsesToken.for_property` therefore carries: `strokeWidth`, not
|
||||
`stroke-width`."
|
||||
{"applied_tokens" json/write-camel-key})
|
||||
|
||||
(defn map-key-fn
|
||||
"Key renderer for a MAP column; `name` unless the column says otherwise."
|
||||
[column]
|
||||
(get map-key-fns column name))
|
||||
|
||||
(defn column-name
|
||||
"The beadpot column name for Penpot key `k`.
|
||||
|
||||
|
||||
@ -21,6 +21,7 @@
|
||||
[app.common.types.component :as ctk]
|
||||
[app.common.types.file :as ctf]
|
||||
[app.common.types.page :as ctp]
|
||||
[app.graph.ladybug :as ladybug]
|
||||
[app.graph.schema.contract :as contract]
|
||||
[app.graph.schema.projection :as projection]
|
||||
[app.graph.schema.types :as types]
|
||||
@ -233,6 +234,21 @@
|
||||
[table k]
|
||||
(str "`" (column-name table k) "`"))
|
||||
|
||||
(defn format-column-value
|
||||
"Cypher literal for `v` in column `k` of `table`.
|
||||
|
||||
The single place that knows both the column's Ladybug type and the contract
|
||||
detail that a MAP column may render its keys differently from `name` — used
|
||||
by the bulk loader's post-COPY fixups and by the incremental sync alike, so
|
||||
the two cannot disagree about a value's shape."
|
||||
[table k v]
|
||||
(let [ladybug-type (column-ladybug-type table k)]
|
||||
(if (ladybug/map-type? ladybug-type)
|
||||
(if (nil? v)
|
||||
"NULL"
|
||||
(ladybug/format-map v (contract/map-key-fn (column-name table k))))
|
||||
(ladybug/format-typed-value ladybug-type v))))
|
||||
|
||||
(defn- create-node-table-ddl
|
||||
[{:keys [table pk]}]
|
||||
(let [cols (for [k (column-keys table)]
|
||||
|
||||
@ -154,7 +154,7 @@
|
||||
|
||||
(defn- format-node-value
|
||||
[table k v]
|
||||
(ladybug/format-typed-value (nodes/column-ladybug-type table k) v))
|
||||
(nodes/format-column-value table k v))
|
||||
|
||||
(defn- create-node-statement
|
||||
[table attrs]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user