The knex version we're using has a known bug that prevents testing connections

[pragone]

Informations

• Node.js version: v10.15.0
• NPM version: 6.4.1
• Strapi version: 3.0.0-alpha.18
• Database: mysql
• Operating system: MacOS

What is the current behavior?
Strapi uses an old version of knex with a known issue.
Specifically the issue I'm experiencing is that after connections to the DB have been idle for quite a while, they may be disconnected, but because of knex/knex#2321 (which I've checked the code and the issue is also affecting knex 0.13.0 which is the version we're using), the connection is not actually being tested before it is used.
The comment that shows the actual key of the issue in knex is knex/knex#2321 (comment) and it also suggests the fix: to pass testOnBorrow: true as a config to the pool.

Now, because the connection pool options are not very configurable (which is called out in #2030) we need to change the code in the knex hook to be able to fix it.

Considering that the fix in knex was to set this option as true by default I'd suggest we do the same.

(Alternatively we could upgrade knex, but the later versions use a different connection pool library tarn instead of generic-pool and that may be a breaking change.

Steps to reproduce the problem
Bring app up that uses a mysql connection.
Let it sleep for a few hours or kill the connection somehow else
Make a request that need the DB
It has an error and does not recover

What is the expected behavior?
It should reconnect to the DB and fulfil the request

Suggested solutions
I'll submit a PR, but I believe it should be as simple as setting testOnBorrow: true as default on the knex options. Also, TBH, there's no good reason why the pool options are not being set and are configurable wherever we use knex. So I'll include some of that in the PR.

[derrickmehaffy]

@pragone Good catch, I had noticed the connection timeouts and my cluster killing them myself but hadn't looked deeper. I think we should test updating to the current recommended version (Separate PR) but in the short term I think you are right on bringing in the recommended changes.

Once you get the PR up feel free to ping me as I would be interested in helping test it.

[pragone]

Well... I'm not so sure now that the solve is so simple. Turns out that while the knex code that initialises the connection pool does not change between 0.13.0 (the version we're using) and 0.14.0 (the version for which the bug was raised), the version of generic-pool does change:

• knex 0.13.0 uses generic-pool ^2.4.2
• knex 0.14.0 uses generic-pool ^3.1.7

Turns out that in 2.4.2 there's no testOnBorrow option :(
While that should have been good news because the validate function is used always, the reality is that it's bad news because it means that this is failing because of another bug in the code of connection pool management.

From a bit more reading they say that knex fixed quite a few connection pool related issues after 0.14.3 (which is when they switched away from generic-pool to tarn).
So possibly, the right thing to do is to upgrade knex.

[derrickmehaffy]

@pragone I remember there being some possible breaking changes between versions outside of what you already mentioned. I think I dropped some links to @lauriejim in slack but I should probably read more into it myself

[pragone]

Perhaps... But TBH, this is becoming a blocker for me. Having a dependable connection pooling mechanism is a hygiene factor of any production-ready system.
You'd be surprised (well... or perhaps not :) of how big a difference a proper connection pool can do in a high traffic site. So I think I'll give it a go.
The good news is that currently in packages/strapi-hook-knex/lib/index.js we're only passing two pool parameters that are the same that tarn expects. So it may be non-breaking from a config perspective.
The big question is whether the actual query building api that knex exposes (which is what is used by the generated code changes at all). From reading the breaking changes in the https://github.com/tgriesser/knex/blob/master/CHANGELOG.md it doesn't seem to be the case.

[pragone]

BTW... thanks @derrickmehaffy for the responsiveness 👍

[derrickmehaffy]

No problem @pragone this directly affects me as well. If you want to give it a shot and update here on your testing I'll start playing around as well. Your skills are probably a bit better than mine but I do agree on the production ready system. There are a few other things that the bookshelf side of Strapi needs to have done for it to be considered "Production ready" mainly getting bookshelf at least up to the current level of mongoose in terms of usability.

[derrickmehaffy]

A few key things I see in that changelog that also will directly affect #2562

0.16.2
Add json type support for SQLite 3.9+ (tested to work with Node package 'sqlite3' 4.0.2+)
Added support for named unique, primary and foreign keys to SQLite3
SQlite3 renameColunm quote fix

There is also quite a few other misc bug fixes in latest versions I think would benefit Strapi going forward, so we should focus on making sure nothing breaks while updating to newer versions. Doing so will make the job easier I referenced in my previous comment

[pragone]

Hi @derrickmehaffy. I've submitted a PR with the change in question. Would love some help on defining a testing process for this.
TBH, I tried it in my local and it worked nicely. No breaks... but well, we all know that that is probably not enough.

I re-read the changelos, and these are all the breaking changes in Knex for the upgrade we're doing here (from 0.13.0 to 0.16.2):

0.16.1

• Use datetime2 for MSSQL datetime + timestamp types. This change is incompatible with MSSQL older than 2008 This is very old, doubt it'd be an issue
• Knex.VERSION() method was removed, run "require('knex/package').version" instead Users of the framework rarely would need to use this
• Knex transpilation now targets Node.js 6, meaning it will no longer run on older Node.js versions I think you guys are targetting node >10, so don't think this is an issue
• Add json type support for SQLite 3.9+ (tested to work with Node package 'sqlite3' 4.0.2+) Not sure why this would be breaking, but it doesn't sound like something that would break the framework

0.15.0

• Stop executing tests on Node 4 and 5. (not supported anymore) Again, not an issue
• json data type is no longer converted to text within a schema builder migration for MySQL databases (note that JSON data type is only supported for MySQL 5.7.8+) This one might require looking into
• Removed WebSQL dialect Knex support wasn't good anyway, so doubt it was in much use
• Drop mariadb support You can use mysql instead
• Primary Key for Migration Lock Table. This shouldn't affect to old loc tables, but if you like to have your locktable to have primary key, delete the old table and it will be recreated when migrations are ran next time. OK
• Ensure knex.destroy() returns a bluebird promise Not sure why this is breaking, but the change just switches a primitive promise with a bluebird one... still a promise though, doesn't change the api fundamentally
• Increment floats Might be marked as breaking and that's "right", but the previous behaviour was wrong... This was a bug
• Testing removal of 'skim', Now rows are not converted to plain js objects, returned row objects might have changed type with oracle, mssql, mysql and sqlite3 skim is not used by the framework
• Drop support for strong-oracle another dialect that was dead/not well supported by knex anyway
• Timeout errors doesn't silently ignore the passed errors anymore Desirable behaviour
• Removed WebSQL dialect Dead dialect
• Various fixes to mssql dialect to make it compatible with other dialects , Unique constraint now allow multiple null values, float type is now float instead of decimal, rolling back transaction with undefined rejects with Error, select for update and select for share actually locks selected row, so basically old schema migrations will work a lot different and produce different schema like before. Also now MSSQL is included in CI tests. OK... this one we might be a change, but again I'm not sure it'll affect the behaviour of the framework

0.14.0

• Remove sorting of statements from update queries Interesting issue... seems like a bug fix though
• Updated allowed operator list with some missing operators and make all to lower case well... not really breaking. Strictly speaking the API changes, but the behaviour not
• Use node-mssql 4.0.0 Should be fine
• Support for enum columns to SQlite3 dialect Enhancement... why breaking?
• Better identifier quoting in Sqlite3 Enhancement... why breaking?
• Migration Errors - Display filename of of failed migration Enhancement... why breaking?

[pragone]

Ok... having said all that, the questions are:
1.- What is the testing required? All the tests in the CI passed, does that give you enough confidence?
2.- Should this be considered a breaking change? (my humble opinion is that given that this is an alpha release I wouldn't bother... but then again.. your call)
3.- How can I help? If there's a test plan or similar, I can chip in

[pragone]

BTW... on your other comment. It would be nice to have the query aggregation support of mongoose in bookshelf as well... But at least, with the help I got in stackoverflow I was able to do the aggregation queries myself so no biggie... I would say that this functionality is desirable, but not required for being prod-ready. Again, my 2 cents :)

[derrickmehaffy]

@pragone So the travisCI tests implemented on the monorepo don't actually test core functionality (working on that with cypress but honestly that is more geared towards adminUI testing) I believe all the current tests are done on mongo anyway. @lauriejim generally tests each DB manually before release.

Obviously in the future we want each possible DB type to be tested automatically but that is WIP. Also in the great words of Captain Jack Sparrow Tests are "more like guidelines" than actual results 😉

It would only be considered a breaking change if you were to update an existing application and it would not start without modifications (core changes required to start the server) in this case I don't think so as you actually have default values assigned if there are none in the config.

Realistically what we should do is test on both platforms Postgres/MySQL with different database versions (recommended and above) so we don't get any surprises after the PR is merged and is live.

And yes it may not be required for production (aggregation support) but with the end goal being: Strapi is frontend and backend agnostic, we should strive to have the same functionality no matter the backend database as much as possible. This also includes the small things like unique constraint keys, ect. But a bit out of the scope of this specific issue.

With the addition of SQLite support (via @pierreburgy 's PR ) we will also have another bookshelf related database to test in the future as well which will be important to allow people to test easily. Among this change and my purposed one on merging the templates for mongoose/bookshelf to allow users to flip flop between either side without having to regenerate their models we are pushing closer to the goal of being both frontend and backend agnostic 😄

TLDR: We are getting there, we just need more people like you to just jump in the deep end and find the small things to work on.