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

Fix rowMode='array' in postgres patcher #363

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Conversation

isaacl
Copy link
Contributor

@isaacl isaacl commented Dec 22, 2020

The patcher incorrectly strips rowMode from calls of the form:
c.query({ text: , rowMode: 'array' })

Fixes #364.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@isaacl
Copy link
Contributor Author

isaacl commented Dec 24, 2020

I am still not sure this works, I had some weird behavior in xray but may be unrelated.

@willarmiros
Copy link
Contributor

Hi @isaacl,

Thanks for this contribution, do you think you could add a unit test to verify this fixes the problematic behavior?

@isaacl
Copy link
Contributor Author

isaacl commented Dec 30, 2020

OK I did a more involved patch where I use the argument logic from the pg module itself.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Looks great to me! This will be an awesome simplification, just one small comment. Thanks!

});

it('should pass down raw sql query', function() {
query.call(postgres, 'sql here', ['values']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also assert that addNewSubsegment is calledOnce in both of these tests? I know we're already testing it in other tests, but since this specific use case was broken before I want to ensure it fully works now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Thanks again! Verified with previous versions of the pg library that this change will be backwards compatible, since the query() API hasn't really changed apart from adding the query config object as an alternative to a string for the first argument, which this change adds support for.

@willarmiros willarmiros merged commit e8f85d5 into aws:master Jan 7, 2021
EkeMinusYou pushed a commit to EkeMinusYou/aws-xray-sdk-node that referenced this pull request Jan 28, 2021
* Fix rowMode='array' in postgres patcher

* Rewrite pg argument handling logic

* Add called assertion to postgres tests
@isaacl
Copy link
Contributor Author

isaacl commented Feb 4, 2021

can haz release?

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.

postgres patcher breaks rowMode: array query
2 participants