Skip to content
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

Always use well documented pg client query() config argument #3004

Merged
merged 2 commits into from Jan 19, 2019

Conversation

Projects
None yet
2 participants
@Druotic
Copy link
Contributor

Druotic commented Jan 18, 2019

For queries that included bindings, I found that arguments were sent in this form:

pg.query({ text: 'SELECT ....' }, ['some', 'bindings'], callback);

Technically, this works when calling out to the Node.js Postgres client directly. However, it is not documented publicly (see here). This change just adopts an approach that is public documented and therefore less prone to breaking in the future. After this change, the above becomes:

pg.query({ text: 'SELECT ....', values: ['some', 'bindings']}, callback);

The motivation behind this change was for other libraries that try to instrument the postgres client in between. Using undocumented inputs can lead to issues if the library didn't consider all possible undocumented input patterns. My particular usecase was an AWS X-Ray instrumentation package, aws-sdk-xray-postgres

note: I was only able to get a subset of tests to pass on my local machine. I added a unit test for explicit arg checking, but am relying on existing integration tests to ensure no regressions in basic querying functionality. I'm relying on the CI job to run the remainder of tests. If something fails, I may need pointers on how to get the full suite running locally - I wasn't able to get everything running using the existing docs... but perhaps I missed some steps.

@elhigu

elhigu approved these changes Jan 18, 2019

Copy link
Collaborator

elhigu left a comment

Looks perfect! Lets wait that tests finish running and merge.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 18, 2019

1) postgresql | pg
       Joins
         supports joins with overlapping column names:
      AssertionError: expected [ Array(2) ] to deeply equal [ Array(2) ]
      + expected - actual
       [
         {
      -    "email": "test2@example.com"
      +    "0": "test@example.com"
      +    "1": "test2@example.com"
         }
         {
      -    "email": "test3@example.com"
      +    "0": "test@example.com"
      32m+    "1": "test3@example.com"
         }
       ]
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 18, 2019

Looks like there is some underlying subtle differences 🤔

@Druotic

This comment has been minimized.

Copy link
Contributor Author

Druotic commented Jan 18, 2019

Aw, darn. I'll dig into this some more after lunch, I must have not considered some edge case.

@Druotic

This comment has been minimized.

Copy link
Contributor Author

Druotic commented Jan 18, 2019

Fixed! Silly typo. The main path we utilize doesn't hit the options path, so I didn't catch it the first time. 👍 for integration tests!

@elhigu elhigu merged commit 816695b into tgriesser:master Jan 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 84.945%
Details
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 19, 2019

Thank you very much! Pleasure to work with you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.