Skip to content

feat(postgres): support editing hstore columns#427

Open
arturbent0 wants to merge 1 commit into
TabularisDB:mainfrom
arturbent0:feat/postgres-hstore-support
Open

feat(postgres): support editing hstore columns#427
arturbent0 wants to merge 1 commit into
TabularisDB:mainfrom
arturbent0:feat/postgres-hstore-support

Conversation

@arturbent0

Copy link
Copy Markdown
Contributor

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'.

  • 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>, 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 #395

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 NewtTheWolf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_hstore duplicates the oid-resolve → hstore_map_from_json_objectType::newBoundValue block across the Object and String arms — could fold into a small local closure.
  • get_hstore_oid_for_column fires one catalog query per USER-DEFINED column per row in the insert_record loop — fine for single-row edits, worth caching if bulk insert ever routes through here.
  • Type::new(..., "public") hardcodes the schema; harmless since prepare_typed pins 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? 🙏

Comment on lines +142 to +144
if options.hstore_oid.is_some() || options.column_type == Some("USER-DEFINED") {
return bind_pg_hstore(value, placeholder_idx, options.hstore_oid);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this comment sits at column 0 — cargo fmt will flag it. Align it with the block:

Suggested change
// 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Add support for HSTORE type (postgresql)

2 participants