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

Pass migration config object as an argument to the migrations (V 2.0) #2802

Merged
merged 20 commits into from Sep 26, 2018

Conversation

Projects
None yet
3 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Sep 11, 2018

Reuses some tests from #2778 but implemented in a completely different way.

dofrisec and others added some commits Aug 21, 2018

Pass migration config object as an argument to the migrations themsel…
…ves, to allow custom parameters to be set for migrations. (#2014)
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 11, 2018

@elhigu Updated existing PR to make it mergeable; please let me know if something is missing, will address ASAP - would like to unblock #2799

@kibertoad kibertoad closed this Sep 13, 2018

@kibertoad kibertoad reopened this Sep 13, 2018

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 13, 2018

I still think that knex should have more general solution which can be used to resolve also this issue.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 13, 2018

Yeah, I will update this PR based on our discussion in the old PR.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

knex/documentation#135 -> documentation

kibertoad added some commits Sep 17, 2018

@elhigu
Copy link
Collaborator

elhigu left a comment

Tests should also check that when transaction is created that user parameters can be accessed through that trx knex instance.

Also if withUserParams is called for transaction knex client. Maybe and error should be thrown.

Lastly currently there doesn't seem to be a way to setup those initial userParam values through configuration (thus they cannot be set in knexfile.js which for me would be the most common usecase for setting them (that could be implemented also later on I think).

src/knex.js Outdated
@@ -72,3 +73,9 @@ Knex.raw = (sql, bindings) => {
);
return new Raw().set(sql, bindings);
};

Knex.withUserParams = (params) => {
const knexClone = clone(this);

This comment has been minimized.

@elhigu

elhigu Sep 17, 2018

Collaborator

Isn't there actually knex.clone() which should be used for this?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

if withUserParams is called for transaction knex client. Maybe and error should be thrown

Should I check by this instanceof Transactor or there is a better way?

Tests should also check that when transaction is created that user parameters can be accessed through that trx knex instance.

How is that different from existing transaction of a copy with userParams retains userparams test?

(that could be implemented also later on I think)

I would appreciate that :). Stayed till 5 AM yesterday to finish this thing, need to recharge before I'll be able to do larger changes again.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

@elhigu OK, changes for initial param support were surprisingly straightforward to implement, so I think all of your comments are covered now (as previously mentioned, there was a test for userParams in transaction already).

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 17, 2018

This looks really promising, but I need to check this out when I'm less tired.

I might try to do some additional manual testing by setting temporarily all knex test instances to be cloned instances to see that tests will work perfectly with those too :)

I also was worried that this could have been actually a lot harder to implement so that everything works fine after cloning, but I'm glad I was wrong. Great work and very much appreciated 👍

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

@elhigu Well, admittedly, it did took a bit of black magic to get cloning exactly right, as we have a function which is also an object with getters, which is not that straightforward to clone, but fortunately JS is flexible enough. You are welcome!

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 25, 2018

I got finally time to do some manual testing of this.

I changed in suite.js passed knex instances to be cloned ones and some of the tests seem to be failing. Most of them seem to be related to knex.raw and maybe to postProcessResponse hook.

    require('./schema')(knex.withUserParams({}));
    require('./migrate')(knex.withUserParams({}));

    require('./seed')(knex.withUserParams({}));
    require('./builder/inserts')(knex.withUserParams({}));
    require('./builder/selects')(knex.withUserParams({}));
    require('./builder/unions')(knex.withUserParams({}));
    require('./builder/joins')(knex.withUserParams({}));
    require('./builder/aggregate')(knex.withUserParams({}));
    require('./builder/updates')(knex.withUserParams({}));
    require('./builder/transaction')(knex.withUserParams({}));
    require('./builder/deletes')(knex.withUserParams({}));
    require('./builder/additional')(knex.withUserParams({}));
    require('./datatype/bigint')(knex.withUserParams({}));
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 25, 2018

@elhigu thanks, will take a look today!

kibertoad added some commits Sep 25, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 25, 2018

@elhigu OK, I've addressed everything that looked like genuine problems - remaining ones look like they are related to some isolation between different copies happening (different transactions?) that doesn't look like something worth spending effort on resolving, as I'm not even sure that it's an invalid behaviour, and user is not really supposed to expect different copies of knex to run in single transaction (you even requested explicitly to forbid transaction copying). If you run

const knexCopy = knex.withUserParams({});
require('./schema')(knexCopy);
    require('./migrate')(knexCopy);

    require('./seed')(knexCopy);
    require('./builder/inserts')(knexCopy);
    require('./builder/selects')(knexCopy);
    require('./builder/unions')(knexCopy);
    require('./builder/joins')(knexCopy);
    require('./builder/aggregate')(knexCopy);
    require('./builder/updates')(knexCopy);
    require('./builder/transaction')(knexCopy);
    require('./builder/deletes')(knexCopy);
    require('./builder/additional')(knexCopy);
    require('./datatype/bigint')(knexCopy);

it seems to pass fine now.

@elhigu

elhigu approved these changes Sep 26, 2018

@elhigu elhigu merged commit d4a3387 into tgriesser:master Sep 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+3.02%) to 88.686%
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

Woohoo, the odyssey is over now!

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 26, 2018

Thanks! I'll make new @next release with this soon.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

Thank you! Any chance #2817 could also get in if there is nothing wrong with the implementation? (I'll resolve conflict in a couple of minutes)

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 26, 2018

I think this + babel update together might have broken something

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

I can totally look into it right now, what's the problem and how I can reproduce?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 26, 2018

tests in #2817 are failing with node 6 and I just tested locally latest master and it failed too TypeError: Object.getOwnPropertyDescriptors is not a function maybe node 6 does not have that one.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

Yup. And Babel seems to be incapable to transpile that one. Is https://www.npmjs.com/package/object.getownpropertydescriptors an acceptable option?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

Alternatively withUserParams could be made node-8+-only feature with explicit error thrown on old versions.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 26, 2018

This one suggests https://www.npmjs.com/package/babel-preset-eslatest-node6 to use https://babeljs.io/docs/en/babel-polyfill/ for that support. Seems a bit more trustworthy.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

Right. Will add it now

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 26, 2018

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