From c3db857936d328527f71baccd33f4cc15535f61d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Sun, 26 Jul 2026 07:07:45 +0200 Subject: [PATCH] :bug: Write graph values Ladybug's CSV reader cannot carry through Cypher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/src/app/graph/bulk.clj | 102 ++++++++++++++++--- backend/src/app/graph/ladybug.clj | 30 +++++- backend/src/app/graph/project/transforms.clj | 14 ++- backend/src/app/graph/schema/contract.clj | 18 ++++ backend/src/app/graph/schema/nodes.clj | 16 +++ backend/src/app/graph/sync.clj | 2 +- 6 files changed, 161 insertions(+), 21 deletions(-) diff --git a/backend/src/app/graph/bulk.clj b/backend/src/app/graph/bulk.clj index a2674a722d..f72db17b42 100644 --- a/backend/src/app/graph/bulk.clj +++ b/backend/src/app/graph/bulk.clj @@ -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)] diff --git a/backend/src/app/graph/ladybug.clj b/backend/src/app/graph/ladybug.clj index 374136dd72..a9f19838bc 100644 --- a/backend/src/app/graph/ladybug.clj +++ b/backend/src/app/graph/ladybug.clj @@ -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) diff --git a/backend/src/app/graph/project/transforms.clj b/backend/src/app/graph/project/transforms.clj index 18cd7513c4..a49d17f89c 100644 --- a/backend/src/app/graph/project/transforms.clj +++ b/backend/src/app/graph/project/transforms.clj @@ -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(*);"))) diff --git a/backend/src/app/graph/schema/contract.clj b/backend/src/app/graph/schema/contract.clj index f2c014fab9..0b20b7f592 100644 --- a/backend/src/app/graph/schema/contract.clj +++ b/backend/src/app/graph/schema/contract.clj @@ -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`. diff --git a/backend/src/app/graph/schema/nodes.clj b/backend/src/app/graph/schema/nodes.clj index 3b37efc9d6..2fde3cb23d 100644 --- a/backend/src/app/graph/schema/nodes.clj +++ b/backend/src/app/graph/schema/nodes.clj @@ -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)] diff --git a/backend/src/app/graph/sync.clj b/backend/src/app/graph/sync.clj index 6691e37791..22ff2f01ef 100644 --- a/backend/src/app/graph/sync.clj +++ b/backend/src/app/graph/sync.clj @@ -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]