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

Do not postprocess internal queries in Migrator #2914

Merged
merged 4 commits into from Nov 19, 2018

Conversation

Projects
None yet
3 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Nov 17, 2018

fixes #2644
Also fixes .withUserParams() method to work correctly with .queryBuilder()

.then(
(exists) =>
.then((exists) => {
return !exists && _createMigrationTable(tableName, schemaName, trxOrKnex);

This comment has been minimized.

@kibertoad

kibertoad Nov 17, 2018

Collaborator

it's a huge pain to debug arrow functions without {} and prettier really butchers formatting for them, hence the change

@@ -10,24 +10,23 @@ import batchInsert from './batchInsert';
export default function makeKnex(client) {
// The object we're potentially using to kick off an initial chain.
function knex(tableName, options) {

This comment has been minimized.

@kibertoad

kibertoad Nov 17, 2018

Collaborator

There are more changes here than I am comfortable with, TBH. Main reason - there was actually a creeping bug after withUserParams change - invoking knex('tableName') would silently create a builder from wrong (original) knex instance. While figuring out a solution I've implemented a stricter separation between knex() function (which should be as lightweight as possible) and the actual knex execution context (that knex function proxies to). I'm still not happy about our magical knex entity which is function, object and class instance at the same time, but at least there is clearer separation of what is responsible for what now)

@@ -0,0 +1,3 @@
exports.up = function(knex) {};

This comment has been minimized.

@kibertoad

kibertoad Nov 17, 2018

Collaborator

having this kinda violates the rule of unit tests having no dependencies, but mocking out require() call for a migration or restructuring migrator to be able to mock out entire sequences is a huge pain, and we don't really need an integration test that would be going over all dialects here at all as we are not testing any db functionality here.

@@ -125,4 +166,20 @@ describe('knex', () => {
/Knex: run\n$ npm install oracle/
);
});

describe('async stack traces', () => {

This comment has been minimized.

@kibertoad

kibertoad Nov 17, 2018

Collaborator

moved this from integration tests since it's actually more of a unit test and it's easier to debug it here

@@ -2,9 +2,9 @@
/*eslint no-var:0, max-len:0 */
'use strict';

This comment has been minimized.

@kibertoad

kibertoad Nov 17, 2018

Collaborator

no meaningful changes in this file, just var -> const and moved one test to unit tests

kibertoad added some commits Nov 17, 2018

// Clone knex instance and remove post-processing that is unnecessary for internal queries from a cloned config
this.knex = isFunction(knex)
? knex.withUserParams(knex.userParams)
: Object.assign({}, knex);

This comment has been minimized.

@heisian

heisian Nov 18, 2018

Contributor

This is great! I attempted to clone knex but unsuccessfully in some prototyping code - I would receive an error (forgot what it was) attempting to make queries with my cloned version. Good to see how it is done here.

This comment has been minimized.

@kibertoad

kibertoad Nov 18, 2018

Collaborator

Admittedly I'm abusing .withUserParams function, but given that it includes all the necessary black magic for cloning to actually produce a fully usable knex instance, that's the best option we have now. Maybe we should eventually replace .clone function with what .withUserParams currently does and call it under-the-hood.

This comment has been minimized.

@elhigu

elhigu Nov 19, 2018

Collaborator

This is good. However in future if skipping custom post process response handlers, becomes common use case, we might even add special method for that like .skipCustomPostProcessResponse.

@elhigu

elhigu approved these changes Nov 19, 2018

Copy link
Collaborator

elhigu left a comment

Everything did seem fine. I couldn't spot anything that would need to be chaged. 👍

@kibertoad kibertoad merged commit 1eac063 into master Nov 19, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 84.89%
Details

@kibertoad kibertoad deleted the fix/2644-internal-post-processing branch Nov 19, 2018

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 20, 2018

@elhigu When do you think the next tagged release will be?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 20, 2018

@heisian Soon. I'm updating changelog and package.json today, but I also hope to squeeze #2861 in - which depends on external project merging and releasing; if it doesn't happen tomorrow, probably we are going to release without it.

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 20, 2018

@kibertoad dope, thank you

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Do not postprocess internal queries in Migrator (tgriesser#2914)
* Do not postprocess internal queries in Migrator

* Fixes

* Fix var -> const autofix

* Fix regression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment