feat(postgres): support editing hstore columns#427
Conversation
Reading hstore columns already worked (tokio-postgres decodes them natively as HashMap<String, Option<String>>), but writing back failed with 'Cannot bind a JSON object to a non-JSON column' because hstore has no fixed OID and information_schema reports it generically as 'USER-DEFINED'. - Resolve the real hstore OID per column via pg_type before binding, since it varies per installation/schema. - Bind JSON objects (and, for now, JSON-encoded strings, the plain text cell editor doesn't yet distinguish hstore from other types) as HashMap<String, Option<String>>, which tokio-postgres encodes natively as hstore. - Expose udt_name on TableColumn so the frontend can identify hstore precisely; data_type alone can't, since it's shared with every other extension/custom type. - Route hstore columns through the existing JSON editor in the row editor sidebar, and fix the inline cell editor to show valid JSON instead of '[object Object]' for any object-valued cell. Closes TabularisDB#395
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for tackling this — resolving the per-install OID via pg_type and leaning on tokio-postgres's native HashMap encoding is the right approach, and the binding-layer test coverage is solid. 🙏
One blocking correctness issue inline: the USER-DEFINED fallback in the bind routing regresses editing of other user-defined types (enums, citext, PostGIS…). Fix + a note on the affected test are inline.
Non-blocking, your call:
bind_pg_hstoreduplicates the oid-resolve →hstore_map_from_json_object→Type::new→BoundValueblock across theObjectandStringarms — could fold into a small local closure.get_hstore_oid_for_columnfires one catalog query perUSER-DEFINEDcolumn per row in theinsert_recordloop — fine for single-row edits, worth caching if bulk insert ever routes through here.Type::new(..., "public")hardcodes the schema; harmless sinceprepare_typedpins on the OID, just noting hstore can live in another schema.
Also: the branch currently has a merge conflict against main (mergeStateStatus: DIRTY) — could you rebase/resolve that when you get a chance? 🙏
| if options.hstore_oid.is_some() || options.column_type == Some("USER-DEFINED") { | ||
| return bind_pg_hstore(value, placeholder_idx, options.hstore_oid); | ||
| } |
There was a problem hiding this comment.
Regression: this captures every USER-DEFINED column, not just hstore. Enums, citext, domains and PostGIS types all report data_type = 'USER-DEFINED', so editing e.g. an enum value "happy" now routes to bind_pg_hstore → the String arm → serde_json::from_str("happy") fails → "hstore column requires a JSON object value...". Before this PR that string fell through bind_pg_string to the text fallback and worked.
The clause is also redundant — get_hstore_oid_for_column already confirms typname = 'hstore', so hstore_oid.is_some() is true iff the column really is hstore. Gate on that alone:
| if options.hstore_oid.is_some() || options.column_type == Some("USER-DEFINED") { | |
| return bind_pg_hstore(value, placeholder_idx, options.hstore_oid); | |
| } | |
| if options.hstore_oid.is_some() { | |
| return bind_pg_hstore(value, placeholder_idx, options.hstore_oid); | |
| } |
Heads up: hstore_object_without_resolved_oid_returns_clear_error asserts the error contains "hstore"; after this change it hits the generic "Cannot bind a JSON object to a non-JSON column" path, so that test needs updating.
| }) | ||
| } | ||
|
|
||
| // The grid's plain-text cell editor doesn't yet know about hstore, so it |
There was a problem hiding this comment.
Nit: this comment sits at column 0 — cargo fmt will flag it. Align it with the block:
| // The grid's plain-text cell editor doesn't yet know about hstore, so it | |
| // The grid's plain-text cell editor doesn't yet know about hstore, so it |
Reading hstore columns already worked (tokio-postgres decodes them natively as HashMap<String, Option>), but writing back failed with 'Cannot bind a JSON object to a non-JSON column' because hstore has no fixed OID and information_schema reports it generically as 'USER-DEFINED'.
Closes #395