The Wayback Machine - https://web.archive.org/web/20200906050735/https://github.com/brianc/node-postgres/pull/2215/
Skip to content

/ node-postgres

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Latin 1 client encoding support for the pure JavaScript client #2215

Draft
wants to merge 4 commits into
base: master
from
Draft

Latin 1 client encoding support for the pure JavaScript client #2215

wants to merge 4 commits into from

Conversation

@boromisp
Copy link
Contributor

boromisp commented May 13, 2020

This PR fixes the handling of the client_encoding parameter in the pure JavaScript client.
The intersection of the natively supported text encodings of Node.js and Postgres is UTF-8 and Latin 1.

The main (only?) use-case for this is compatibility with existing SQL_ASCII databases containing LATIN1 encoded text.

I've could not get the native client working.

Related issues: #2203 (#2204 unconfirmed)
@brianc
brianc reviewed May 13, 2020
View changes
packages/pg-protocol/src/serializer.ts Show resolved Hide resolved
@brianc
brianc reviewed May 13, 2020
View changes
packages/pg/test/integration/connection/test-helper.js
@@ -6,7 +6,11 @@ var utils = require(__dirname + '/../../../lib/utils')
var connect = function (callback) {
var username = helper.args.user
var database = helper.args.database
var con = new Connection({ stream: new net.Stream() })
var client_encoding = helper.args.client_encoding

This comment has been minimized.

Sign in to view
@brianc

brianc May 13, 2020

Owner

I see you're passing this in here which is cool, but were you intending on setting this in CI to test both permutations? AFAIK this will always still default to utf-8 for now?

This comment has been minimized.

Sign in to view
@boromisp

boromisp May 13, 2020

Author Contributor

I haven't considered how to test it. Would it be better to write a few targeted integration and unit tests instead of running all the tests with different client encodings?

This comment has been minimized.

Sign in to view
@brianc

brianc Jun 18, 2020

Owner

Would it be better to write a few targeted integration and unit tests instead of running all the tests with different client encodings?

yeah i think so - running the whole test suite again just to test a different encoding is overkill IMO
@brianc
Copy link
Owner

brianc commented May 13, 2020

whoah that was super fast! thanks! LMK if there's anything else you want me to take a look at or any additional tests you'd like me to write. I left a touch of feedback on there but this is fabulous. 👯
@boromisp boromisp marked this pull request as draft Jun 1, 2020
boromisp added 3 commits May 13, 2020
@boromisp
latin1 client_encoding support for the pure JavaScript client
2fad45c
@boromisp
Resolve review comment
1c6b789
@boromisp
A more thorough client_encoding implementation
Loading status checks…
78af388 
The pg-protocol/Parser and the pg/Connection automatically change
their encoding based on the ParameterStatus messages received
from the server.
@boromisp boromisp force-pushed the boromisp:fix-client_encoding branch from 648d8b2 to 78af388 Jun 1, 2020
@boromisp
Node 8.x compatibility fix
Loading status checks…
ec2da25
@boromisp
Copy link
Contributor Author

boromisp commented Jun 1, 2020 •
edited

@brianc I've made a couple of improvements:

  • Validate that the chosen client_encoding is supported by the library (UTF8 and LATIN1 with the JavaScript and only UTF8 with the native client)
  • Detect client_encoding changes and use the new encoding for parsing and serializing messages
  • pg-protocol encoding unit tests

There is a known bug in the current implementation:

If the user sets the client_encoding with a query to an unsupported value (eg. 'SET client_encoding TO LATIN2'), then when the Parser intercepts the ParameterStatus message and tries to change its encoding, it will throw an error. This goes uncaught, since there is no exception handling in Parser.parse.

The simplest way to handle this error would be to terminate the connection, but I'm not sure how to propagate this error to the Connection.

As an aside, since the only use-case for this code seems to be supporting misconfigured databases (SQL_ASCII server_encoding with non-ASCII text) it might be worth considering dropping all this code and simply enforcing UTF8 encoding with errors and documentation. That's what the JDBC driver is doing.
@brianc
Copy link
Owner

brianc commented Jun 3, 2020

Hey sorry for the delay! I was out on vacation & just getting back...I'll take a look at this tomorrow! ❤️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianc brianc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@boromisp @brianc
You can’t perform that action at this time.