fix: resolve domain types to their base type serializer/parser - #1187
Open
san7iya wants to merge 2 commits into
Open
fix: resolve domain types to their base type serializer/parser#1187san7iya wants to merge 2 commits into
san7iya wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes array serialization for Postgres domain types that wrap arrays (e.g. CREATE DOMAIN x AS text[]) by resolving each domain OID to its base type OID and reusing the existing serializer/parser for that base type, preventing malformed array literal errors on inserts.
Changes:
- Extend
fetchArrayTypes()to also fetch domain types (typtype='d') in the same round-trip and register domain OIDs to use their base type serializer/parser. - Add
addDomainType()to map domain OIDs to base type handlers. - Add a regression test covering a domain-wrapped array column insert/return round-trip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/connection.js | Adds domain type resolution/mapping so domain OIDs reuse base serializers/parsers (and combines lookups into one round-trip). |
| tests/index.js | Adds regression coverage for inserting/returning an array via a domain-wrapped array column. |
Suppressed comments (1)
src/connection.js:797
- addDomainType unconditionally overwrites parsers/serializers for the domain OID when a base mapping exists. This can clobber user-provided type handlers (mergeUserTypes allows custom parsers/serializers), which is an API-breaking behavior change. Only fill in missing handlers for the domain OID.
function addDomainType(oid, basetype) {
if (options.parsers[basetype]) options.parsers[oid] = options.parsers[basetype]
if (options.serializers[basetype]) options.serializers[oid] = options.serializers[basetype]
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #592
Postgres domain types (CREATE DOMAIN x AS text[]) aren't tagged with typcategory = 'A', so fetchArrayTypes() never registered a serializer/parser for them — inserting into a domain-typed array column failed with malformed array literal.
This adds a lookup for domain types (typtype = 'd'), resolving each domain's OID back to its base type (typbasetype) and reusing whichever serializer/parser is already registered for that base type.
Both lookups are combined into a single round-trip (multi-statement query, same pattern already used in fetchState()) rather than two separate queries. An earlier version of this fix used two sequential awaited queries, which introduced a race condition: needsTypes flips to false after the first round-trip completes, which lets the connection execute the next queued query before a second query would have finished populating the serializer map. Bundling both into one round-trip avoids that.
Verified locally against PostgreSQL 18 with both a domain-wrapped array column and a plain array column, confirming correct storage and no regression in the plain-array path. Added a test following the existing array-test conventions in tests/index.js; wasn't able to run the full local suite due to its Linux-oriented bootstrap script (creates test users via bare psql/createdb calls), but the new test's logic is verified in isolation and CI should cover the rest.