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

Druotic
Copy link
Contributor

@Druotic 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.

Copy link
Member

@elhigu elhigu left a comment

Choose a reason for hiding this comment

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

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

@elhigu
Copy link
Member

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
Copy link
Member

elhigu commented Jan 18, 2019

Looks like there is some underlying subtle differences 🤔

@Druotic
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
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 knex:master Jan 19, 2019
@elhigu
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants