Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

Support Knex#468

Merged
DominicKramer merged 14 commits intogoogleapis:masterfrom
DominicKramer:feature/support-knex
May 23, 2017
Merged

Support Knex#468
DominicKramer merged 14 commits intogoogleapis:masterfrom
DominicKramer:feature/support-knex

Conversation

@DominicKramer
Copy link
Copy Markdown
Contributor

@DominicKramer DominicKramer commented Apr 14, 2017

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2017
@DominicKramer
Copy link
Copy Markdown
Contributor Author

DominicKramer commented Apr 14, 2017

PTAL

matthewloring
matthewloring reviewed Apr 16, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

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

It looks good overall but I agree that we should understand what async work is being done by knex to cause this context loss (even though this bluebird patch does fix this problem).

Comment thread test/plugins/test-trace-knex.js Outdated
databaseResultReportingSize: RESULT_SIZE
});
assert = require('assert');
knex = require('./fixtures/knex0.12')({

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/plugins/plugin-bluebird.js Outdated
}

function patchPromise(Promise, api) {
shimmer.wrap(Promise.prototype, '_addCallbacks', function(original) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer DominicKramer force-pushed the feature/support-knex branch from 425818b to 6010c4b Compare May 4, 2017 19:37
matthewloring
matthewloring suggested changes May 11, 2017
View reviewed changes
Comment thread src/plugins/plugin-knex.js
shimmer.wrap(Client.prototype, 'runner', function(original) {
return function() {
var runner = original.apply(this, arguments);
runner.query = api.wrap(runner.query);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/plugins/plugin-knex.js Outdated
function interceptKnex(Knex, api) {
return function() {
var result = Knex.apply(this, arguments);
var proto = Object.getPrototypeOf(result.client);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/plugins/plugin-knex.js Outdated
var proto = Object.getPrototypeOf(result.client);
shimmer.wrap(proto, 'transaction', function(original) {
return function() {
var args = Array.prototype.slice.call(arguments).map(function(item) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer
Initial commit
698af35
@DominicKramer
Fix the test-config-plugins.js test
6b9b5da
@DominicKramer
Fix context loss in knex without patching bluebird
a7c6bdf 
I think this patch is specific to mysql.  I will investigate this
further.
@DominicKramer
Fix the test-config-plugins.js test
f80681f
@DominicKramer
Intercept the module root to patch transaction
aa7279a 
This commit intercepts the module root to patch the `transaction`
method.  This works because the `knex` module exports a
constructor and the object that the constructor constructs has a
`client` property that is the client being used.  This client
can have its `transaction` method updated in a way to preserve
context.
@DominicKramer DominicKramer force-pushed the feature/support-knex branch from 80d3264 to aa7279a Compare May 13, 2017 00:04
matthewloring
matthewloring suggested changes May 17, 2017
View reviewed changes
Comment thread src/plugins/plugin-knex.js Outdated
var util = require('util');
var is = require('is');

var VERSIONS = '0.12.x';

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/plugins/plugin-knex.js Outdated

function interceptTransaction(Transaction, api) {
function WrappedTransaction(client, container, config, outerTx) {
Transaction.call(this, client, wrapIfFn(container, api), config, outerTx);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

matthewloring
matthewloring approved these changes May 23, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

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

LGTM with nit once appveyor and circle are passing (travis failures are unrelated).

Comment thread test/plugins/test-trace-knex.js Outdated
enhancedDatabaseReporting: true,
databaseResultReportingSize: RESULT_SIZE
});
assert = require('assert');

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer DominicKramer merged commit dd7bc9b into googleapis:master May 23, 2017
vmarchaud pushed a commit to keymetrics/trassingue that referenced this pull request Jun 1, 2017
@DominicKramer @vmarchaud
Support Knex (#468)
b5c7603 
This commit includes support and tests for knex satsifying `>=0.10 <=0.13`.

PR-URL: googleapis/cloud-trace-nodejs#468
@matthewloring matthewloring mentioned this pull request Jun 7, 2017
Closed
yoshi-automation added a commit that referenced this pull request Apr 7, 2020
@yoshi-automation
fix: apache license URL (#468)
933f429 
googleapis/synthtool@1df68ed
commit 1df68ed6735ddce6797d0f83641a731c3c3f75b4
Author: Alexander Fenster <[email protected]>
Date:   Mon Apr 6 16:17:34 2020 -0700

    fix: apache license URL (#468)
gcf-merge-on-green Bot pushed a commit that referenced this pull request Apr 7, 2020
@yoshi-automation
fix: apache license URL (#468) (#1232)
ac7e886 
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/9c94202f-63a5-4df0-9d76-871a00f99b85/targets
Mistic92 pushed a commit to Mistic92/cloud-trace-nodejs that referenced this pull request Apr 15, 2020
@yoshi-automation @Mistic92
fix: apache license URL (googleapis#468) (googleapis#1232)
6ea2bae 
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/9c94202f-63a5-4df0-9d76-871a00f99b85/targets
@release-please release-please Bot mentioned this pull request Jun 30, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@matthewloring matthewloring matthewloring approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DominicKramer @matthewloring @googlebot