Fix columnMapper to support loading subsets #3564
Conversation
When using columnMapper with ShapeStream, the columns parameter was not being encoded from app column names to database column names. This fix ensures that when a user specifies columns using app format (e.g., camelCase like `userId`), they are properly converted to the database format (e.g., snake_case like `user_id`) before being sent to the server. This makes columnMapper work correctly with column subsets.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3564 +/- ##
=======================================
Coverage 61.64% 61.64%
=======================================
Files 40 40
Lines 1559 1559
Branches 106 107 +1
=======================================
Hits 961 961
Misses 597 597
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds a test to verify that when data comes back from the server with database column names (e.g., snake_case like user_id), the columnMapper properly decodes them to app column names (e.g., camelCase like userId). This complements the encoding test and verifies the full round-trip works correctly with columnMapper and column subsets.
After toInternalParams converts the columns array to a comma-separated string, we need to split it back to encode each column name, then join them back together. Also added type guard for TypeScript safety.
- Add encodeColumns helper function in column-mapper.ts that follows the same pattern as encodeWhereClause - Update client.ts to use encodeColumns for consistent encoding - Add unit tests for encodeColumns function
commit: |
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
I'm worried about the corner case where a column name contains commas. The encodeColumns function won't handle this properly. I'm fine getting this out as a quick fix but we should then make an issue to fix this corner case in a follow up PR.
| encode?: (columnName: string) => string | ||
| ): string { | ||
| if (!columns || !encode) return columns ?? `` | ||
| return columns.split(`,`).map(encode).join(`,`) |
There was a problem hiding this comment.
What if a column name contains a comma? Postgres allows quoted names to contain special characters like commas.
- Rewrite encodeColumns to parse columns respecting quoted identifiers
- Quoted identifiers (double-quoted) are preserved as-is without encoding
- Handle escaped quotes ("") inside quoted identifiers
- Add tests for quoted column names with commas and special characters
Instead of parsing the comma-separated string to handle quoted column names, access the original columns array directly from options and encode each column before joining. This avoids the complexity of parsing quoted identifiers with commas. - Remove encodeColumns helper function - Encode columns directly in #constructUrl from original array - Remove encodeColumns unit tests (covered by stream integration tests)
|
@kevin-dp I realized we were doing this wrong — we have access to the original column data so don't need to bother with parsing issues. |
| // Get original columns array from options (before toInternalParams converted to string) | ||
| const originalColumns = await resolveValue(this.options.params?.columns) | ||
| if (Array.isArray(originalColumns)) { | ||
| const encodedColumns = this.options.columnMapper |
There was a problem hiding this comment.
nit: the ternary is a bit harder to read than:
let encodedColumns = originalColumns
if (this.options.columnMapper) {
encodedColumns = originalColumns.map((col) =>
this.options.columnMapper!.encode(String(col))
)
}| this.options.columnMapper!.encode(String(col)) | ||
| ) | ||
| : originalColumns | ||
| setQueryParam(fetchUrl, COLUMNS_QUERY_PARAM, encodedColumns.join(`,`)) |
There was a problem hiding this comment.
Similar concern to before, what if the columns contain a comma. Imagine a column named "foo,bar" and a column "baz". After this encodedColumns.join(',') it will send "foo,bar,baz" to the server which will incorrectly interpret it as 3 columns.
There was a problem hiding this comment.
as it turns out we've always had this looming edge case —
— but yes, we should fix it — lemme consolidate the logic & then have it fix itAdd quoteIdentifier helper function that properly quotes PostgreSQL identifiers by wrapping in double quotes and escaping internal quotes. This ensures column names with special characters (commas, spaces, quotes) are serialized correctly and won't be misinterpreted by the server when parsing the columns parameter. - Add quoteIdentifier function in column-mapper.ts - Use quoteIdentifier when serializing column names - Add tests for quoteIdentifier and columns with special characters - Update existing tests to expect quoted column names
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Add integration test that verifies column names with commas, quotes, and spaces are correctly handled when selecting specific columns. - Add testWithSpecialColumnsTable fixture that creates a table with columns named "normal", "has,comma", "has\"quote", "has space" - Add integration test that selects a subset of these columns and verifies the data is returned correctly
|
@kevin-dp ok I think we're actually handling things now |
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
Thanks for fixing this Kyle. All looks good now!
When using columnMapper with ShapeStream, the columns parameter was not being encoded from app column names to database column names. This fix ensures that when a user specifies columns using app format (e.g., camelCase like
userId), they are properly converted to the database format (e.g., snake_case likeuser_id) before being sent to the server.This makes columnMapper work correctly with column subsets.